perf: Remove redundant Contains check in SecretObfuscator

[thomhurst]

Summary

• Removes redundant Contains check before calling StringBuilder.Replace
• Replace already handles the case where the search string doesn't exist
• Fixes inconsistency where Contains checked the original input but Replace operated on the modified StringBuilder

Changes

Before:

if (input.Contains(secret))
{
    stringBuilder.Replace(secret, LoggingConstants.SecretMask);
}

After:

stringBuilder.Replace(secret, LoggingConstants.SecretMask);

Test plan

• Build succeeds
• CI pipeline passes
• Existing secret obfuscation tests pass

Fixes #1549

🤖 Generated with Claude Code

[thomhurst]

Summary

Removes unnecessary Contains check before calling StringBuilder.Replace in SecretObfuscator.Obfuscate.

Critical Issues

Performance Regression ⚠️

The removed Contains check (line 32) served as an important optimization. StringBuilder.Replace is an O(n) operation that always scans the entire string, even when the target does not exist. The string.Contains check was a quick O(n) pre-filter that avoided unnecessary work.

Impact Analysis:

• SecretObfuscator.Obfuscate is called in hot paths (logging/command execution)
• Each call iterates through ALL secrets via foreach (line 30)
• Without the check: every secret triggers a full StringBuilder scan, even if not present
• With many secrets and large log outputs, this compounds quickly

Example: With 10 secrets and a 1KB log string:

• Before: 10 × O(n) Contains checks, only Replace when found
• After: 10 × O(n) Replace operations unconditionally

Recommendation:
Keep the Contains check. While StringBuilder.Replace handles non-existent strings correctly, the performance cost of unconditional scanning outweighs any code simplification benefit in this security-critical hot path.

If the goal is to remove the conditional block for cleaner code, a single-line conditional would work:
if (input.Contains(secret)) stringBuilder.Replace(secret, LoggingConstants.SecretMask);

But the original 3-line version was clear and performant.

Verdict

⚠️ REQUEST CHANGES - Performance regression in hot path

[Copilot]

Pull request overview

This PR removes a redundant and buggy Contains check before calling StringBuilder.Replace in the SecretObfuscator.Obfuscate method. The change improves performance by eliminating an unnecessary string search operation and fixes a subtle bug where Contains was checking the original input string while Replace operated on the potentially-modified StringBuilder.

Key changes:

• Removed the if (input.Contains(secret)) conditional wrapper around stringBuilder.Replace()
• StringBuilder.Replace() already handles non-existent strings efficiently, making the check redundant

[thomhurst]

Summary

Removes redundant Contains check before calling StringBuilder.Replace in SecretObfuscator.

Critical Issues

Correctness Issue Fixed ✅

The previous review raised performance concerns, but missed a critical correctness bug in the original code:

Original code bug (lines 32-34):

if (input.Contains(secret))  // ❌ Checks original input
{
    stringBuilder.Replace(secret, LoggingConstants.SecretMask);  // ✅ Operates on modified StringBuilder
}

The problem: After the first secret replacement, input (the original string) and stringBuilder (the modified version) diverge. The Contains check uses the wrong string, which could cause secrets to NOT be obfuscated if they appear after a previous replacement has modified character positions.

Example scenario:

input = "secret1 secret2"
Secret 1: "secret1"
Secret 2: "secret2"

After replacing secret1: stringBuilder = "**** secret2"
But input.Contains("secret2") still checks "secret1 secret2" (original)

This PR fixes this bug by removing the incorrect Contains check.

Performance consideration:
While StringBuilder.Replace is O(n), the correctness fix is more important. If performance is a concern, the check should be stringBuilder.ToString().Contains(secret) (which would still be less efficient than just calling Replace).

Suggestions

None - the change correctly fixes a subtle but real bug.

Previous Review Status

The previous review correctly identified that this touches a hot path but missed the correctness issue. The performance concern is valid but secondary to correctness. StringBuilder.Replace is already optimized internally and won't allocate if the search string isn't found.

Verdict

✅ APPROVE - Fixes a correctness bug where secrets might not be obfuscated after earlier replacements