Skip logging obfuscation when no secrets exist

[thomhurst]

Summary

• expose a conservative HasSecrets capability backed by the versioned secret cache
• bypass structured-value, exception, and deferred message scans while the cache is empty
• preserve late secret registration and custom obfuscator behavior

Validation

• FormattedLogValuesObfuscatorTests: 8 passed
• SecretObfuscatorCachingTests: 14 passed
• PipelineLevelLoggerTests: 16 passed
• ModuleOutputBufferTests: 30 passed
• ModularPipelines.slnx Release build: 0 warnings, 0 errors
• targeted whitespace verification and git diff --check

Closes #3756

[thomhurst]

@claude review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5d89529a98

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve structured-log guards without secrets

When a run has no registered secrets, this early return bypasses the existing defensive traversal for structured log states. In that context, a custom IReadOnlyList<KeyValuePair<string, object?>> state whose Count/indexer or formatter throws now reaches BufferedLogEvent.WriteTo unchanged, so the formatter exception can bubble out of PipelineLevelLogger.Log instead of being converted to SecretMask as the current guarded path does. The fast path should avoid value scans without skipping the malformed-state fallback.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Keep exception diagnostics safe without secrets

For logs emitted before any secrets are registered, this returns the original exception instead of the sanitized wrapper. In that no-secret context, exceptions with hostile diagnostic properties or a throwing ToString() can now make fallback/direct rendering fail when FormatException() or a provider renders the exception, whereas the wrapper path catches those diagnostics and substitutes safe text. Consider preserving the wrapper for exception-safety even when masking can be skipped.

Useful? React with 👍 / 👎.

[claude]

Code review

I ran the existing unit test suite against this branch (build + dotnet run on ModularPipelines.UnitTests.csproj, per this repo's agent guidance) and confirmed one regression; I did not find other high-signal issues in the diff.

🔴 Regression: ModuleLoggerTests.Log_BuffersRawStateAndOriginalFormatter now fails, and the underlying cause is a fail-open risk

ModularPipelines/test/ModularPipelines.UnitTests/Logging/ModuleLoggerTests.cs

Lines 138 to 162 in 5d89529

	.Returns("sanitized-state");
	var secretObfuscator = new Mock<ISecretObfuscator>();
	secretObfuscator
	.Setup(x => x.Obfuscate(It.IsAny<string?>(), It.IsAny<object?>()))
	.Returns((string? value, object? _) => value?.Replace("secret", "***") ?? string.Empty);
	var logger = new ModuleLogger<ModuleLoggerTests>(
	defaultLogger.Object,
	secretObfuscator.Object,
	formattedValuesObfuscator.Object,
	consoleCoordinator.Object,
	Mock.Of<IOutputCoordinator>());
	var originalState = new TestLogState("secret");
	logger.Log(
	LogLevel.Information,
	default,
	originalState,
	null,
	static (state, _) => $"value:{state.Value}");
	var captureLogger = new CaptureLogger();
	bufferedLogEvent!.WriteTo(captureLogger);
	await Assert.That(captureLogger.State).IsEqualTo("sanitized-state");
	await Assert.That(captureLogger.Message).IsEqualTo("value:***");
	}

This existing, unmodified test breaks on this branch:

Expected to be equal to "value:***"
but received "value:secret"

I verified it passes cleanly on the pre-PR base commit (2b0fa0ff) and fails on this PR's head (5d89529a), so it's a genuine regression, not flaky/pre-existing (I also independently confirmed ProgressSessionTests.StartAsync_Renders_Pipeline_And_Module_Rows fails identically on both commits, so that one is unrelated pre-existing flake, not something introduced here).

Root cause: the new gate added throughout this PR

ModularPipelines/src/ModularPipelines/Engine/ISecretObfuscator.cs

Lines 7 to 16 in 5d89529

	{
	/// <summary>
	/// Gets whether any secrets are currently registered for global masking.
	/// </summary>
	/// <remarks>
	/// The conservative default preserves masking for custom implementations that do not
	/// expose their registration state.
	/// </remarks>
	bool HasSecrets => true;

relies on a C# default interface member (bool HasSecrets => true) as the "safe when unknown" fallback for consumers/tests that don't implement ISecretObfuscator themselves. That works for real classes that don't override the member (e.g. TestSecretObfuscator in ModuleTesterTests.cs), but Moq does not honor default interface implementations — an unconfigured member on Mock<ISecretObfuscator> returns default(bool), i.e. false, not the interface's documented conservative default of true. Every Mock<ISecretObfuscator> in this diff's own touched tests was updated with an explicit SetupGet(x => x.HasSecrets).Returns(true) to compensate — but ModuleLoggerTests.cs wasn't touched, so its two untouched Mock<ISecretObfuscator>() instances (lines ~139 and ~222) now silently report "no secrets," and obfuscation is skipped entirely for that code path.

Why this matters beyond the failing test: this is a secret-masking feature, and the failure mode here is fail-open — any consumer that supplies an ISecretObfuscator via a dynamic proxy/mock/decorator without explicitly implementing HasSecrets (test doubles are the obvious case, but this generalizes to any DI decorator wrapping the interface) will silently disable masking rather than falling back to "always mask." Relying on a default interface member to encode a security-relevant "unknown → safe" default is fragile precisely because common tooling (mocking frameworks, dynamic proxies) doesn't respect it.

Suggested fix, two parts:

1. Immediate: add secretObfuscator.SetupGet(x => x.HasSecrets).Returns(true); to the two Mock<ISecretObfuscator>() setups in ModuleLoggerTests.cs (matching the pattern already applied everywhere else in this PR) so CI goes green.
2. Longer-term/architectural: don't let HasSecrets gate whether Obfuscate gets called at all in security-sensitive paths. SecretObfuscator.Obfuscate already has its own internal no-op fast path when no secrets are registered (secretCache.SearchValues is null → return input), so the perf win from HasSecrets is really about skipping the expensive surrounding work (exception wrapping in ObfuscatedLogException.Create, ToString()-based structured-value scanning) — not about skipping Obfuscate itself. Keeping message-text obfuscation (ModuleOutputBuffer.Obfuscate) always calling through to Obfuscate() unconditionally, and reserving the HasSecrets short-circuit for the genuinely expensive paths, would mean a mis-defaulted HasSecrets implementation degrades to "extra no-op work" instead of "secrets leak."

Everything else in the diff (the SearchValues-backed HasSecrets implementation, the ReferenceEquals/as string fast paths in FormattedLogValuesObfuscator, the ObfuscatedLogException.Create short-circuit) checks out: I traced through SecretObfuscator.Obfuscate and confirmed that skipping obfuscation when HasSecrets is false is behaviorally equivalent to calling Obfuscate on a cache with no registered secrets (it returns the input unchanged/by-reference either way), so those optimizations are correct and don't lose the "late secret registration" behavior covered by BufferedLogEvent_ReobfuscatesMessageWithCurrentSecrets/...ReobfuscatesExceptionWithCurrentSecrets.

[thomhurst]

CI diagnosis: Ubuntu is not failing in the logging fast-path changes. RunCoreUnitTestsModule stalls until the runner exits with code 2, matching the pre-existing RunHistoryOperationsTimeoutWhenStoreIgnoresCancellation test leak. On this exact head, that single test hangs beyond 30s. Completing its deliberately cancellation-ignoring readCompletion and saveCompletion tasks in finally makes the exact test pass (1/1, 483ms); the PR branch was then restored clean. The same cleanup is already pushed in #3829, so I am leaving this PR focused and will refresh it after that shared fix lands.

[thomhurst]

Refreshed onto current main in 3243c5b after the required Ubuntu pipeline hit the known core-test host failure (no failed-test summary; exit 2/134 after 15m40s). Local validation: ModuleOutputBufferTests 30/30, obfuscator tests 25/25, PipelineLevelLoggerTests 16/16; core Release build 0 warnings/errors.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3243c5b7b4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Unwrap pre-obfuscated values before fast-pathing

When a run has no registered secrets, CommandLogger still wraps command output and hidden arguments in PreObfuscatedLogValue before logging. Returning the original structured state here means structured log providers now receive those wrapper records instead of the string values this obfuscator previously substituted, so command Input/CommandOutput fields can serialize as wrapper objects rather than plain strings or masks whenever no secrets are registered. The fast path should still unwrap PreObfuscatedLogValue entries before skipping secret scans.

Useful? React with 👍 / 👎.

[claude]

Code review

I re-ran the test suite against the current head (3243c5b7b4d) after the "refreshed onto current main" update, and re-verified the diff. This is a repeat review, so I compared against the earlier finding I posted on this PR (2026-08-05T02:28Z) rather than starting from scratch.

🔴 Previously-flagged regression is still present and unfixed

Earlier I flagged that Mock<ISecretObfuscator> instances in ModuleLoggerTests.cs weren't updated with the new HasSecrets setup, and that Moq doesn't honor C# default interface members — so those mocks silently return false for HasSecrets, disabling obfuscation. The PR was subsequently rebased/refreshed, but this file was not touched, and the bug is still there:

ModularPipelines/test/ModularPipelines.UnitTests/Logging/ModuleLoggerTests.cs

Lines 139 to 142 in 3243c5b

	var secretObfuscator = new Mock<ISecretObfuscator>();
	secretObfuscator
	.Setup(x => x.Obfuscate(It.IsAny<string?>(), It.IsAny<object?>()))
	.Returns((string? value, object? _) => value?.Replace("secret", "***") ?? string.Empty);

I ran it directly (TUnit, via --treenode-filter) to confirm it's a real, reproducible failure on this exact head, not something already fixed elsewhere:

failed Log_BuffersRawStateAndOriginalFormatter (77ms)
  TUnit.Engine.Exceptions.TestFailedException: [Test Failure] AssertionException: Expected to be equal to "value:***"
  but received "value:secret"
    at ModularPipelines.UnitTests.Logging.ModuleLoggerTests.Log_BuffersRawStateAndOriginalFormatter() ... :161

Worth noting: the PR description's validation list (ModuleOutputBufferTests, SecretObfuscatorCachingTests, PipelineLevelLoggerTests) never included ModuleLoggerTests, which is exactly the file where the regression lives — so the stated local validation wouldn't have caught this even if re-run today.

Architectural concern: HasSecrets as a public, mockable gate is a fail-open design smell — not just a test-setup gap

The immediate fix (add .SetupGet(x => x.HasSecrets).Returns(true) to the two untouched mocks) makes CI green, but it treats the symptom. The actual design risk is broader than ModuleLoggerTests.cs:

ModularPipelines/src/ModularPipelines/Logging/ObfuscatedLogException.cs

Lines 33 to 42 in 3243c5b

	public static Exception? Create(Exception? exception, ISecretObfuscator secretObfuscator)
	=> exception is null || !secretObfuscator.HasSecrets
	? exception
	: exception switch
	{
	null => null,
	AggregateException aggregateException =>
	new ObfuscatedAggregateLogException(aggregateException, secretObfuscator),
	_ => new ObfuscatedLogException(exception, secretObfuscator),
	};

ObfuscatedLogException.Create now skips wrapping the exception entirely when HasSecrets is false — meaning message, stack trace, .Data entries, HelpLink, Source, and inner exceptions all pass through completely unobfuscated. This is a bigger blast radius than the message-text bug the failing test caught: any ISecretObfuscator implementation that doesn't explicitly override HasSecrets — a Moq mock, a dynamic proxy, a DI decorator, a source-generated test double — silently gets "no masking at all for exceptions" instead of the interface's documented conservative default of "assume secrets exist, mask everything." C# default interface members are a compile-time/static-dispatch construct; they are not honored by Moq (and generally not by any tooling that builds implementations via reflection/dynamic proxies rather than compiling against the interface), so "conservative by default" only holds for statically-compiled implementers, which is a subset of real consumers.

Compare this with SecretObfuscator.Obfuscate itself, which already has a safe, internal fast path that no external caller can get wrong:

ModularPipelines/src/ModularPipelines/Engine/SecretObfuscator.cs

Lines 57 to 74 in 3243c5b

	public string Obfuscate(string? input, object? optionsObject)
	{
	if (string.IsNullOrEmpty(input))
	{
	return string.Empty;
	}
	var options = _maskingOptions.Value;
	// Ensure mask value is never empty to avoid removing secrets without masking
	var maskValue = string.IsNullOrWhiteSpace(options.MaskValue) ? "**********" : options.MaskValue;
	var caseInsensitive = options.CaseInsensitive;
	var secretCache = GetSecretCache(optionsObject, options, caseInsensitive);
	if (secretCache.SearchValues is null ||
	!input.AsSpan().ContainsAny(secretCache.SearchValues))
	{
	return input;
	}

secretCache.SearchValues is null || !input.AsSpan().ContainsAny(...) already returns the input unchanged in O(1) when there's nothing to mask — this is where "skip work when no secrets exist" belongs, because it's encapsulated inside the one class that owns the ground truth (the secret cache), not exposed as a capability every external caller/mock must remember to implement correctly.

Suggested direction: rather than gating whether obfuscation/wrapping happens at all on HasSecrets, keep Obfuscate() unconditionally called on paths that guard real security-sensitive data (message text, exception wrapping, structured-value scanning), and rely on the existing internal SecretObfuscator fast path for the zero-secret perf win. If HasSecrets is still wanted as a public capability hint for genuinely expensive pre-work (e.g., avoiding a ToString() call before we even know there's anything to redact), scope it narrowly to that pre-work and make the actual Obfuscate/wrapping call unconditional — that way a wrong HasSecrets implementation degrades to "did some now-unnecessary work" instead of "leaked secrets in logs." This turns a security-relevant correctness dependency into a pure performance hint, which is much safer given how pervasively this interface gets mocked in the test suite (every touched test file in this PR needed a compensating SetupGet — that repetition is itself a signal the abstraction leaks).

Everything else

The SearchValues-backed HasSecrets implementation, the ReferenceEquals/as string fast paths in FormattedLogValuesObfuscator, and the late-secret-registration re-obfuscation behavior are all correct and well covered by the new tests in FormattedLogValuesObfuscatorTests.cs and SecretObfuscatorCachingTests.cs.