Web pipeline: step modes + audit artifacts on fail

[PrzemyslawKlys]

Adds step-level mode gating via mode/modes/skipModes (effective mode defaults to default) and aligns audit summary/SARIF *OnFail behavior with doctor. Also updates docs and schema.

[claude]

Pull Request Review: Web pipeline step modes + audit artifacts on fail

Summary

This PR adds step-level mode gating functionality to the web pipeline, allowing users to conditionally execute steps based on the pipeline mode (e.g., dev, ci, default). It also aligns the audit summary/SARIF *OnFail behavior with the doctor task.

Code Quality & Best Practices

Strengths:

• Clean separation of concerns with the new GetSkipReason() and ShouldExecuteStepMode() helper methods
• Consistent naming conventions following C# standards
• Good use of string comparison with StringComparison.OrdinalIgnoreCase for case-insensitive mode matching
• Proper null handling throughout the mode checking logic
• Documentation is comprehensive and includes clear examples

Minor Issues:

1. Potential inefficiency in mode merging (PowerForge.Web.Cli/WebPipelineRunner.cs:1204)

   if (!string.IsNullOrWhiteSpace(mode))
       modes = modes is null ? new[] { mode } : modes.Concat(new[] { mode }).ToArray();

   Using Concat().ToArray() creates unnecessary intermediate allocations. Consider:

   if (!string.IsNullOrWhiteSpace(mode))
   {
       if (modes is null)
           modes = new[] { mode };
       else
       {
           var temp = new string[modes.Length + 1];
           Array.Copy(modes, temp, modes.Length);
           temp[modes.Length] = mode;
           modes = temp;
       }
   }

   However, given this is typically a small array, the current approach is acceptable for readability.

2. Schema redundancy (schemas/powerforge.web.pipelinespec.schema.json)
   The schema includes both camelCase and kebab-case variants (skipModes, skip-modes, onlyModes, only-modes). While this provides flexibility, it could lead to inconsistent usage across configs. Consider documenting the preferred style in CLAUDE.md or the schema description.

Potential Bugs or Issues

1. Null element handling (PowerForge.Web.Cli/WebPipelineRunner.cs:1208)

   skipModes.Any(m => string.Equals(m?.Trim(), effectiveMode, StringComparison.OrdinalIgnoreCase))

   The code uses m?.Trim() which can return null. While string.Equals handles null, consider filtering out null/whitespace entries earlier:

   var skipModes = GetArrayOfStrings(step, "skipModes") ?? GetArrayOfStrings(step, "skip-modes");
   if (skipModes is not null)
   {
       var cleanedSkipModes = skipModes.Where(m => !string.IsNullOrWhiteSpace(m)).Select(m => m.Trim());
       if (cleanedSkipModes.Any(m => string.Equals(m, effectiveMode, StringComparison.OrdinalIgnoreCase)))
           return false;
   }

2. Audit OnFail logic (PowerForge.Web.Cli/WebPipelineRunner.cs:820-821)

   SummaryOnFailOnly = summaryOnFail && !summary,
   SarifOnFailOnly = sarifOnFail && !sarif,

   This logic is correct but subtle. The intent is: "only generate on fail if the user specified *OnFail but NOT the unconditional flag." Consider adding a comment explaining this logic for future maintainers.

Performance Considerations

• The mode checking logic is executed once per step, which is minimal overhead
• The Concat().ToArray() on line 1204 could be optimized (mentioned above), but the impact is negligible for typical pipelines with few modes
• Overall performance impact is negligible

Security Concerns

No security issues identified. The code:

• Properly validates input using case-insensitive string comparison
• Doesn't execute user-provided code
• Handles null/empty strings appropriately
• Uses safe string operations

Test Coverage

Critical Gap: No unit tests were added for the new mode gating functionality.

Recommended tests:

1. Test that steps execute when mode matches mode field
2. Test that steps execute when mode matches one entry in modes array
3. Test that steps are skipped when mode is in skipModes
4. Test that skipModes takes precedence over modes
5. Test that steps without mode constraints run in all modes
6. Test that the default mode is "default" when --mode is not specified
7. Test case-insensitive mode matching
8. Test both camelCase and kebab-case field variants
9. Test the new audit *OnFail behavior

Example test structure:

[Fact]
public void ShouldExecuteStepMode_WhenModeMatchesSingleMode_ReturnsTrue()
{
    // Arrange: step with mode="dev", effectiveMode="dev"
    // Act: call ShouldExecuteStepMode
    // Assert: returns true
}

Documentation

The documentation in Docs/PowerForge.Web.Pipeline.md is excellent:

• Clear explanation of the feature
• Concrete example demonstrating use case
• Properly describes precedence and default behavior

Suggestion: Add a note about combining mode with modes - the current implementation merges them, which might be unexpected.

Overall Assessment

This is a well-implemented feature that adds useful functionality with minimal complexity. The main concern is the lack of unit tests for the new mode gating logic. Given that this affects pipeline execution flow, comprehensive tests are essential.

Recommendations

1. High Priority: Add unit tests for ShouldExecuteStepMode() covering all scenarios
2. Medium Priority: Add explanatory comment for the *OnFail logic (lines 820-821)
3. Low Priority: Document preferred casing style for mode field names
4. Optional: Consider optimizing the Concat().ToArray() call if this code path proves hot

The code is production-ready pending test coverage. Once tests are added, this would be a solid improvement to the pipeline system.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bbe3f4fa03

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

Treat mode-skipped dependencies as not executed

When a step is filtered out by mode/modes/skipModes, GetSkipReason now causes it to be marked as a successful skip, but dependency enforcement later only checks dependencyResult.Success; this means downstream steps with dependsOn can still run even though their prerequisite never executed. In pipelines where a producer step is mode-gated (for example build in ci) and a consumer step is not, running in another mode can execute the consumer against stale or missing artifacts, which is a correctness regression introduced by the new mode filtering path.

Useful? React with 👍 / 👎.

[Copilot]

Pull request overview

Adds pipeline step-level mode gating and adjusts audit artifact writing so summary/SARIF can be emitted only on failure (consistent with doctor), with corresponding schema + documentation updates.

Changes:

• Add per-step mode filtering via mode / modes / onlyModes (+ kebab-case aliases) and skipModes (+ alias), with default as the effective mode when --mode is omitted.
• Update pipeline audit/doctor execution to honor SummaryOnFailOnly / SarifOnFailOnly behavior when *OnFail is enabled but summary/sarif isn’t explicitly enabled.
• Document step modes and extend the pipeline spec JSON schema with the new mode-related fields.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
schemas/powerforge.web.pipelinespec.schema.json	Adds mode-related fields to step definitions (schema support for step mode gating).
PowerForge.Web.Cli/WebPipelineRunner.cs	Implements effective mode defaulting + step skip reasoning by mode; aligns audit artifact on-fail handling.
Docs/PowerForge.Web.Pipeline.md	Documents --mode semantics and the new per-step mode gating fields with an example.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

ShouldExecuteStepMode treats modes/onlyModes/only-modes as mutually exclusive by using ?? (first non-null wins). If a pipeline step accidentally specifies more than one of these arrays, the later ones are silently ignored, which can lead to unexpected step execution/skipping. Consider either merging all provided arrays into a single allow-list, or explicitly detecting the conflict and failing with a clear error message (same applies to skipModes vs skip-modes).

Suggested change

	var modes = GetArrayOfStrings(step, "modes") ??
	GetArrayOfStrings(step, "onlyModes") ?? GetArrayOfStrings(step, "only-modes");
	if (!string.IsNullOrWhiteSpace(mode))
	modes = modes is null ? new[] { mode } : modes.Concat(new[] { mode }).ToArray();
	var skipModes = GetArrayOfStrings(step, "skipModes") ?? GetArrayOfStrings(step, "skip-modes");
	// Combine all possible "allowed modes" sources into a single array.
	List<string>? modesList = null;
	var modesFromModes = GetArrayOfStrings(step, "modes");
	if (modesFromModes is not null && modesFromModes.Length > 0)
	{
	modesList ??= new List<string>();
	modesList.AddRange(modesFromModes);
	}
	var modesFromOnlyModes = GetArrayOfStrings(step, "onlyModes");
	if (modesFromOnlyModes is not null && modesFromOnlyModes.Length > 0)
	{
	modesList ??= new List<string>();
	modesList.AddRange(modesFromOnlyModes);
	}
	var modesFromOnlyModesKebab = GetArrayOfStrings(step, "only-modes");
	if (modesFromOnlyModesKebab is not null && modesFromOnlyModesKebab.Length > 0)
	{
	modesList ??= new List<string>();
	modesList.AddRange(modesFromOnlyModesKebab);
	}
	if (!string.IsNullOrWhiteSpace(mode))
	{
	modesList ??= new List<string>();
	modesList.Add(mode);
	}
	var modes = modesList is not null && modesList.Count > 0 ? modesList.ToArray() : null;
	// Combine all possible "skip modes" sources into a single array.
	List<string>? skipModesList = null;
	var skipModesCamel = GetArrayOfStrings(step, "skipModes");
	if (skipModesCamel is not null && skipModesCamel.Length > 0)
	{
	skipModesList ??= new List<string>();
	skipModesList.AddRange(skipModesCamel);
	}
	var skipModesKebab = GetArrayOfStrings(step, "skip-modes");
	if (skipModesKebab is not null && skipModesKebab.Length > 0)
	{
	skipModesList ??= new List<string>();
	skipModesList.AddRange(skipModesKebab);
	}
	var skipModes = skipModesList is not null && skipModesList.Count > 0 ? skipModesList.ToArray() : null;

[Copilot]

The pipeline runner parses summaryOnFail/summary-on-fail and sarifOnFail/sarif-on-fail for audit/doctor steps, but those fields are not present in the JSON schema. This makes valid configs fail schema validation and also hides the new/updated on-fail artifact behavior from schema-driven tooling. Please add these properties (and their kebab-case aliases) to the relevant step definitions in the schema.