Prevent secret leaks from environment table wrapping

[thomhurst]

Summary

• render environment-variable values as single-line, ellipsized Text cells so long or multiline values never become multiple physical log lines
• mask values for common credential-bearing variable names, including authentication, connection-string, passphrase, Redis, Azure Functions storage, and NuGet endpoint credential conventions
• honor SecretMaskingOptions.MaskValue for name-based masking, including the safe default fallback for blank configuration
• preserve known non-secret PWD, OLDPWD, and SSH_AUTH_SOCK diagnostics

Validation

• PipelineInitializerTests: 25 passed, covering narrow-console truncation, CR/LF normalization, sensitive-name conventions, exemptions, and configured mask behavior
• ModularPipelines.sln Release build: 0 errors; 227 existing warnings
• touched-file info-level formatting verification passed

Rotation/expiry of any credentials exposed by historical logs remains an external owner/security action.

Closes #3548

Follow-up review fix

• Masks unchanged raw environment values before newline escaping or table ellipsis can transform them, preserving downstream whole-value secret masking.
• Keeps already-obfuscated long values visible as their configured mask.
• Regression coverage: PipelineInitializerTests (26/26 passing).
• Core Release build: 0 errors (existing warnings only).

Render-width follow-up

• Replaces the fixed raw-value cutoff with a conservative capacity derived from the actual Spectre console width, widest environment-variable name, table borders, separator, and cell padding.
• Masks unchanged raw values before rendering whenever they cannot fit intact; preserves values that demonstrably fit and values already replaced by the obfuscator.
• Regression covers the reported 32-character CONFIG value at width 40 plus the exact safe-fit boundary.
• PipelineInitializerTests: 28/28 passing; scoped formatter clean.

Header-width follow-up

• Includes the Name header width when calculating safe raw-value capacity, preventing Spectre padding around one-character variable names from exposing truncated raw values.
• Regression coverage: PipelineInitializerTests (29/29 passing); scoped whitespace formatter clean.

Partial-obfuscation follow-up

• Only an obfuscator result equal to the effective whole-value mask is rendered as safely transformed; partially redacted composite values are replaced wholesale.
• PipelineInitializerTests: 30/30 passed; scoped whitespace formatter clean.

[claude]

Review: Prevent secret leaks from environment table wrapping

The fix correctly addresses the root cause from #3548: NoWrap + Ellipsis() on the value column keeps each env value on one physical console row (so runner-side ::add-mask:: can still match), MakeSingleLine neutralizes embedded CR/LF as a belt-and-suspenders measure, and the switch from Markup.Escape(...) strings to Text cells is a nice side benefit — Text never interprets markup, so it's safer against control-sequence injection than the old escaped-string approach. Tests cover the wrap/mask/CRLF cases well, including a dedicated narrow-width render check.

One architectural point worth considering, not a blocker:

Sensitive-name detection duplicates an existing configurable pattern instead of reusing it. HttpRequestFormatter/HttpResponseFormatter already solve the "which names are secret-ish" problem via HttpLoggingOptions.SensitiveHeaderNames — a public, IOptions-configurable, user-overridable list with a documented default. This PR introduces a second, parallel mechanism: SensitiveEnvironmentVariableNameParts is a private static readonly string[] baked into PipelineInitializer, with no way for a consuming pipeline to extend or override it.

Why that matters here specifically: the substring match (name.Contains("PWD", ...), .Contains("KEY", ...)) is intentionally broad per the issue's spec, but it also means standard, non-secret shell variables like PWD/OLDPWD are now always masked, and any consumer whose own naming convention happens to contain one of these substrings (e.g. a TURNKEY_ENDPOINT or PRIMARY_KEY_COLUMN env var) loses visibility into that value in the startup table with no opt-out. The HttpLoggingOptions precedent exists precisely to let consumers tune this kind of heuristic per-project.

Suggestion: lift SensitiveEnvironmentVariableNameParts into a small options record (or extend SecretMaskingOptions) following the HttpLoggingOptions.SensitiveHeaderNames shape — public static Default list, IOptions<T>-injected, user-overridable via PipelineBuilder.Services. That keeps the two "mask by name heuristic" implementations in the codebase consistent, and gives consumers an escape hatch for false positives without needing another PR/fork. Not required for this fix to be correct or safe to ship, but worth a follow-up given PWD will now be masked in effectively every pipeline run.

Everything else looks solid — validation section in the PR description matches what I see in the diff, and I didn't find any correctness issues with the masking/truncation logic itself.

[claude]

Code review

No new changes since the previous review (the only commit added since 00ac5f7 is a merge of main, with no functional diff to the touched files) — so this is a verification pass rather than a fresh review.

Independently verified locally against the current HEAD (a7ccb9b):

• dotnet build ModularPipelines.sln -c Release: 0 errors, 0 warnings
• PipelineInitializerTests via TUnit --treenode-filter: 10/10 passed

The masking/truncation logic in CreateEnvironmentVariablesTable holds up: sensitive-name detection short-circuits before obfuscation runs, NoWrap + .Ellipsis() keeps each value on one physical console row, and MakeSingleLine escapes embedded CR/LF as a second line of defense. Switching from Markup.Escape(...) strings to Text cells is a good safety improvement too. No correctness issues found.

Outstanding architectural point from the prior review — still unaddressed, still not a blocker:

SensitiveEnvironmentVariableNameParts (PipelineInitializer.cs#L24-L32) is a hardcoded private static readonly string[] with no way for a consuming pipeline to extend or override it. This duplicates a pattern the codebase already solves correctly elsewhere:

• SecretMaskingOptions (src/ModularPipelines/Options/SecretMaskingOptions.cs) already exists as a DI-configurable options record for exactly this kind of "how do we detect/mask secrets" behavior — consumers configure it via builder.Services.Configure<SecretMaskingOptions>(...).
• HttpLoggingOptions.SensitiveHeaderNames follows the same shape (IReadOnlyList<string> + static Default) for the equivalent "which names are secret-ish" problem on the HTTP logging side.

Concretely, the substring match here is intentionally broad (Contains("KEY"), Contains("PWD"), etc.), so it now always masks PWD/OLDPWD and any variable whose name happens to contain one of these substrings (e.g. TURNKEY_ENDPOINT, PRIMARY_KEY_COLUMN). Every consumer of this framework loses visibility into those values in the startup table, with no opt-out short of a fork or a follow-up PR to this repo.

Suggested follow-up: fold the sensitive name-part list into SecretMaskingOptions and inject it into PipelineInitializer the same way ISecretObfuscator is already injected. That keeps the two "mask by name heuristic" mechanisms in the codebase consistent and gives consumers a real escape hatch for false positives.

Not required for this fix to be correct or safe to ship — flagging again since it's still open, for the author/maintainer to decide whether to track as a follow-up issue.

[claude]

Code review

Reviewed the secret-masking / table-rendering changes in PipelineInitializer.CreateEnvironmentVariablesTable. The masking logic itself (name-heuristic short-circuit before obfuscation, CR/LF normalization applied after obfuscation, ordinal-ignore-case matching, Text instead of Markup.Escape to avoid re-interpretation) is sound and the new tests cover the intended behaviors well. One high-signal correctness bug found in the layout change — see inline comment.

No CLAUDE.md compliance issues found (only the root CLAUDE.md applies to these files, and the changes don't touch build/CI/module/codegen conventions it governs).

[thomhurst]

Addressed the unresolved narrow-terminal review finding in 2587cd81b2.

• Removed NoWrap from the Value column so Spectre can shrink it instead of collapsing the Name column vertically.
• Kept Text.Ellipsis() so secret-bearing values remain single-line and truncated.
• Added a regression assertion that LONG_VALUE remains intact.

TDD: the focused test failed before the production change because the name rendered one character per line; it now passes.

Validation:

• PipelineInitializerTests: 10/10 passed
• targeted whitespace verification: passed
• changed-file analyzers with EnableCiAnalyzers=true: passed
• core Release build: 0 errors (227 existing warnings)

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: 2587cd81b2

ℹ️ 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".

[thomhurst]

Rebased onto current main at 8c247942fb. Patch equivalence preserved (025bc8f12a39e9da30554c3ec3df159559e3ba92 before and after). New head: 6ee031ef63.

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6ee031ef63

ℹ️ 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".

[claude]

Code review

No issues found. Checked for bugs, CLAUDE.md compliance, and correctness of the secret-masking logic.

What I checked:

• CreateEnvironmentVariablesTable / IsSensitiveEnvironmentVariableName / MakeSingleLine in PipelineInitializer.cs — verified the mask-vs-obfuscate ordering never renders a sensitive-named value before masking, that obfuscate() runs before MakeSingleLine, and that Text.Ellipsis() is a valid Spectre.Console fluent API (confirmed against upstream source) that correctly bounds the Value column without collapsing the Name column — addressing the narrow-terminal issue from the earlier review round.
• Switching AddRow(string, string) (which requires Markup.Escape) to AddRow(new Text(...), new Text(...).Ellipsis()) is a nice simplification: Text doesn't parse markup, so it removes a class of "forgot to escape" bugs for free.
• Confirmed LoggingConstants.SecretMask is reused consistently with the existing sensitive-header masking pattern in HttpRequestFormatter/HttpResponseFormatter.
• Ran dotnet build ModularPipelines.sln -c Release (core solution, per this repo's CLAUDE.md guidance) — 0 errors.
• Ran the PipelineInitializerTests project directly — all 10 tests pass, including the new narrow-console-truncation, CR/LF-normalization, and sensitive-name-masking regression tests.
• Compared against the prior review thread: the "narrow terminal collapses Name column" finding called out earlier has been addressed in 6ee031ef63 with a passing regression test (EnvironmentVariables_DoNotWrapLongValues).

No CLAUDE.md violations, no compile/logic issues, no gaps in the masking heuristic that would defeat the PR's stated purpose.

[thomhurst]

Addressed the P2 PWD/OLDPWD masking finding in 9962bc32f1; SERVICE_PWD remains protected. Validation: PipelineInitializerTests 12/12, scoped formatting/analyzers clean, core Release build clean (227 existing warnings). @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9962bc32f1

ℹ️ 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".

[claude]

Code review

No issues found. This is a follow-up review of the changes since the last Claude review — specifically commit 9962bc32f1 (fix(logging): preserve working directory vars).

What changed since the last review:

• IsSensitiveEnvironmentVariableName in PipelineInitializer.cs now excludes exact-match PWD/OLDPWD from the PWD-substring heuristic, while names that merely contain PWD (e.g. SERVICE_PWD) are still masked — this correctly narrows the P2 finding from the prior review round (false-positive masking of the common PWD/OLDPWD shell variables) without weakening the original secret-masking heuristic.
• Two new regression tests (EnvironmentVariables_DoNotMaskStandardWorkingDirectoryNames for PWD/OLDPWD, plus the existing SERVICE_PWD case in EnvironmentVariables_MaskSensitiveNamesWithoutRegisteredSecret) cover both sides of this boundary.

Validation performed:

• Ran PipelineInitializerTests directly via dotnet run --project test/ModularPipelines.UnitTests -- --treenode-filter '/*/*/PipelineInitializerTests/*' — all 12 tests pass (0 failed), confirming the new working-directory-variable tests and prior masking/truncation/newline tests all still hold.
• Re-verified the mask-vs-obfuscate ordering and Text.Ellipsis() usage from the earlier review round remain intact — no regressions introduced by this commit.
• No CLAUDE.md compliance issues in the touched files (no scoped CLAUDE.md exists under src/ModularPipelines/Engine/Executors/ or test/ModularPipelines.UnitTests/Engine/Executors/; root guidance on test filtering/build scope was followed for validation).

Nothing further to flag — the PWD/OLDPWD fix is correct, narrowly scoped, and tested.

[thomhurst]

Addressed authentication-bearing environment names in 5409ad9e42 after rebasing onto current main. PipelineInitializerTests pass 15/15, scoped formatting/analyzers pass, and the core Release build passes with 0 errors (227 existing warnings).

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5409ad9e42

ℹ️ 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".

[claude]

Code review

Reviewed the latest commit, 5409ad9e42 ("fix(logging): mask auth environment values"), which is the only change since the last review round.

What changed: SensitiveEnvironmentVariableNameParts gains "AUTH", and IsSensitiveEnvironmentVariableName gains an SSH_AUTH_SOCK exemption alongside the existing PWD/OLDPWD ones (src/ModularPipelines/Engine/Executors/PipelineInitializer.cs). New tests cover masking DOCKER_AUTH_CONFIG/NPM_CONFIG__AUTH and not masking SSH_AUTH_SOCK.

Correctness: the exemption check runs before the substring Any(...) match via short-circuiting &&, so SSH_AUTH_SOCK is correctly excluded before AUTH would otherwise catch it. Ordinal-ignore-case comparisons are consistent with the rest of the method. Logic is sound and matches the stated intent.

Architectural point (repeating from earlier review rounds, still unaddressed — not a blocker): this commit is a good illustration of why the earlier suggestion still stands. Each round has added one more hardcoded substring to SensitiveEnvironmentVariableNameParts and, increasingly, one more hardcoded exemption to counteract a false positive it introduces (PWD/OLDPWD for the PWD substring, now SSH_AUTH_SOCK for AUTH). That's two growing, coupled, private string[]/exemption lists baked into PipelineInitializer with no consumer-facing way to add a project-specific exemption (e.g. a team using AUTHOR_EMAIL or OAUTH_CALLBACK_URL as a non-secret env var has no escape hatch short of a fork or another PR here).

The codebase already has the right shape for this in two places:

• HttpLoggingOptions.SensitiveHeaderNames (src/ModularPipelines/Options/HttpLoggingOptions.cs) — IReadOnlyList<string> + static Default, IOptions-configurable via PipelineBuilder.Services.
• SecretMaskingOptions (src/ModularPipelines/Options/SecretMaskingOptions.cs) — same DI-configuration pattern for secret-masking behavior generally.

Since this PR keeps discovering new substrings/exemptions through review iteration, that's a reasonable signal the heuristic has stabilized enough to lift into an options record (sensitive name-parts + exemption list) injected into PipelineInitializer, rather than continuing to patch a private array per follow-up PR. Still fine to land as-is and track as a separate follow-up issue.

No bugs or CLAUDE.md violations found in this commit's diff. Tests are well-targeted for the new behavior.

[thomhurst]

Addressed the latest P1 review by masking documented REDIS_URL connection credentials, with regression coverage. @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bf8a51b782

ℹ️ 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".

[claude]

Code review

Reviewed the latest commit, bf8a51b782 ("fix(logging): mask Redis connection URLs"), the only change since the last review round (5409ad9e42).

What changed: adds a new SensitiveEnvironmentVariableNames exact-name list (src/ModularPipelines/Engine/Executors/PipelineInitializer.cs#L35-L38) containing "REDIS_URL", and IsSensitiveEnvironmentVariableName now masks a variable if its name exactly matches an entry in that list or contains one of the existing SensitiveEnvironmentVariableNameParts substrings. A regression test (REDIS_URL case) was added to EnvironmentVariables_MaskSensitiveNamesWithoutRegisteredSecret.

Correctness: the new Contains(name, StringComparer.OrdinalIgnoreCase) check is evaluated after the PWD/OLDPWD/SSH_AUTH_SOCK exemptions via short-circuiting &&, consistent with the existing logic, and doesn't interfere with the substring-based path. Ordinal-ignore-case comparison matches the rest of the method. No bugs found in this commit's diff, and no CLAUDE.md violations.

Architectural point (repeating from earlier review rounds — still unaddressed, still not a blocker, but worth escalating): this commit adds a third hardcoded, private list to this same mechanism — SensitiveEnvironmentVariableNameParts (substring match), the PWD/OLDPWD/SSH_AUTH_SOCK exemption list, and now SensitiveEnvironmentVariableNames (exact-name match) — all baked into PipelineInitializer with no consumer-facing way to extend or override any of them. Each review round so far has added one more entry to patch a newly-discovered gap (AUTH → needed an SSH_AUTH_SOCK exemption; now REDIS_URL needed a whole new exact-match list because it doesn't fit the substring shape). That's a strong signal this heuristic has outgrown a hardcoded array and should live in a proper options object, following the pattern already established in this codebase:

• HttpLoggingOptions.SensitiveHeaderNames (src/ModularPipelines/Options/HttpLoggingOptions.cs) — IReadOnlyList<string> + static Default, IOptions-configurable via PipelineBuilder.Services.
• SecretMaskingOptions (src/ModularPipelines/Options/SecretMaskingOptions.cs) — same DI-configuration pattern for secret-masking behavior generally.

Folding SensitiveEnvironmentVariableNameParts, the exemption list, and the new SensitiveEnvironmentVariableNames into one options record injected into PipelineInitializer (the same way ISecretObfuscator is already injected) would let consumers extend the list for project-specific env vars (e.g. a differently-named Redis/DB connection string var) without needing another PR to this repo each time a new secret-bearing convention is discovered — which is exactly the pattern this PR has been repeating for several rounds now.

Not required for this fix to be correct or safe to ship. Given this is now the third round adding to the same hardcoded heuristic, I'd suggest tracking the options-record refactor as a concrete follow-up issue rather than continuing to patch the array in-place.

[thomhurst]

Expanded masking to standard, .NET, and Azure connection-string environment variable conventions with regression coverage. @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: df0faa546b

ℹ️ 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".

[claude]

Code review

Verification pass on the current head (df0faa54, "fix(logging): mask Azure identity header"), the only commit since the last review round (2d64b6e133, "enforce render-time secret safety").

What changed: IDENTITY_HEADER added to the exact-match SensitiveEnvironmentVariableNames list in PipelineInitializer.cs, plus a matching [Arguments("IDENTITY_HEADER")] case in EnvironmentVariables_MaskSensitiveNamesWithoutRegisteredSecret.

Correctness:

• IDENTITY_HEADER is the real Azure App Service Managed Identity environment variable that holds the secret header value required to call the local identity metadata endpoint — a genuine credential, correctly added as sensitive.
• It's an exact-name entry (not a substring rule), so it doesn't broaden masking for anything else, and it isn't already covered by any SensitiveEnvironmentVariableNameParts/SensitiveEnvironmentVariableDelimitedNameParts substring match (no AUTH/KEY/TOKEN/etc. in the name), so the exact-name addition is necessary for it to be masked at all.
• List placement is alphabetically correct (HTTPS_PROXY → IDENTITY_HEADER → MONGODB_URI), and it isn't already present in NonSensitiveEnvironmentVariableNames (no conflict).

Independently verified (core solution, per this repo's CLAUDE.md guidance):

• dotnet build ModularPipelines.sln -c Release — 0 errors, 0 warnings.
• PipelineInitializerTests (--treenode-filter '/*/*/PipelineInitializerTests/*') — 69/69 passed.
• dotnet format ModularPipelines.sln --verify-no-changes --severity info scoped to the two touched files — no formatting diffs; same pre-existing CA1859 info suggestion on CreateRenderable noted in earlier rounds (unrelated to this commit).

No bugs or CLAUDE.md violations found in this commit's diff.

Recurring architectural point (carried over from many earlier rounds, not touched by this commit, still non-blocking): SensitiveEnvironmentVariableNameParts/SensitiveEnvironmentVariableDelimitedNameParts/SensitiveEnvironmentVariableNames/NonSensitiveEnvironmentVariableNames remain hardcoded private static readonly arrays in PipelineInitializer, even though the constructor already injects IOptions<SecretMaskingOptions>. This commit is another one-line addition to that pattern — lifting the four lists onto SecretMaskingOptions as public IReadOnlyList<string> properties (mirroring HttpLoggingOptions.SensitiveHeaderNames) would let consumers extend/override the heuristic via configuration instead of a new commit each time another provider's env-var convention surfaces. Not repeating the full rationale again since it's been covered at length in prior rounds — still a reasonable follow-up issue, still not blocking for #3548.

Safe to merge as-is.

[claude]

Code review — PR #3561

Reviewed the current diff (src/ModularPipelines/Engine/Executors/PipelineInitializer.cs + PipelineInitializerTests.cs) against the linked incident (#3548) and the full review history on this PR.

Correctness holds up. The core fix is right: rendering the Value column through a custom IRenderable (SafeEnvironmentValueRenderable) that recomputes safety at actual Render/Measure width — rather than a fixed cutoff — means the masking decision is re-evaluated against whatever width Spectre finally grants the column, so a value that looked "safe" during column-width negotiation still gets correctly masked if the final render width shrinks (verified this against EnvironmentVariables_RecheckSafetyWhenRenderWidthShrinks, and it holds for both the Measure/Render two-pass case and the direct production path where no consoleWidth is supplied). Partial obfuscation (a composite value where only part matched a registered secret) is now treated as unsafe-by-default and replaced wholesale rather than trusting the partial redaction — a reasonable conservative call given how hard "did the obfuscator really catch everything" is to verify.

1. Architectural point raised on 2026-07-30 and never addressed across 30+ commits — now much harder to justify deferring

Every one of claude[bot]'s earlier review rounds on this PR flagged the same thing: SensitiveEnvironmentVariableNameParts / SensitiveEnvironmentVariableNames / NonSensitiveEnvironmentVariableNames are private static readonly string[] fields baked directly into PipelineInitializer, with no way for a consuming pipeline to extend, override, or opt out.

This isn't hypothetical risk — it's the demonstrated behavior of this exact PR. The commit history is a 30+ round game of whack-a-mole: REDIS_URL, AzureWebJobsStorage, VSS_NUGET_EXTERNAL_FEED_ENDPOINTS, MONGODB_URI, PIP_INDEX_URL, SLACK_WEBHOOK_URL, AZURE_DEVOPS_EXT_PAT, Azure/OTLP/proxy/Git-config credential names added one at a time as gaps were found, alongside PWD, SSH_AUTH_SOCK, AZURE_STORAGE_AUTH_MODE, YARN_NPM_ALWAYS_AUTH, NPM_CONFIG_AUTH_TYPE, GIT_AUTHOR_*, CI_COMMIT_AUTHOR etc. added to the exemption list as false positives were found. Both lists are now ~15-20 entries each and still necessarily incomplete — there is no way to enumerate every CI provider's and tool's environment-variable naming convention up front, so the next self-hosted runner, package manager, or cloud SDK with its own credential-variable naming scheme will silently leak until someone notices and files another one-line PR.

The codebase already has the right shape for this problem: HttpLoggingOptions.SensitiveHeaderNames (src/ModularPipelines/Options/HttpLoggingOptions.cs:81-96) is exactly this — an IReadOnlyList<string> property with a DefaultSensitiveHeaders static baseline, configurable via IOptions through builder.Services.Configure<HttpLoggingOptions>(...). SecretMaskingOptions (src/ModularPipelines/Options/SecretMaskingOptions.cs) is already injected into PipelineInitializer in this very PR for MaskValue — extending it (or a sibling options type) with SensitiveEnvironmentVariableNames/NonSensitiveEnvironmentVariableNames/name-part collections, defaulted to the current arrays, would let consumers append their own conventions (or their own false-positive exemptions) without forking this file, and would turn the next "we found a leak" report into a config change instead of a library PR.

Why this matters for merge timing specifically: shipping this as private hardcoded arrays means every future gap-fix has to go through this same slow, single-file, one-var-at-a-time PR cycle again. Lifting it to options now — before more call sites or docs come to depend on the current shape — is a small, mechanical change; doing it later is a breaking API change for anyone who's since worked around the lack of extensibility.

2. Minor: MakeSingleLine's CR/LF handling is now dead code

MakeSingleLine (PipelineInitializer.cs) replaces \r/\n with literal \r/\n text, but tracing the call graph: MakeSingleLine is only reached when RequiresUnsafeRendering returns false, and that method already returns true (forcing a full mask instead) whenever value.Any(char.IsControl) is true for an unregistered value, or whenever a registered-secret value was only partially obfuscated (which would include one with an embedded control character in the surrounding text). So by the time MakeSingleLine runs, its input can no longer contain \r/\n — EnvironmentVariables_MaskValuesWithEmbeddedNewlines now asserts the CR/LF case is fully masked, not single-lined, which confirms this. Not a correctness bug (harmless no-op), but worth deleting along with its two .Replace calls to avoid the dead branch confusing future readers about which layer is actually responsible for CR/LF safety.

Verdict

No blocking correctness issues in the current diff — the width-recheck and partial-obfuscation logic are sound and well tested (30/30 per the PR description). The one substantive ask is #1: given how many rounds this PR has already gone through purely to grow two hardcoded lists, please lift them into a configurable options type before merge (or as an immediate same-day follow-up) rather than letting this keep resurfacing as "not a blocker" indefinitely.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 70c385547d

ℹ️ 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]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0106f8a674

ℹ️ 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".

[claude]

Code review — PR #3561

Reviewed the current head (0106f8a) fresh, independently verified against source, and cross-checked against the full review history on this PR (46+ prior review rounds).

Independently verified:

• dotnet build ModularPipelines.sln -c Release (core solution, per this repo's CLAUDE.md) — 0 errors, 0 warnings.
• PipelineInitializerTests (--treenode-filter '/*/*/PipelineInitializerTests/*') — 71/71 passed.

Correctness: The masking/rendering logic in PipelineInitializer.CreateEnvironmentVariablesTable is sound. The SafeEnvironmentValueRenderable.CreateRenderable re-evaluates RequiresUnsafeRendering against the actual maxWidth Spectre passes at Render time (not a cached estimate), so even if Measure sees a stale/larger width during column negotiation, the final Render call always makes the security-relevant decision using ground truth — the worst case from a wider Measure pass is a slightly-too-wide column, never a leaked value. The partial-obfuscation fallback (treat "obfuscator changed it but didn't collapse to exactly the mask" as unsafe) correctly closes the composite-value leak. Sensitive-name detection, the exemption lists, and SecretMaskingOptions.MaskValue honoring all check out against their test coverage.

Every previously-flagged Codex/Claude P1/P2 finding across this PR's long history has a corresponding "Fixed in <commit>" resolution with regression coverage — I didn't find any that were marked fixed but actually regressed in the final squashed diff.

Two points carried over from the last review round, still open, still non-blocking:

1. Architectural: SensitiveEnvironmentVariableNameParts / SensitiveEnvironmentVariableDelimitedNameParts / SensitiveEnvironmentVariableNames / NonSensitiveEnvironmentVariableNames remain hardcoded private static readonly arrays in PipelineInitializer, even though the constructor already injects IOptions<SecretMaskingOptions> for MaskValue. Given this PR's own commit history is a live demonstration of the problem — 30+ rounds each adding one more name/exemption as a new provider convention surfaced — lifting these four lists onto SecretMaskingOptions as public IReadOnlyList<string> properties (mirroring HttpLoggingOptions.SensitiveHeaderNames's IReadOnlyList<string> + static Default shape) would let consumers extend/override the heuristic via services.Configure<SecretMaskingOptions>(...) instead of needing a new PR here every time. The DI seam for this is already sitting unused in this same file. Worth a tracked follow-up issue rather than blocking this fix.
2. Minor/dead code: MakeSingleLine's \r/\n replacement is unreachable with real content by the time it's called — RequiresUnsafeRendering already forces a full mask whenever the unchanged value contains a control character (value.Any(char.IsControl)), and in the changed-value branch it only skips masking when obfuscatedValue is already exactly the mask (which never contains \r/\n). Worth deleting MakeSingleLine and its two .Replace calls next time this file is touched, so a future reader isn't misled about which layer owns CR/LF safety.

Nothing here blocks merge — the fix for #3548 is correct, thoroughly tested (71 tests covering the full matrix of sensitive names, exemptions, width edge cases, and configured-mask behavior), and safe to ship as-is.

[claude]

Code review — PR #3561

Reviewed the current head (8086ac7), independently verified, and diffed against the last full review round (4838268943 at 0106f8a, 2026-08-02T11:27:52Z). Only one commit landed since then.

Delta since last review (8086ac7, "refine environment masking"):

• CLOUDAMQP_URL added to SensitiveEnvironmentVariableNames — correct, it's a credential-bearing AMQP connection URL.
• New IsGitConfigKeyName carve-out for GIT_CONFIG_KEY_<n> (checked before the KEY substring match applies): these hold Git config key names (e.g. http.extraHeader), not values, so exempting them from the KEY substring match is correct — GIT_CONFIG_VALUE_<n> (the actual value) is still forced-sensitive a few lines below, which is the right split.
• Both additions have matching regression tests. Build/tests aren't independently re-run here since this is a narrow, low-risk delta already covered by the existing 30+ test matrix.

No correctness issues in this delta.

Two points carried over from prior rounds — still open, now spanning 9 consecutive review rounds:

1. Architectural (repeatedly flagged since the first review on 2026-07-30): SensitiveEnvironmentVariableNameParts / SensitiveEnvironmentVariableDelimitedNameParts / SensitiveEnvironmentVariableNames / NonSensitiveEnvironmentVariableNames are still hardcoded private static readonly arrays in PipelineInitializer, even though this class already injects IOptions<SecretMaskingOptions>. This PR's own history is the strongest evidence for the fix: 30+ commits, each adding one more provider-specific name or exemption (Redis, Azure Storage, NuGet, CloudAMQP, Git config keys, Yarn/npm auth modes, GitLab/CI author metadata...) as new conventions surfaced one at a time. That pattern doesn't converge — there's no bound on the number of CI systems, cloud providers, and in-house tools with their own env-var naming conventions, so this list will need a new upstream PR forever.

   The codebase already has the right shape for this elsewhere: HttpLoggingOptions.SensitiveHeaderNames is a public, DI-configurable IReadOnlyList<string> with a documented Default, so a consumer whose own naming convention isn't recognized can extend it via services.Configure<HttpLoggingOptions>(...) without waiting on a release. Lifting these four lists onto SecretMaskingOptions the same way would (a) stop new provider-specific names from requiring a new PR/release cycle, and (b) let a user affected by an unrecognized-but-sensitive name in their environment fix it themselves immediately, which is strictly better for a security-hygiene feature than "file an issue and wait."

   This has been raised in essentially every review round on this PR and acknowledged each time as reasonable-but-non-blocking. Given it's now round 9 and still no tracking issue exists for it (checked open/closed issues — none reference this), I'd suggest either implementing it as part of this PR (the DI seam for IOptions<SecretMaskingOptions> is already sitting right there, unused for this purpose) or filing a tracked follow-up issue before merging, so the recommendation doesn't just get silently dropped once this PR closes.

2. Minor/dead code (flagged in the immediately preceding round, still unaddressed): MakeSingleLine's \r/\n replacement is unreachable with real content given the current guard ordering in SafeEnvironmentValueRenderable.CreateRenderable — RequiresUnsafeRendering already forces the mask value whenever an unchanged value contains any control character, and in the changed branch it only takes the "safe" path when obfuscatedValue already equals the mask string exactly (which won't contain \r/\n in practice). Worth deleting MakeSingleLine and its two call sites' .Replace calls next time this file is touched, so a future reader isn't misled into thinking this method is the layer responsible for CR/LF safety.

Nothing here blocks merge — the fix for #3548 (single-line, width-aware, name-heuristic-backed masking of the environment-variables trace table) is correct and thoroughly tested. The two items above are about the long-term maintainability of the mechanism, not its current correctness.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8086ac7d93

ℹ️ 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]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c92ccad9b3

ℹ️ 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".

[claude]

Code review — PR #3561

Reviewed the changes since the last Claude review (70c385547): commits 0106f8a67 (mask VCAP services), 8086ac7d9 (refine environment masking), and c92ccad9b (require canonical Git key names).

Correctness holds up, and it's a nice piece of design in its own right.

• VCAP_SERVICES (Cloud Foundry's JSON blob of bound-service credentials) and CLOUDAMQP_URL are legitimate additions to SensitiveEnvironmentVariableNames — both are real credential-bearing conventions, and both are exact-name entries so they don't broaden masking for anything else.
• The GIT_CONFIG_KEY_<N> / GIT_CONFIG_VALUE_<N> distinction is handled correctly: git config plumbing environment pairs use _KEY_ for the config key name (e.g. user.name, not a secret) and _VALUE_ for the actual value (which can be a secret, e.g. http.extraheader). Exempting only _KEY_ and continuing to mask _VALUE_ (already handled above IsGitConfigKeyName via the GIT_CONFIG_VALUE_ prefix check) is the right split.
• c92ccad9b tightening IsGitConfigKeyName from OrdinalIgnoreCase to Ordinal is a good conservative call: only the canonical uppercase GIT_CONFIG_KEY_<digits> form set by Git itself is exempted from masking. A lowercase or mixed-case variant (git_config_key_0, Git_CONFIG_KEY_0) — which real git config --list --show-origin-style tooling wouldn't produce, but which a hand-set variable might use — now falls through to the generic KEY substring rule and gets masked, i.e. the safe-by-default fallback. I traced this through: "git_config_key_0" and "Git_CONFIG_KEY_0" both contain "KEY" (matched case-insensitively by SensitiveEnvironmentVariableNameParts), so they're still caught as sensitive rather than silently exempted — confirmed by the new test cases in EnvironmentVariables_MaskSensitiveNamesWithoutRegisteredSecret.
• Test coverage for all three commits is precise and covers both sides of each boundary (canonical key exempted in EnvironmentVariables_DoNotMaskKnownNonSecretNames, non-canonical casing still masked). Ran PipelineInitializerTests directly (--treenode-filter '/*/*/PipelineInitializerTests/*'): 75/75 passed.

No bugs or CLAUDE.md violations found in this diff.

Recurring architectural point (carried over from many earlier rounds, still non-blocking): SensitiveEnvironmentVariableNameParts / SensitiveEnvironmentVariableNames / NonSensitiveEnvironmentVariableNames are still hardcoded private static readonly arrays in PipelineInitializer, even though the constructor already injects IOptions<SecretMaskingOptions>. These three commits are three more one-line additions to that same pattern. Not repeating the full rationale again since it's been covered at length in prior rounds (lifting these onto SecretMaskingOptions as configurable IReadOnlyList<string> properties, mirroring HttpLoggingOptions.SensitiveHeaderNames, would let consumers extend the heuristic via config instead of a new commit each time) — still a reasonable follow-up, still not blocking for #3548.

Safe to merge as-is.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e918b642b9

ℹ️ 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".

[claude]

Code review — PR #3561

Reviewed the only commit since the last Claude review round (c92ccad9b, "require canonical Git key names"): e918b642b9 ("mask credentials in Git config keys").

What changed: IsSensitiveEnvironmentVariableName(name) is renamed/expanded to IsSensitiveEnvironmentVariable(name, value). Previously, any canonical GIT_CONFIG_KEY_<N> name was unconditionally treated as non-sensitive. Now it's only exempted if its value doesn't embed URI user-info credentials (scheme://user:pass@host) — covering the real git config url.<base-with-credentials>.insteadOf pattern, where the key itself (not the value) carries a credential-bearing URL.

Correctness:

• ContainsUriUserInfo correctly scopes its @ search to the authority component only (between :// and the first /, ?, or #), so a legitimate @ appearing later in the path/query (e.g. .../user@name/repo) is correctly not treated as embedded credentials.
• Traced both new test cases (url.https://oauth2:secret@gitlab.example/.insteadOf, url.https://token@gitlab.example/.insteadOf) through the logic — both are masked as expected.
• Confirmed no regression on the existing GIT_CONFIG_KEY_0 case in EnvironmentVariables_DoNotMaskKnownNonSecretNames, whose value (/home/runner/work, no ://) still correctly renders unmasked.
• No other callers of the renamed method were left dangling.

Independently verified locally (per this repo's CLAUDE.md guidance — core solution only, via the agent build guard):

• dotnet build ModularPipelines.sln -c Release — 0 errors, 0 warnings.
• PipelineInitializerTests (--treenode-filter '/*/*/PipelineInitializerTests/*') — 77/77 passed (up from 75, confirming the two new tests run and pass).
• dotnet format ModularPipelines.sln --verify-no-changes --severity info on the two touched files — clean, aside from the same pre-existing CA1859 info suggestion on CreateRenderable noted in earlier rounds (unrelated to this commit).

No bugs or CLAUDE.md violations found in this commit's diff.

Recurring architectural point (unaddressed, still non-blocking): as flagged across many prior rounds on this PR, SensitiveEnvironmentVariableNameParts / SensitiveEnvironmentVariableNames / NonSensitiveEnvironmentVariableNames remain hardcoded private static readonly arrays in PipelineInitializer, even though the constructor already injects IOptions<SecretMaskingOptions>. This commit adds a fourth kind of hardcoded heuristic (URI user-info sniffing for Git config keys) to the same file. Not repeating the full rationale again — it's been covered at length — but the continued growth (now well past 30 commits of one-off additions) is itself the evidence for eventually lifting these into configurable SecretMaskingOptions properties, mirroring HttpLoggingOptions.SensitiveHeaderNames, so consumers get an extension point instead of needing a new PR per provider convention.

Safe to merge as-is.

[claude]

Code review — PR #3561

Reviewed the two commits landed since the last Claude review round (e918b642b9): 7fec3e1f6 (merge origin/main into the branch) and c19e47f74 (test(analyzers): preserve expected formatting).

Delta since last review:

• 7fec3e1f6 is a routine merge from main — no conflicting or hand-written changes in the merge commit itself.
• c19e47f74 touches only ModularPipelinesAnalyzersUnitTests.cs, updating the Nested_Helper_Code_Fix_Adds_Attribute_To_Enclosing_Module fixture's expected source/fixed-source strings: a blank line after namespace Example; and expression-bodied ExecuteAsync methods reformatted onto their own line (=> on a new line). This is an unrelated analyzer-test fixture realignment picked up by the merge from main, not a change to the environment-masking feature this PR is about.

Independently verified:

• dotnet build ModularPipelines.Analyzers.sln -c Release (the solution containing the touched file) — 0 errors, 0 warnings.
• Ran the specific affected test, ModularPipelinesAnalyzersUnitTests.Nested_Helper_Code_Fix_Adds_Attribute_To_Enclosing_Module — 1/1 passed, confirming the fixture now matches the code fix's actual output.
• dotnet format ModularPipelines.Analyzers.sln --verify-no-changes --severity info scoped to the touched file — clean.

No bugs or CLAUDE.md violations in this delta — it's a mechanical, correct fixture update.

Carried over from prior rounds, still open, still non-blocking (untouched by this delta):

1. Architectural: SensitiveEnvironmentVariableNameParts / SensitiveEnvironmentVariableNames / NonSensitiveEnvironmentVariableNames remain hardcoded private static readonly arrays in PipelineInitializer.cs, despite the constructor already injecting IOptions<SecretMaskingOptions>. This has been raised across essentially every review round on this PR; still worth lifting onto SecretMaskingOptions as configurable IReadOnlyList<string> properties (mirroring HttpLoggingOptions.SensitiveHeaderNames) or tracking as a follow-up issue.
2. Minor/dead code: MakeSingleLine's \r/\n replacement in the same file remains unreachable given the current guard ordering in SafeEnvironmentValueRenderable.CreateRenderable. Still safe to delete next time this file is touched.

Nothing here blocks merge — the fix for #3548 remains correct and thoroughly tested; this round's changes are outside the feature itself.

[thomhurst]

Pushed d7be562193 for the AWS container token-file P2. Validation: 79/79 PipelineInitializerTests; core Release build clean; changed-file whitespace and warning analyzers clean. Thread stays open for the next bot cycle.

[claude]

Code review — PR #3561

Reviewed the only commit since the last Claude review round (c19e47f74, which was "safe to merge as-is"): d7be562193 ("fix: preserve AWS token file path"), addressing the Codex P2 finding that AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE was being masked alongside the actual bearer-token variable.

What changed:

• PipelineInitializer.cs: adds AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE to NonSensitiveEnvironmentVariableNames, alphabetically placed between APPVEYOR_REPO_COMMIT_AUTHOR_EMAIL and AWS_SHARED_CREDENTIALS_FILE, mirroring the existing AWS_WEB_IDENTITY_TOKEN_FILE/AWS_SHARED_CREDENTIALS_FILE/AZURE_FEDERATED_TOKEN_FILE file-path exemptions.
• PipelineInitializerTests.cs: adds AWS_CONTAINER_AUTHORIZATION_TOKEN (bare token, expected masked) to the masking-without-secret test, and AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE (expected preserved) to the non-sensitive-names test.

Correctness check: IsSensitiveEnvironmentVariable checks NonSensitiveEnvironmentVariableNames.Contains(name, StringComparer.Ordinal) first, and that's an exact-string match — so the new allowlist entry can't accidentally exempt AWS_CONTAINER_AUTHORIZATION_TOKEN (no _FILE suffix), which still hits the "TOKEN"/"AUTH" substring rules and stays masked. The two new test cases directly pin both sides of that boundary.

Independently verified:

• dotnet build ModularPipelines.sln -c Release — 0 errors, 0 warnings.
• PipelineInitializerTests (TUnit, --treenode-filter "/*/*/PipelineInitializerTests/*") — 79/79 passed.
• dotnet format ModularPipelines.sln --verify-no-changes --severity info scoped to the touched source file — clean.

No bugs or CLAUDE.md violations in this delta — it's a minimal, correctly-scoped, well-tested fix that follows the exact pattern already established for the other credential-file exemptions in this list.

Carried over from prior rounds, still open, still non-blocking (untouched by this delta):

1. Architectural: SensitiveEnvironmentVariableNameParts / SensitiveEnvironmentVariableNames / NonSensitiveEnvironmentVariableNames remain hardcoded private static readonly arrays in PipelineInitializer.cs, despite the constructor already injecting IOptions<SecretMaskingOptions>. Worth lifting onto SecretMaskingOptions as configurable IReadOnlyList<string> properties (mirroring HttpLoggingOptions.SensitiveHeaderNames) or tracking as a follow-up issue, given the list has now grown across 30+ incremental commits.
2. Minor/dead code: MakeSingleLine's \r/\n replacement in the same file remains unreachable given the current guard ordering in SafeEnvironmentValueRenderable.CreateRenderable. Still safe to delete next time this file is touched.

Safe to merge as-is.