Split optional dependency-backed features into packages

[PrzemyslawKlys]

Summary

• isolate PGP-backed security.txt verification into DomainDetective.Pgp
• isolate Playwright/ImageSharp typosquatting support into DomainDetective.Visual
• prepare a DomainDetective.CtSql package and registration seam without moving the live PostgreSQL implementation yet
• add package-specific NuGet readmes and align the build scripts/package plan with the new package set

Notes

• the existing DomainDetective API stays intact; optional features are activated by registering the new packages
• DomainDetective.CtSql is intentionally scaffold-only in this PR so the active CT/PostgreSQL work can move later without churn
• visual fingerprint dimensions now preserve the original image size instead of the resized 9x8 hash canvas; this fixes a pre-existing refactor-adjacent bug
• passive CT defaults are more conservative for shared services, including slower pacing and lower per-run crt.sh request limits
• DomainDetective.Visual supports image fingerprinting on .NET Framework targets; browser capture requires .NET 8 or later

Real CT validation

• exact eurofins.com metadata was validated live through CertSpotter with DER hydration into thumbprint, subject, issuer, serial, validity, authentication profile, and CT source fields
• passive discovery for eurofins.com was validated live through crt.sh and returned real subdomain candidates
• CtSql remains prepared but not moved here, so existing CT/PostgreSQL consumers are not cut over in this PR

Validation

• dotnet build DomainDetective.Visual/DomainDetective.Visual.csproj -c Debug --framework net8.0
• dotnet build DomainDetective.Pgp/DomainDetective.Pgp.csproj -c Debug --framework net8.0
• dotnet build DomainDetective.CtSql/DomainDetective.CtSql.csproj -c Debug --framework net472
• dotnet test DomainDetective.Tests/DomainDetective.Tests.csproj -c Debug --framework net10.0 --filter "FullyQualifiedName~SignedSecurityTxtWarnsWhenPgpVerificationIsUnavailable|FullyQualifiedName~TryHydrateExactCtMetadataThumbprintFromCrtShCertificateAsync_ComputesThumbprintFromDownloadedCertificate|FullyQualifiedName~TryHydrateExactCtMetadataThumbprintFromCrtShCertificateAsync_AcceptsWildcardSubjectAlternativeNames"
• dotnet test DomainDetective.Tests/DomainDetective.Tests.csproj -c Debug --framework net8.0 --filter "FullyQualifiedName~SignedSecurityTxtWarnsWhenPgpVerificationIsUnavailable|FullyQualifiedName~TryHydrateExactCtMetadataThumbprintFromCrtShCertificateAsync_ComputesThumbprintFromDownloadedCertificate|FullyQualifiedName~TryHydrateExactCtMetadataThumbprintFromCrtShCertificateAsync_AcceptsWildcardSubjectAlternativeNames"
• dotnet test DomainDetective.Tests/DomainDetective.Tests.csproj -c Debug --framework net472 --filter "FullyQualifiedName~SignedSecurityTxtWarnsWhenPgpVerificationIsUnavailable|FullyQualifiedName~TryHydrateExactCtMetadataThumbprintFromCrtShCertificateAsync_ComputesThumbprintFromDownloadedCertificate|FullyQualifiedName~TryHydrateExactCtMetadataThumbprintFromCrtShCertificateAsync_AcceptsWildcardSubjectAlternativeNames"
• DOMAINDETECTIVE_INTEGRATION=1 dotnet test DomainDetective.Tests/DomainDetective.Tests.csproj -c Debug --framework net8.0 --filter "FullyQualifiedName~CaptureAsync_CtEvidenceRefreshProfile_HydratesEurofinsExactHostFromLivePassiveCtSources|FullyQualifiedName~CaptureAsync_CtDiscoveryOnlyProfile_DiscoversEurofinsSubdomainsFromLivePassiveCtSources"

Native CT validation

• direct/native CT polling was tested against eurofins.com with --enable-native-ct-logs --native-ct-log-only, no passive crt.sh/CertSpotter fallback, and bounded log/entry caps
• live native CT reached real CT logs including Cloudflare Nimbus 2026 and Google Xenon/Solera and produced native CT diagnostics
• the bounded native run found 0 eurofins.com subdomains in the sampled tail window, which matches the design constraint: direct CT logs are append-only streams and need cursoring/indexing/backfill depth to become dependable domain search

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[claude]

Code Review

This is a well-structured refactoring. The registration-seam pattern is clean, backward-compatible, and extensible. A few items worth addressing:

---

Bug: BuildFingerprint returns post-resize dimensions

In DomainDetective.Visual/DomainDetectiveVisualProvider.cs, image.Width and image.Height are read after the image is mutated to 9×8:

image.Mutate(ctx => ctx.Resize(new ResizeOptions { Size = new Size(9, 8), ... }).Grayscale()); return (hash.ToString("x16"), image.Width, image.Height); // ^^^^^^^^^^^ ^^^^^^^^^^^^ // Always 9 Always 8 (post-resize)

If the intent is to return the original screenshot dimensions (for display or comparison purposes), capture them before mutating:

var originalWidth = image.Width; var originalHeight = image.Height; image.Mutate(...); return (hash.ToString("x16"), originalWidth, originalHeight);

If 9×8 is intentional (e.g. the caller only cares about the hash), then Width/Height in the return tuple are meaningless and the type signature should be reconsidered.

---

Missing solution entries for DomainDetective.Visual and DomainDetective.CtSql

The .sln diff only adds DomainDetective.Pgp. DomainDetective.Visual and DomainDetective.CtSql are not registered in DomainDetective.sln, so they won't appear in Visual Studio's Solution Explorer and won't be included in solution-wide builds/analysis. The projects build correctly when referenced directly (as the dotnet build validation confirms), but this is likely an oversight.

---

Thread safety of static delegate fields

DomainDetectiveOptionalFeatures holds four static fields that are written during registration and read later:

private static Func<...>? _securityTxtPgpVerifier; private static Func<...>? _typosquattingVisualFingerprintBuilder; // ...

The read side already captures into a local variable (good pattern), but the fields are neither volatile nor protected by a lock. If registration can race with first use on another thread, this is a data race. In practice, registering once at startup before any analysis runs avoids this, but it's an implicit contract. Marking the fields volatile (safe for reference types in .NET) would make the intent explicit and correct on all memory models:

private static volatile Func<string, string, (bool IsVerified, string ClearText)>? _securityTxtPgpVerifier;

---

Spurious await Task.CompletedTask in the #else branch

DomainDetectiveVisualProvider.CaptureBrowserArtifactAsync has an async method signature with a #else path that only exists to satisfy the compiler:

#else await Task.CompletedTask.ConfigureAwait(false); return null; #endif

This may generate a CS1998 warning ("async method lacks await operators") on non-NET8 targets and introduces a pointless state-machine allocation. Consider splitting the method or using a non-async wrapper for the fallback:

#else return Task.FromResult<TyposquattingVisualArtifact?>(null); #endif

(The method would need to drop async in the #else case, or the body can be restructured so the #else path delegates to a non-async helper.)

---

Minor: RegisterVisualProvider registers both delegates atomically, but HasVisualProvider checks them independently

HasVisualProvider returns true if either delegate is non-null, but RegisterVisualProvider always sets both together. This means there is no scenario in practice where one is set and the other is not — the guard is slightly wider than necessary. This is harmless but could be tightened to _typosquattingVisualFingerprintBuilder != null && _typosquattingBrowserCapture != null for correctness.

---

Positive notes

• The local-variable capture pattern in TryVerifySecurityTxtSignature, BuildTyposquattingFingerprint, and CaptureTyposquattingBrowserArtifactAsync is correct and avoids TOCTOU issues.
• ArgumentNullException guards on all Register* calls are good.
• Graceful degradation (ExtractClearText fallback, null returns) is well-thought-out.
• The #if NET8_0_OR_GREATER guard inside DomainDetective.Visual (rather than scattered through core) is much cleaner.
• NuGet README files are clear and include concrete startup snippets.
• Build script changes (structured plan objects, return instead of exit 0) are improvements.

[claude]

PR Review: Split optional dependency-backed features into packages

Overall this is a clean architectural improvement. Moving Playwright, ImageSharp, and PgpCore out of the base package is the right call for trimming/AOT scenarios and for consumers who don't need those features. The delegate-registry pattern in DomainDetectiveOptionalFeatures is pragmatic and keeps the core API surface unchanged. A few items worth addressing before merge:

---

Bugs / Correctness

1. Playwright IBrowserContext not disposed — DomainDetectiveVisualProvider.cs

var context = await browser.NewContextAsync(...).ConfigureAwait(false); var page = await context.NewPageAsync().ConfigureAwait(false);

context is var, not await using var. Playwright contexts hold browser state and should be disposed. browser is correctly await using, but the context leaks if an exception is thrown after it is created. Fix:

await using var context = await browser.NewContextAsync(...).ConfigureAwait(false);

2. Linked CancellationTokenSource is created but its token is never used

using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); linkedCts.CancelAfter(options.BrowserCaptureTimeout); // ... but linkedCts.Token is never passed to any Playwright call

The timeout fires but nothing observes linkedCts.Token, so the cancellation has no effect. Either pass linkedCts.Token to the operations that support it, or remove the linked CTS and rely solely on the Playwright Timeout parameter (which is already set). Note: this was copied from the original code, so it is a pre-existing bug — but now is a good time to fix it since the code is being moved.

---

Minor Issues

3. Unnecessary .Trim() on FingerprintHex — TyposquattingVisualSimilarity.cs

return (fingerprint.Value.FingerprintHex.Trim(), fingerprint.Value.Width, fingerprint.Value.Height);

hash.ToString("x16") never produces whitespace, so .Trim() is a no-op. The caller does not need it either. Safe to remove to avoid implying the value might have leading/trailing whitespace.

4. publicKey! null-suppression is redundant — SecurityTXTAnalysis.cs

if (DomainDetectiveOptionalFeatures.TryVerifySecurityTxtSignature(txt, pgpPublicKey!, ...))

TryVerifySecurityTxtSignature already guards string.IsNullOrWhiteSpace(publicKey) internally, so the ! is unnecessary. The outer !string.IsNullOrEmpty(pgpPublicKey) check already established it is non-null at this point, but the suppression is misleading about ownership of the null contract.

---

Design Observations

5. Silent overwrite on repeated Register() calls

All three Register* methods silently replace an existing registration:

_securityTxtPgpVerifier = verifier ?? throw new ArgumentNullException(nameof(verifier));

In production this is usually fine (startup runs once), but in test suites it can cause subtle ordering dependencies when multiple tests register different providers. Consider at least throwing InvalidOperationException if a provider is already set, or providing a Reset() method (internal/test-only) to clear state between tests.

6. HasVisualProvider is not atomically consistent

public static bool HasVisualProvider => _typosquattingVisualFingerprintBuilder != null && _typosquattingBrowserCapture != null;

Two separate volatile reads that are not atomically paired. RegisterVisualProvider sets them in sequence; a reader on another thread could observe one set and the other not yet set, causing HasVisualProvider to return false even though registration is in progress. In practice this is a startup-time check and the race window is microseconds, but if correctness is important here a simple lock or an Interlocked-based approach would eliminate the concern.

---

Positive Observations

Accidental bug fix in BuildFingerprint

The original TyposquattingVisualSimilarity.cs returned image.Width, image.Height after image.Mutate(resize), which would always be 9×8 (the resize target). The new DomainDetectiveVisualProvider.BuildFingerprint correctly captures originalWidth/originalHeight before the resize and returns those. This is a real bug fix that ships silently with the refactor — worth calling out in the commit message or changelog so it is not missed.

exit 0 → return in PowerShell scripts

Correct change. exit 0 terminates the hosting process (breaks dot-sourcing and pipeline composition); return exits the script scope cleanly. The [pscustomobject] plan output is also cleaner for downstream pipeline consumers than Write-Host.

[Nullable[bool]] tri-state pattern in Build-Project.ps1

Clean approach for distinguishing "not provided", "true", and "false" for PublishNuget/PublishCli. Pairs well with the JSON config fallback.

---

Questions

• DomainDetective.CtSql ships as a NuGet package (listed in project.build.json) but contains no implementation and the base package still depends on Npgsql. Is there a versioning plan to signal to users that this package is intentionally a stub? A [Obsolete] or a prominent note in the NuGet README about its scaffold-only status would help avoid confusion.
• The app.config binding redirect removal for System.Threading.Tasks.Extensions — was this verified on net472? The new DomainDetective.CtSql.csproj and DomainDetective.Visual.csproj both reference System.Threading.Tasks.Extensions 4.6.0. If another transitive dependency pulls in an older version, the missing redirect could cause FileLoadException at runtime on net472.

[claude]

PR Review: Split optional dependency-backed features into packages

Good structural change overall — the registration pattern is clean and the public API surface is preserved. A few items worth addressing before merge:

---

Bug / Behavioral change: browser-capture timeout coverage regressed

Old code (TyposquattingVisualSimilarity.Capture.cs):

using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); linkedCts.CancelAfter(options.BrowserCaptureTimeout);

The linked CTS applied the timeout to the entire operation, including Playwright.CreateAsync() and browser.NewContextAsync().

New code (DomainDetectiveVisualProvider.cs):
The cancellationToken is passed directly; the timeout is only applied to GotoAsync via the Timeout option. If Playwright.CreateAsync() or browser.NewContextAsync() hang, there is now no cancellation guard on those calls.

Suggestion: either restore the linked-CTS approach inside CaptureBrowserArtifactCoreAsync, or add a top-level CancellationTokenSource.CreateLinkedTokenSource there.

---

Good catch: image dimensions fix

In the old TyposquattingVisualSimilarity.cs, BuildFingerprint returned image.Width/image.Height after resizing to 9×8, so callers always saw (Width=9, Height=8). The new DomainDetectiveVisualProvider.BuildFingerprint saves originalWidth/originalHeight before the resize — this is a genuine bug fix worth calling out in the PR description.

---

No guard against double-registration

DomainDetectiveOptionalFeatures.RegisterPgpVerifier (and the other Register* methods) silently overwrite the previous delegate if called twice. In a DI or module-import context this could mask misconfiguration. Consider either throwing on re-registration or making it a no-op:

public static void RegisterPgpVerifier(Func<string, string, (bool, string)> verifier) { if (_securityTxtPgpVerifier != null) throw new InvalidOperationException("A PGP verifier is already registered."); _securityTxtPgpVerifier = verifier ?? throw new ArgumentNullException(nameof(verifier)); }

---

app.config binding redirect removal may break net472 tests

DomainDetective.Tests/app.config drops the System.Threading.Tasks.Extensions binding redirect. The new DomainDetective.Pgp and DomainDetective.Visual packages both pull System.Threading.Tasks.Extensions 4.6.0 for net472 targets. If any transitive dependency in the test project's net472 build references an older version, this removal could cause FileLoadException or MissingMethodException at runtime on net472. Please verify that the net472 test run still passes, or explain why the redirect is no longer needed.

---

Minor: DomainDetectiveVisualProvider catches bare Exception via catch-all in caller

DomainDetectiveOptionalFeatures.CaptureTyposquattingBrowserArtifactAsync is called inside a bare catch { return null; } in TyposquattingVisualSimilarity.Capture.cs. The inner DomainDetectiveVisualProvider already catches PlaywrightException and TimeoutException. The outer bare catch is fine as a last-resort safety net, but it will also swallow OperationCanceledException (including cancellation from the token passed by the caller), which could prevent clean cancellation propagation. Consider catching only non-cancellation exceptions or re-throwing OperationCanceledException:

catch (OperationCanceledException) { throw; } catch { return null; }

---

Nit: DomainDetective.CtSql description is placeholder-ish

Directory.Build.props describes it as "Prepared package shell for future CT SQL components". Once this ships to NuGet, consumers who install it will find an essentially empty package. Consider adding a note in the NuGet README that the package is intentionally a pre-release scaffold (or publishing it as a pre-release version only until the implementation lands).

---

Summary

Area	Finding	Severity
DomainDetectiveVisualProvider	Browser launch timeout not covered	Medium
DomainDetectiveVisualProvider	Image dimension fix (was a bug)	Positive
DomainDetectiveOptionalFeatures	No double-registration guard	Low
DomainDetective.Tests/app.config	Binding redirect removed — verify net472	Low
TyposquattingVisualSimilarity.Capture.cs	Bare catch swallows cancellation	Low
DomainDetective.CtSql NuGet README	Scaffold nature not clearly flagged	Nit

The registration/delegation architecture is solid and the separation of concerns is well-executed. The main item to resolve before merging is the timeout regression in the browser capture path.

[claude]

PR Review: Split optional dependency-backed features into packages

Overall this is a well-structured refactor. The plugin-registration pattern is clean, the separation of concerns is clear, and the build-script improvements (structured plan output, exit 0 → return) are genuinely better. A few items worth addressing before merge:

---

Bug: EncryptionKeys not disposed in DomainDetective.Pgp

File: DomainDetective.Pgp/DomainDetectivePgpSignatureVerifier.cs

var keys = new EncryptionKeys(publicKey); // IDisposable — never disposed using var pgp = new PGP(keys);

PgpCore.EncryptionKeys implements IDisposable (it holds key streams). Wrap it:

using var keys = new EncryptionKeys(publicKey); using var pgp = new PGP(keys);

---

Behaviour change: fingerprint now returns original dimensions instead of resized (9×8)

Old code (TyposquattingVisualSimilarity.cs) called image.Mutate(resize to 9×8) and then returned image.Width, image.Height — which was always 9 and 8 after the resize.

New code (DomainDetectiveVisualProvider.cs) correctly captures originalWidth/originalHeight before the resize and returns those instead. This is a silent fix that changes the stored dimensions for every fingerprinted image. If any downstream consumers or tests compare stored width/height values, they will see a difference. Worth calling out explicitly in the PR description.

---

app.config binding redirect removed without explanation

File: DomainDetective.Tests/app.config

The System.Threading.Tasks.Extensions assembly binding redirect (4.x → 4.2.1.0) was removed. On .NET Framework 4.7.2 this redirect exists to avoid version conflicts between the BCL version and the NuGet-provided version. If DomainDetective.Pgp or DomainDetective.Visual pulls in a conflicting transitive reference, tests may start failing on net472. Please verify this is safe or document why the redirect is no longer needed.

---

Double-registration silently overwrites

RegisterPgpVerifier, RegisterVisualProvider, and RegisterCtSqlExactMetadataProvider all overwrite without any guard. Calling Register() twice (e.g. in a test setup that doesn't reset state) silently replaces the prior registration. Consider at minimum a debug-mode guard or a no-op-if-already-registered path:

public static void RegisterPgpVerifier(Func<...> verifier) { if (_securityTxtPgpVerifier != null) throw new InvalidOperationException("A PGP verifier is already registered."); _securityTxtPgpVerifier = verifier ?? throw new ArgumentNullException(nameof(verifier)); }

This is especially relevant for test isolation — static state persists across tests in the same process.

---

DomainDetectiveCtSqlRegistration has an asymmetric signature

The PGP and Visual registrations are Register() with no arguments; the CT SQL registration requires the provider delegate as a parameter. This asymmetry is fine (and intentional since there's no bundled implementation yet), but it breaks the pattern and may confuse users who see the other two registrations. A short doc comment explaining why it's caller-supplied would help.

---

Minor notes

• Fingerprint trim removed: artifact.FingerprintHex.Trim() → artifact.FingerprintHex. Fine if whitespace can't appear here, but worth a note.
• No tests for DomainDetectiveOptionalFeatures: The registration hub itself has no tests — in particular, there's nothing testing the "provider not registered → graceful fallback" paths. Given this is the core seam of the whole feature, a small unit test suite would add confidence.
• DomainDetective.CtSql NuGet description: "Prepared package shell for future CT SQL extraction" will confuse NuGet users who install it and find an empty package. Consider a more explicit message like "Placeholder package. No functionality in this release; the PostgreSQL CT implementation remains in the base DomainDetective package."
• Build script: $shouldRunNuGet/$shouldRunCli priority chain is correct — the switch overrides the nullable bool which overrides the config default. The removed -not $Plan guard is fine since the $Plan block now returns early.

[claude]

Code Review

Overall this is a well-structured refactor that cleanly separates optional heavy dependencies (PGP, Playwright, ImageSharp) into opt-in packages. The architecture is sound and the new tests add meaningful coverage. A few items worth discussing:

---

Issues / Bugs

1. Hardcoded IP for crt.sh PostgreSQL (CertificateInventoryCapture.CtMetadataBackfill.cs)

// Hostname defaults to IP 91.199.212.73

The BuildCrtShPostgreSqlConnectionString now defaults to the IP address 91.199.212.73 instead of "crt.sh". If Sectigo ever rotates that IP, backfill will silently fail (Npgsql will throw a generic connection error with no indication that the configured host is stale). Since there's already a CrtShPostgreSqlHostOverride escape hatch, consider at least adding a code comment documenting why the IP is preferred (avoids extra DNS RTT?) and that it may need updating.

---

2. IsCtCertificateMetadataMissing logic flip (&& → ||) — potential performance regression

The change from all fields must be missing to any field missing is semantically correct for completeness checking. However, on an existing dataset where prior runs only populated a subset of fields (e.g., thumbprint present but EKU absent), this will now trigger a backfill query for every such entry on the next run. This could cause a significant burst of crt.sh PostgreSQL queries on first run after the upgrade. Worth calling out explicitly in the PR description / migration notes, and potentially worth adding a rate-limiting safeguard or a one-time migration note.

---

Design Concerns

3. DomainDetective.CtSql is a registration scaffold, not a self-contained package

DomainDetective.Pgp and DomainDetective.Visual each contain the implementation and just call Register() with no external delegate. DomainDetective.CtSql requires the caller to supply the exactMetadataProvider delegate:

// Pgp/Visual pattern: DomainDetectivePgpRegistration.Register(); // self-contained // CtSql pattern: DomainDetectiveCtSqlRegistration.Register(myProviderDelegate); // caller must supply impl

This is intentional (the real implementation lives outside this repo), but it creates an asymmetric API surface. Consider adding a XML doc comment to Register() explaining what callers are expected to pass in and where the reference implementation lives, so future consumers don't have to reverse-engineer the intent.

---

4. Broad catch in TyposquattingVisualSimilarity.Capture.cs

catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { throw; } catch { return null; // swallows all exceptions silently }

Swallowing all exceptions (including OutOfMemoryException, StackOverflowException, AccessViolationException) makes debugging failures very difficult. Consider catching only Exception and logging a warning at minimum, or narrowing to Playwright-specific exceptions (PlaywrightException).

---

Code Quality

5. Reflection in test is fragile

ExactCtMetadataBackfill_UsesDirectPostgreSqlMetadataWhenPassiveFallbackIsDisabled uses BindingFlags.NonPublic | BindingFlags.Instance to invoke an internal method. If that method is renamed or its signature changes, the test will fail at runtime rather than compile time. Consider making the method internal + [assembly: InternalsVisibleTo("DomainDetective.Tests")] if it needs to be tested directly, which would give compile-time safety.

---

6. Minor: volatile fields in DomainDetectiveOptionalFeatures are correct but implicit

Using volatile for single-assignment singletons is valid and avoids the lock overhead of full Interlocked. Worth a brief comment clarifying the intent (write-once after startup, reads are safe after registration) so future maintainers don't add unnecessary synchronization.

---

Positive Highlights

• The BuildWildcardCandidateHost fix correctly handles registrable roots (e.g., eurofins.pl) to avoid broadening to *.pl — the new tests cover this edge case well.
• ResolveTargetBudget special-casing maxTargets == 1 to prefer HTTPS is a sensible default with a dedicated test.
• The BuildWildcardCandidateHost and domain-batched CT query tests are well-structured unit tests with clear intent.
• Separating ImageSharp and Playwright into DomainDetective.Visual (with correct TFM-conditional references: net8.0/net10.0 only for Playwright) is clean.
• The [Collection("HttpListener")] addition in TestMtaStsNarrative and 127.0.0.1 fix are good defensive test hygiene.

---

Review generated with Claude Sonnet 4.6

[claude]

PR Review: Split optional dependency-backed features into packages

Great architectural direction overall. The delegate-registry pattern cleanly isolates heavy dependencies while preserving full backward compatibility. A few things worth looking at before merge:

---

Resource leak in PGP verifier

File: DomainDetective.Pgp/DomainDetectivePgpSignatureVerifier.cs

var keys = new EncryptionKeys(publicKey); // no using / no dispose using var pgp = new PGP(keys);

EncryptionKeys wraps cryptographic key material. If it implements IDisposable (check the PgpCore source), this is a resource leak on every call. Either wrap it in using or confirm it is not disposable.

---

Non-atomic visual provider registration

File: DomainDetective/DomainDetectiveOptionalFeatures.cs

RegisterVisualProvider writes two separate volatile fields:

_typosquattingVisualFingerprintBuilder = fingerprintBuilder ...; _typosquattingBrowserCapture = browserCapture ...; // second write

Between these two stores, HasVisualProvider returns false while BuildTyposquattingFingerprint could return a non-null value (the first field is already set). In practice registration only happens at startup before any analysis, so the window is harmless — but the invariant is fragile. An Interlocked-swap on a single record/tuple, or a dedicated registration-complete flag set last, would close the gap cleanly.

---

Unused net472 assembly references in scaffold package

File: DomainDetective.CtSql/DomainDetective.CtSql.csproj

<ItemGroup Condition="'$(TargetFramework)' == 'net472'"> <PackageReference Include="System.Threading.Tasks.Extensions" Version="4.6.0" /> <Reference Include="System.IO.Compression" /> <Reference Include="System.IO.Compression.FileSystem" /> <Reference Include="System.ComponentModel.DataAnnotations" /> </ItemGroup>

The CtSql package has no implementation yet. These look like a copy-paste from another .csproj. System.IO.Compression and System.ComponentModel.DataAnnotations are not referenced by anything in the package. Worth trimming so consumers do not pull in unused assemblies from a scaffold.

---

Double timeout in browser capture

File: DomainDetective.Visual/DomainDetectiveVisualProvider.cs

The method creates a linked CTS with CancelAfter(options.BrowserCaptureTimeout) and passes the same duration as the Playwright GotoAsync timeout:

using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); linkedCts.CancelAfter(options.BrowserCaptureTimeout); // ... await page.GotoAsync(url, new PageGotoOptions { Timeout = (float)options.BrowserCaptureTimeout.TotalMilliseconds // same value }).WaitAsync(linkedCts.Token)...

Both paths are caught (OperationCanceledException vs TimeoutException), but the result is identical silence either way. If the intent is a hard wall-clock cap for the entire capture, using linkedCts alone is cleaner; the inner Playwright timeout can be omitted or set longer.

---

Silent overwrite on double-registration

RegisterPgpVerifier, RegisterVisualProvider, and RegisterCtSqlExactMetadataProvider all silently replace an existing registration. A second call from a misconfigured startup is invisible. Throwing InvalidOperationException when already registered (or at least logging a warning) would catch wiring bugs early.

---

Reflection-based tests on private methods

File: DomainDetective.Tests/TestCertificateInventoryCapture.cs

Two new tests call BackfillMissingCtCertificateMetadataExactAsync and BackfillMissingCtCertificateMetadataAsync via reflection. The Assert.NotNull(method) guard catches a rename, but the object[] parameter list is unchecked at compile time — a signature change produces a runtime TargetParameterCountException rather than a build error. Flagging this so it is not forgotten if those signatures evolve.

---

Minor: IgnoreHTTPSErrors coupling to DisableTlsValidation

File: DomainDetective.Visual/DomainDetectiveVisualProvider.cs

IgnoreHTTPSErrors = options.HttpRequestOptions.DisableTlsValidation

Intentional for typosquatting analysis (squatted domains often have invalid certs), but a short inline comment would prevent a future reader from reading this as a mistake.

---

What looks solid

• The volatile delegate pattern is the right lightweight mechanism for startup-time registration with no locking overhead.
• Graceful degradation is well-handled: TryVerifySecurityTxtSignature returns false and the caller falls back to ExtractClearText; browser capture returns null; no features silently fail in a way that produces wrong results.
• The new test for MaxTargets == 1 HTTPS preference (CaptureAsync_PrefersHttpsOverMail_WhenOnlyOneTargetIsAllowed) closes a real edge case cleanly.
• The DomainDetective.CtSql README is clear that this is a stable-identity placeholder — the package name commitment story makes sense.
• The build-script refactor from Write-Host/exit 0 to [pscustomobject]/return is a nice improvement that makes plan output composable.

[claude]

Code Review: Split Optional Dependency-Backed Features Into Packages

This is a well-structured refactoring that cleanly isolates heavy optional dependencies into separate NuGet packages. The overall architecture is sound and the plugin pattern is a good fit for this use case. Here's detailed feedback:

---

Architecture & Design — Positive

• Plugin pattern in DomainDetectiveOptionalFeatures.cs is clean and avoids DI container coupling. The static service locator works well for an SDK-style library.
• Conditional compilation (#if NET8_0_OR_GREATER) in DomainDetectiveVisualProvider.cs correctly restricts Playwright to supported runtimes without penalizing .NET 4.7.2 users.
• Backward compatibility is fully maintained — existing callers with no optional packages registered still get the same behavior via graceful degradation.
• using statements and ConfigureAwait(false) in the async Playwright/ImageSharp paths are correctly applied.

---

Issues Found

1. Silent Signature Skip — Security Risk (SecurityTXTAnalysis.cs)

When the PGP package is not registered but a security.txt contains a PGP-signed block, the code silently falls through to ExtractClearText() without any warning. A user who forgets to call DomainDetectivePgpRegistration.Register() gets no indication that signature verification is being skipped.

Recommendation: Log a warning when a PGP signature is detected but no verifier is registered:

if (DomainDetectiveOptionalFeatures.TryVerifySecurityTxtSignature(...)) { // verified path } else { // PGP verifier not registered — warn if a signature header is present if (txt.Contains("-----BEGIN PGP SIGNED MESSAGE-----")) { Logger.WriteWarningCode(SecurityTxtCodes.SignatureVerifyFailed, "PGP signature found but no verifier registered. Install DomainDetective.Pgp and call DomainDetectivePgpRegistration.Register()."); } txt = ExtractClearText(txt); }

2. Thread Safety in Registration (DomainDetectiveOptionalFeatures.cs)

The volatile keyword prevents stale cache reads but doesn't protect against a race condition if two threads call a RegisterXxx() method simultaneously — both can read null, both proceed to assign, and the last write wins silently. For a one-time startup registration this is low risk in practice, but worth hardening:

// Before _securityTxtPgpVerifier = verifier ?? throw new ArgumentNullException(nameof(verifier)); // After — idempotent, first-wins, thread-safe Interlocked.CompareExchange(ref _securityTxtPgpVerifier, verifier ?? throw new ArgumentNullException(nameof(verifier)), null);

3. No Input Validation in DomainDetectivePgpSignatureVerifier.VerifyClearSignedMessage()

signedText and publicKey have no null/empty guard. A null publicKey causes an opaque exception from inside PgpCore rather than a clear ArgumentNullException at the call boundary.

internal static async Task<(bool IsVerified, string ClearText)> VerifyClearSignedMessage( string signedText, string publicKey) { if (string.IsNullOrWhiteSpace(signedText)) throw new ArgumentNullException(nameof(signedText)); if (string.IsNullOrWhiteSpace(publicKey)) throw new ArgumentNullException(nameof(publicKey)); // ... }

4. Test Isolation — Static State Pollution (DomainDetectiveOptionalFeatures.cs)

Once a provider is registered it cannot be removed. If tests run in-process (e.g., xUnit with parallel test classes), registrations from one test bleed into others. Consider adding an internal-only reset method for test cleanup:

#if DEBUG internal static void ResetForTesting() { _securityTxtPgpVerifier = null; _visualProvider = null; _ctSqlExactMetadataProvider = null; } #endif

Or expose it as internal with [InternalsVisibleTo("DomainDetective.Tests")].

5. Timeout Validation in CaptureBrowserArtifactAsync()

The timeout value from options.BrowserTimeoutForScreenshotMs is used directly in CancellationTokenSource(TimeSpan.FromMilliseconds(...)). A zero or negative value will throw ArgumentOutOfRangeException from CancellationTokenSource. A small guard prevents a confusing stack trace:

var timeout = Math.Max(options.BrowserTimeoutForScreenshotMs, 1000); // minimum 1s using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); cts.CancelAfter(TimeSpan.FromMilliseconds(timeout));

---

Minor / Nits

• DomainDetectiveCtSqlRegistration.cs is a near-empty scaffold. The NuGet.README.md for this package should clearly say "not yet functional" so consumers don't ship it thinking it works. Currently the README says "prepared package shell" which is good — consider also adding a [Obsolete] attribute or XML doc note on the Register() method itself.

• Fingerprint versioning: BuildFingerprint() returns a raw 64-bit hex string with no version prefix. If the algorithm changes (e.g., different resize kernel or bit comparison order), old and new fingerprints become incompatible silently. Consider prefixing with a version byte ("v1:" + hex) so future changes can be detected.

• VisualProviderRegistration sealed inner class: The class is internal and only used in one place. It could be a simple record or value tuple to reduce boilerplate, but this is purely stylistic.

---

Summary

Area	Verdict
Architecture	✅ Clean plugin pattern, good separation of concerns
Backward Compatibility	✅ Fully maintained
Security	⚠️ Silent PGP skip needs a warning log
Thread Safety	⚠️ Low-risk race in registration; Interlocked preferred
Input Validation	⚠️ Missing null guards in PGP verifier
Testability	⚠️ Static state needs a reset path for test isolation
Code Style	✅ Consistent with existing codebase patterns

The main items worth addressing before merge are the silent PGP skip (#1) and the missing null guards (#3). The thread-safety and test isolation points (#2, #4) are lower priority but good to fix before this becomes a widely-used pattern in the codebase.

[codecov]

Codecov Report

❌ Patch coverage is 73.79501% with 473 lines in your changes missing coverage. Please review.
✅ Project coverage is 45.29%. Comparing base (614de71) to head (3c071f7).
⚠️ Report is 42 commits behind head on master.

Files with missing lines	Patch %	Lines
...ive/CertificateTransparency/CtCertificateRecord.cs	46.09%	118 Missing and 20 partials ⚠️
.../CertificateInventoryCapture.CtMetadataBackfill.cs	68.75%	73 Missing and 22 partials ⚠️
...nDetective.Visual/DomainDetectiveVisualProvider.cs	0.00%	83 Missing ⚠️
...tive/CertificateTransparency/CtIngestionPlanner.cs	91.47%	17 Missing and 23 partials ⚠️
DomainDetective/DomainDetectiveOptionalFeatures.cs	35.41%	28 Missing and 3 partials ⚠️
...tive/CertificateTransparency/CtProviderProfiles.cs	88.82%	14 Missing and 7 partials ⚠️
...icateTransparency/CtProviderRuntimeStateUpdater.cs	73.23%	7 Missing and 12 partials ⚠️
...tive/CertificateTransparency/CtCertificateQuery.cs	83.87%	6 Missing and 9 partials ⚠️
...tective.Pgp/DomainDetectivePgpSignatureVerifier.cs	23.07%	6 Missing and 4 partials ⚠️
...ncy/CtCertificateTransparencyProviderExtensions.cs	74.07%	2 Missing and 5 partials ⚠️
... and 6 more

Additional details and impacted files

@@ Coverage Diff @@ ## master #1172 +/- ## ========================================== + Coverage 44.10% 45.29% +1.18%  ========================================== Files 1123 1150 +27 Lines 104269 106330 +2061 Branches 21871 22216 +345 ========================================== + Hits 45993 48164 +2171  + Misses 52428 52275 -153  - Partials 5848 5891 +43 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[claude]

Code Review: Split optional dependency-backed features into packages

Overall this is a clean, well-motivated PR. Isolating PgpCore, Playwright/ImageSharp, and DBAClientX.PostgreSql into opt-in packages is the right call for a library — it keeps the base DomainDetective package lean and avoids forcing heavy transitive dependencies on consumers who don't need them. The registration seam design is simple and effective.

---

Architecture & Design

Strengths:

• The static-volatile registry in DomainDetectiveOptionalFeatures is appropriate for startup-time registration. volatile provides the necessary memory-visibility guarantee for the publish-once pattern without needing locks.
• InternalsVisibleTo is scoped correctly — only DomainDetective.CtSql gets internal access (because it calls internal CertificateInventoryCapture helpers), while .Pgp and .Visual consume only the public registration API. This is the right boundary.
• Framework-conditional Playwright compilation (#if NET8_0_OR_GREATER) is cleanly handled with a no-op stub for downlevel targets.
• Cancellation handling in CaptureBrowserArtifactCoreAsync is careful: the when (linkedCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) predicate correctly distinguishes a browser capture timeout from an externally requested cancellation.
• NuGet metadata (PackageId, Description, README, tags) is centrally managed in Directory.Build.props — no duplication across the three .csproj files.

---

Specific observations

1. Redundant null guard in DomainDetectivePgpProvider (minor)

DomainDetective.Pgp/DomainDetectivePgpSignatureVerifier.cs:11-28

if (signedText == null) throw new ArgumentNullException(...); if (string.IsNullOrWhiteSpace(signedText)) throw new ArgumentException(...);

The null check is redundant — IsNullOrWhiteSpace already handles null. The same pattern is repeated for publicKey. Consider collapsing each pair to the single IsNullOrWhiteSpace guard (or keep the null check and drop the whitespace check if empty-string inputs are semantically the same as null for callers).

2. NormalizeCtMetadataCandidate is duplicated (minor)

DomainDetective.CtSql/CrtShPostgreSqlMetadataProvider.cs:158 contains a private copy of NormalizeCtMetadataCandidate that is byte-for-byte identical to CertificateInventoryCapture.CtMetadataBackfill.cs:1721. Since DomainDetective.CtSql already has InternalsVisibleTo access, it could call the internal version directly:

// CtSql already has InternalsVisibleTo — could call: CertificateInventoryCapture.NormalizeCtMetadataCandidate(name, preserveWildcard: true)

Alternatively, the method could be promoted to internal on a shared helper class. The duplication is low-risk but will silently diverge if the normalization logic ever changes in one place.

3. CrtShPostgreSqlMetadataProvider disposal (medium)

DomainDetective.CtSql/CrtShPostgreSqlMetadataProvider.cs:12-13

internal sealed class CrtShPostgreSqlMetadataProvider : ICtSqlMetadataProvider { private readonly PostgreSql _client = new();

CrtShPostgreSqlMetadataProvider doesn't implement IDisposable. Whether PostgreSql (DBAClientX) needs explicit disposal (connection pool cleanup, etc.) is worth confirming. Additionally, DomainDetectiveOptionalFeatures.RegisterCtSqlMetadataProvider is last-write-wins but never disposes the replaced provider, so any held resources on the old instance would leak. If PostgreSql is stateless / doesn't hold connections open, this is a non-issue — worth a comment either way.

4. Directory.Build.props description mismatch (minor)

<Description>Prepared package shell for future CT SQL components in DomainDetective.</Description>

This description says "prepared package shell" but CrtShPostgreSqlMetadataProvider is a real, working implementation (not just a scaffold). The NuGet package description will be misleading to consumers once published. The NuGet README is accurate; the .props description should match it.

5. DomainDetective.CtSql not covered by the test project

The test .csproj references .Pgp and .Visual but not .CtSql. This means CrtShPostgreSqlMetadataProvider — including QueryExactMetadataAsync, QueryDomainMetadataAsync, MapRow, and ReadDateTimeOffset — has no unit-test coverage. The PR notes CtSql as "scaffold-only" but the implementation is substantive. Adding even a few offline unit tests (using mock/fake IDataRecord rows) would give the MapRow and BuildCrtShPostgreSqlConnectionString paths some coverage before the package ships.

6. Connection string concatenation (low risk, worth noting)

DomainDetective.CtSql/CrtShPostgreSqlMetadataProvider.cs:126-141

return PostgreSql.BuildConnectionString(...) + $";Timeout={timeoutSeconds};Command Timeout={timeoutSeconds}";

Appending raw key-value pairs to an opaque connection string is fragile if BuildConnectionString ever changes its output format or already includes a Timeout key. An NpgsqlConnectionStringBuilder (or DBAClientX's equivalent, if one exists) would be safer. Not blocking for a public crt.sh target where the connection string is fixed, but worth revisiting.

---

Positive callouts

• The fingerprint fix (returning originalWidth/originalHeight instead of the resized 9×8 canvas dimensions) is the right call, and the comment in the PR summary explains the pre-existing bug clearly.
• Passive CT defaults being more conservative for shared services is a good operational judgment.
• The NuGet READMEs are well-written and include actionable startup snippets.
• The CaptureBrowserArtifactCoreAsync WaitAsync approach on Playwright.CreateAsync() is a smart workaround for the API not accepting CancellationToken directly.

---

Summary: Items 2–4 are the most actionable before merge. Items 1 and 6 are minor style/robustness nits. Item 5 (test coverage for CtSql) can reasonably follow in a subsequent PR if the package is intentionally "prepared but not yet battle-tested." No blocking issues found.

[claude]

Code Review — Split optional dependency-backed features into packages

This is a well-motivated architectural change. Isolating heavy optional dependencies (PgpCore, Playwright, ImageSharp, Npgsql/DBAClientX) into separate NuGet packages is the right call — it keeps the base package lean and avoids forcing consumers to carry transitive dependencies they don't use. The registration-seam pattern in DomainDetectiveOptionalFeatures is a clean way to wire the optional providers in without polluting the core with conditional #if compile-time guards. Overall the shape of the change looks good. A few things worth discussing:

---

Static global registration state

DomainDetectiveOptionalFeatures uses static fields to hold the registered delegates. That works fine in single-process production use, but it creates test-isolation friction: any test that registers a provider (or checks HasPgpVerifier, HasVisualProvider, etc.) can bleed into adjacent tests, which likely explains the SignedSecurityTxtWarnsWhenPgpVerificationIsUnavailable test needing to tolerate a registered verifier. If you have parallel test runs across frameworks this can become flaky. Worth either:

• adding a package-scoped reset helper (e.g. DomainDetectiveOptionalFeatures.Reset()) used in test teardown/fixtures, or
• documenting the once-per-process registration intent clearly so consumers know not to call RegisterX repeatedly.

---

RegisterCtSqlMetadataProvider visibility asymmetry

There are two registration paths for the SQL provider — the internal bundled path and the public RegisterCtSqlExactMetadataProvider caller-supplied path. The naming (Exact vs. no qualifier) doesn't make the intent obvious from the outside. A brief XML doc summary on the public overload explaining when to use it vs. just adding the DomainDetective.CtSql package reference would help consumers who discover it via IntelliSense.

---

crt.sh FTS query targeting certificate not certificate_identity

The README for DomainDetective.CtSql explicitly notes the query targets the certificate table via FTS rather than the superseded certificate_identity table. This is the correct choice for crt.sh's current schema, but it is worth leaving a comment in CrtShPostgreSqlMetadataProvider.cs itself explaining why — someone reading the code later may wonder why the query doesn't use certificate_and_identities or the identity table, and the README won't be their first stop.

---

ImageSharp 2.x / 3.x split behavior differences

DomainDetective.Visual conditionally references ImageSharp 2.1.13 on net472 and 3.1.12 on net8.0+. These are major versions with a breaking API surface (particularly around pixel buffers and Image.Load overloads). The fingerprint implementation should be verified to produce identical byte-level output across both versions — if the hash canvas operations diverge subtly between 2.x and 3.x, fingerprints will not be comparable across target frameworks. This matters if a net8.0 service produces fingerprints compared against fingerprints from a net472 desktop tool.

---

CT pacing/limit changes

Increasing PassiveCtCrtShMinimumSpacing from 5 s to 15 s and decreasing PassiveCtCrtShMaximumRequestsPerRun from 8 to 5 is the right direction for a shared guest replica. Just worth confirming these new defaults are also reflected in any documentation or CLI help text that quoted the old numbers, so operators who tuned around the old values are not surprised.

---

Visual fingerprint dimension fix

The bug fix preserving original image dimensions instead of the 9x8 hash-canvas dimensions is correct. It is a silent correctness issue (the previous output was structurally valid but misleading metadata). Good catch.

---

Test coverage

The new TestCtCertificateQuery.cs and TestCtIngestionPlanner.cs files are solid additions, and the updated TestSecurityTXTAnalysis.cs with the PGP-unavailability warning path covers the most important new behavior from the consumer's perspective. The integration tests gated on DOMAINDETECTIVE_INTEGRATION=1 are the right pattern for network-dependent assertions.

---

Minor notes

• The ApplyCtDiscoveryOnlyProfile() and ApplyCtEvidenceRefreshProfile() methods are a clean API addition — explicit profiles beat boolean soup.
• The CrtShPostgreSqlMaximumConcurrentRequests default of 2 is conservative and appropriate for the shared crt.sh guest replica; worth keeping that context in the XML doc since callers will be tempted to bump it.
• CI is still running at time of review; no conclusions drawn from check status.

---

Overall this is a well-structured PR with clear separation of concerns, good iterative commit history, and real validation evidence. The points above are mostly documentation/isolation nits rather than blockers.

[claude]

Code Review

Overall this is a well-structured PR with clear architectural intent. Separating heavy optional dependencies (PGP, Playwright/ImageSharp, PostgreSQL) into dedicated packages is the right call and the registration seam pattern in DomainDetectiveOptionalFeatures is clean and minimal.

Below are observations from most to least critical.

---

Bug / Correctness

TryVerifySecurityTxtSignature has no exception guard around the delegate call

DomainDetectiveOptionalFeatures.cs (TryVerifySecurityTxtSignature):

var result = verifier(signedText, publicKey!);

PgpCore will throw (e.g. PgpException, InvalidDataException, FormatException) on a malformed key or signature. The method's bool-returning signature implies it should not throw, but a malformed payload will propagate an unhandled exception to any caller that assumes graceful failure. The test SignedSecurityTxtWarnsWhenPgpVerificationIsUnavailable only covers the no-verifier path and doesn't exercise a bad key/signature case.

Suggest wrapping the delegate call in a try/catch that logs and returns false.

---

Design / API

DomainDetectiveCtSqlRegistration.Register() overloads register fundamentally different things

Register() (no args) registers the full domain-batching ICtSqlMetadataProvider, while Register(Func<...>) registers an exact-host-only provider via a different slot. They look identical at the call site but behave very differently in the backfill pipeline (the exact-only path emits a verbose warning and falls back to per-host queries). Callers who choose the single-argument overload don't get domain batching and might not know why.

Consider using distinct method names — e.g. RegisterBundledProvider() and RegisterCustomExactProvider(...) — to make the difference self-documenting.

ICtSqlMetadataProvider is internal but DomainDetectiveCtSqlRegistration.Register() is public

The public Register() method in DomainDetective.CtSql calls DomainDetectiveOptionalFeatures.RegisterCtSqlMetadataProvider(ICtSqlMetadataProvider) which is internal. The design relies on InternalsVisibleTo to bridge this. That's fine architecturally, but it means a caller outside the DomainDetective package family can only use the exact-provider overload. If the intent is that only the bundled package or test harnesses ever supply a full domain provider, this is correct — but it is worth a brief doc comment on the internal method so the restriction is explicit.

---

Performance / Correctness

Double-budget timeout in CaptureBrowserArtifactCoreAsync

DomainDetective.Visual/DomainDetectiveVisualProvider.cs:

linkedCts.CancelAfter(options.BrowserCaptureTimeout); // whole-method budget // ... browser launch, context creation consume some of that budget ... await page.GotoAsync(url, new PageGotoOptions { Timeout = (float)options.BrowserCaptureTimeout.TotalMilliseconds // same value, not remaining time })

PageGotoOptions.Timeout is measured from when GotoAsync starts, not from method entry. If browser launch takes 5 s out of a 30 s budget, the navigation can still run for up to 30 s before Playwright's internal timer fires, while the linkedCts fires after only 25 s more. In the common case this just means the CTS fires first and returns null correctly, but the Playwright timeout will never actually trigger — the CTS always beats it. If the intent is to bound the navigation step specifically, the Playwright timeout should be the remaining budget, not the full budget.

---

Code Quality

Redundant null check before IsNullOrWhiteSpace in DomainDetectivePgpProvider

DomainDetective.Pgp/DomainDetectivePgpSignatureVerifier.cs:

if (signedText == null) throw new ArgumentNullException(...); if (string.IsNullOrWhiteSpace(signedText)) throw new ArgumentException(...);

IsNullOrWhiteSpace already returns true for null, so the first check is unreachable. This is a minor style issue — the double-check pattern is common as documentation, but given this is a new file it's worth aligning with one approach.

Brace style inconsistency between packages

AGENTS.MD specifies K&R/OTBS for C#. The new package files (CrtShPostgreSqlMetadataProvider.cs, DomainDetectiveVisualProvider.cs) use K&R correctly. DomainDetectiveOptionalFeatures.cs (core project) uses Allman. The inconsistency is pre-existing in the core project, but the new public-API surface in DomainDetectiveOptionalFeatures is a high-visibility file — worth aligning on the next pass if not immediately.

---

Security

Empty password in default PostgreSQL connection string

CrtShPostgreSqlMetadataProvider.cs:

PostgreSql.BuildConnectionString( host: "crt.sh", database: "certwatch", username: "guest", password: string.Empty, // guest user on public crt.sh ...)

This is the documented public crt.sh read-only guest interface, so an empty password is expected and correct. The concern is that callers may copy this pattern when setting up a private mirror. A doc comment (or README note) that the guest/empty-password defaults are specific to the public crt.sh instance would reduce the risk of misconfigured private deployments.

---

Test Coverage

• SignedSecurityTxtWarnsWhenPgpVerificationIsUnavailable covers the no-verifier warning path. There is no test that exercises the verifier when registered but given a bad key/signature — that is the path that currently has the missing exception guard noted above.
• The visual fingerprint (BuildFingerprint) and browser capture wiring have no unit tests. Integration tests via DOMAINDETECTIVE_INTEGRATION=1 are valuable but a unit test that registers a mock visual provider and confirms BuildTyposquattingFingerprint routes through it would lock in the registration contract cheaply.
• DomainDetectiveOptionalFeatures.HasCtSqlProvider is true if either provider slot is populated. It would be useful to have a test that registers each slot independently and confirms the property value.

---

Minor

• DomainDetective.CtSql.csproj targets net472/net8.0/net10.0 but does not include the System.Threading.Tasks.Extensions reference for net472 that the Pgp and Visual projects include. If DBAClientX.PostgreSql pulls in any ValueTask-based APIs on net472, this will fail to build on .NET Framework. Worth verifying, even if currently benign.
• The limit formula Math.Min(Math.Max(32, normalizedHosts.Count * 8), 512) in QueryDomainMetadataAsync is reasonable but undocumented. A comment explaining the 8 rows/host heuristic and the 512 ceiling would help the next reviewer understand the trade-off.

---

Good work on the architecture overall. The core abstraction is sound and the scaffold approach for CtSql is a sensible way to avoid churn before the live implementation lands.