web: pipeline visibility on failure#72
Conversation
…f on fail, optimize report)
Code Review: Pipeline Visibility on FailureSummaryThis PR improves pipeline failure diagnostics by ensuring critical output files (profiles, audit summaries, and optimization reports) are written even when pipelines fail. This is a valuable improvement for debugging CI/CD failures. ✅ Strengths
🔍 Code Quality Observations1. Profile Writing Logic (WebPipelineRunner.cs:973, 1004)Issue: Inconsistent conditions for writing profile on success vs failure // On failure (line 973):
if (\!string.IsNullOrWhiteSpace(profilePath) && (profileEnabled || profileWriteOnFail))
// On success (line 1004):
if (\!string.IsNullOrWhiteSpace(profilePath) && profileEnabled)Analysis: The failure path writes when
Recommendation: Add a code comment explaining this asymmetric behavior, or consider simplifying to always write if profilePath exists: // Write profile on failure when profileOnFail is true (default)
// even if profile tracking is disabled, to aid debugging
if (\!string.IsNullOrWhiteSpace(profilePath) && (profileEnabled || profileWriteOnFail))2. Audit Summary/SARIF Path Resolution (WebPipelineRunner.cs:617-622, 806-811)Potential Issue: Default paths are set AFTER explicit path resolution var resolvedSummaryPath = ResolveSummaryPathForPipeline(summary, summaryPath);
if (string.IsNullOrWhiteSpace(resolvedSummaryPath) && summaryOnFail)
resolvedSummaryPath = ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "audit-summary.json"));Question: What does Recommendation: Verify this logic works as intended. Consider simplifying: var resolvedSummaryPath = \!string.IsNullOrWhiteSpace(summaryPath)
? ResolveSummaryPathForPipeline(summary, summaryPath)
: (summaryOnFail ? ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "audit-summary.json")) : null);3. Optimize Report Auto-Generation (WebPipelineRunner.cs:480-484)Good Practice: Automatically creates a report when any optimization is enabled if (string.IsNullOrWhiteSpace(reportPath) &&
(minifyHtml || minifyCss || minifyJs || optimizeImages || hashAssets || cacheHeaders))
{
reportPath = ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "optimize-report.json"));
}Note: This always writes the report (not just on failure), which differs from the audit behavior. This is reasonable since optimize steps typically succeed, and the report shows savings/metrics. 4. Audit Options - OnFailOnly Flags (WebSiteAuditor.cs:742-743, 781-782)Correctness Check: The condition logic is correct if (\!string.IsNullOrWhiteSpace(options.SummaryPath) &&
(\!options.SummaryOnFailOnly || \!result.Success))Logic Analysis:
🐛 Potential IssuesIssue 1: Edge Case in Audit Step Parameter PassingIn WebPipelineRunner.cs lines 831-838, the new parameters are set: SummaryPath = resolvedSummaryPath,
SarifPath = resolvedSarifPath,
SummaryMaxIssues = summaryMax,
SummaryOnFailOnly = summaryOnFail && \!summary,
SarifOnFailOnly = sarifOnFail && \!sarif,Concern: The logic
Is this intended? It seems like the intent is: "if user explicitly enabled summary, always write it; otherwise use the fail-only flag." This is reasonable but could be documented. Issue 2: Duplicate CodeThe audit step logic appears twice in the file (around lines 614-650 and 772-850) with nearly identical parameter parsing. This creates maintenance burden. Recommendation: Extract common parameter parsing into a helper method to ensure consistency and reduce duplication. ⚡ Performance Considerations
✅ No performance concerns 🔒 Security Considerations
✅ No major security concerns (assuming 🧪 Test CoverageMissing: No visible test changes in this PR Recommendations:
📋 DocumentationMissing: No documentation updates visible Recommendations:
Final VerdictOverall: ✅ Approve with minor suggestions This PR delivers valuable functionality for debugging pipeline failures. The code is generally well-structured and backward compatible. Main suggestions:
The changes are production-ready as-is, but addressing the suggestions would improve maintainability. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd6fda5581
ℹ️ 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".
| var resolvedSummaryPath = ResolveSummaryPathForPipeline(summary, summaryPath); | ||
| if (string.IsNullOrWhiteSpace(resolvedSummaryPath) && summaryOnFail) | ||
| resolvedSummaryPath = ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "audit-summary.json")); |
There was a problem hiding this comment.
This branch computes resolvedSummaryPath with the new summaryOnFail fallback, but the audit call still uses ResolveSummaryPathForPipeline(summary, summaryPath) instead of the resolved value. With default summaryOnFail: true and summary: false, failed audits will not write the expected .powerforge/audit-summary.json, so the new failure-only summary artifact is silently skipped.
Useful? React with 👍 / 👎.
| var resolvedSarifPath = ResolveSarifPathForPipeline(sarif, sarifPath); | ||
| if (string.IsNullOrWhiteSpace(resolvedSarifPath) && sarifOnFail) | ||
| resolvedSarifPath = ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "audit.sarif")); |
There was a problem hiding this comment.
These lines assign a SARIF output path whenever sarifOnFail is true (the new default), but this audit branch does not set WebAuditOptions.SarifOnFailOnly. In WebSiteAuditor.Audit, SARIF is written whenever SarifPath is present unless SarifOnFailOnly is true, so successful audits will now produce SARIF files even though the pipeline option is intended to be failure-only.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR aims to improve post-failure visibility in the web pipeline by emitting profiling/audit/optimize artifacts automatically so failures are actionable without reruns.
Changes:
- Add
SummaryOnFailOnly/SarifOnFailOnlyswitches toWebAuditOptionsand gate summary/SARIF writing on audit failure. - Extend pipeline config handling with
profileOnFail,summaryOnFail, andsarifOnFaildefaults. - Add a default optimize report path when any optimize action is enabled.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| PowerForge.Web/Services/WebSiteAuditor.cs | Adds “write outputs only on failure” switches for audit summary/SARIF. |
| PowerForge.Web.Cli/WebPipelineRunner.cs | Adds pipeline defaults for profile-on-fail, audit artifacts on fail, and optimize report output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| : null; | ||
| var profileEnabled = (GetBool(root, "profile") ?? false) || forceProfile; | ||
| var profileWriteOnFail = GetBool(root, "profileOnFail") ?? GetBool(root, "profile-on-fail") ?? true; | ||
| var profilePath = ResolvePathWithinRoot(baseDir, GetString(root, "profilePath") ?? GetString(root, "profile-path"), Path.Combine(".powerforge", "pipeline-profile.json")); |
There was a problem hiding this comment.
profilePath is resolved/validated unconditionally. If a config provides an out-of-root profilePath but both profile and profileOnFail are false, the pipeline will now throw during startup even though profiling is disabled. Consider only calling ResolvePathWithinRoot when (profileEnabled || profileWriteOnFail) (or deferring resolution until you know you will write a profile).
| var profilePath = ResolvePathWithinRoot(baseDir, GetString(root, "profilePath") ?? GetString(root, "profile-path"), Path.Combine(".powerforge", "pipeline-profile.json")); | |
| var profilePathSetting = GetString(root, "profilePath") ?? GetString(root, "profile-path"); | |
| var profilePath = (profileEnabled || profileWriteOnFail) | |
| ? ResolvePathWithinRoot(baseDir, profilePathSetting, Path.Combine(".powerforge", "pipeline-profile.json")) | |
| : profilePathSetting ?? Path.Combine(".powerforge", "pipeline-profile.json"); |
| if (string.IsNullOrWhiteSpace(reportPath) && | ||
| (minifyHtml || minifyCss || minifyJs || optimizeImages || hashAssets || cacheHeaders)) | ||
| { | ||
| reportPath = ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "optimize-report.json")); |
There was a problem hiding this comment.
The default reportPath is resolved to an absolute path under the pipeline root, but WebAssetOptimizerOptions.ReportPath is treated as a path relative to SiteRoot (see TryResolveUnderRoot). Passing an absolute pipeline path will fail resolution and the report won’t be written (only a trace warning). Either pass a relative path under siteRoot (e.g. .powerforge/optimize-report.json) or extend the optimizer to support an explicit absolute report path.
| reportPath = ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "optimize-report.json")); | |
| // Use a path relative to the site root so WebAssetOptimizerOptions.ReportPath is resolved correctly. | |
| reportPath = Path.Combine(".powerforge", "optimize-report.json"); |
| var resolvedSarifPath = ResolveSarifPathForPipeline(sarif, sarifPath); | ||
| var resolvedSummaryPath = ResolveSummaryPathForPipeline(summary, summaryPath); | ||
| if (string.IsNullOrWhiteSpace(resolvedSummaryPath) && summaryOnFail) | ||
| resolvedSummaryPath = ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "audit-summary.json")); |
There was a problem hiding this comment.
resolvedSummaryPath is computed (and may be defaulted to .powerforge/audit-summary.json when summaryOnFail is true) but it is not used when building WebAuditOptions later in this case block, so the new “summary on fail” behavior won’t take effect. Use resolvedSummaryPath for SummaryPath and wire SummaryOnFailOnly so the file is only written on failures.
| resolvedSummaryPath = ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "audit-summary.json")); | |
| resolvedSummaryPath = ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "audit-summary.json")); | |
| if (!string.IsNullOrWhiteSpace(resolvedSummaryPath)) | |
| summaryPath = resolvedSummaryPath; |
| var resolvedSarifPath = ResolveSarifPathForPipeline(sarif, sarifPath); | ||
| if (string.IsNullOrWhiteSpace(resolvedSarifPath) && sarifOnFail) | ||
| resolvedSarifPath = ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "audit.sarif")); |
There was a problem hiding this comment.
resolvedSarifPath is set to an absolute path under the pipeline root (.powerforge/audit.sarif). WebSiteAuditor requires SarifPath to resolve under SiteRoot and will throw when writing. Prefer a path relative to siteRoot (e.g. .powerforge/audit.sarif) or adjust the auditor’s path validation if pipeline-root outputs are intended, and set SarifOnFailOnly so sarifOnFail=true doesn’t write on success.
| resolvedSummaryPath = ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "audit-summary.json")); | ||
|
|
||
| var resolvedSarifPath = ResolveSarifPathForPipeline(sarif, sarifPath); | ||
| if (string.IsNullOrWhiteSpace(resolvedSarifPath) && sarifOnFail) | ||
| resolvedSarifPath = ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "audit.sarif")); |
There was a problem hiding this comment.
In doctor, the default resolvedSummaryPath is resolved to an absolute path under the pipeline root via ResolvePathWithinRoot. Since WebSiteAuditor enforces output paths under SiteRoot (effectiveSiteRoot here), this will throw when trying to write the summary. Use a path relative to effectiveSiteRoot (e.g. .powerforge/audit-summary.json) or relax the auditor’s validation if pipeline-root outputs are required.
| resolvedSummaryPath = ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "audit-summary.json")); | |
| var resolvedSarifPath = ResolveSarifPathForPipeline(sarif, sarifPath); | |
| if (string.IsNullOrWhiteSpace(resolvedSarifPath) && sarifOnFail) | |
| resolvedSarifPath = ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "audit.sarif")); | |
| resolvedSummaryPath = Path.Combine(".powerforge", "audit-summary.json"); | |
| var resolvedSarifPath = ResolveSarifPathForPipeline(sarif, sarifPath); | |
| if (string.IsNullOrWhiteSpace(resolvedSarifPath) && sarifOnFail) | |
| resolvedSarifPath = Path.Combine(".powerforge", "audit.sarif"); |
|
|
||
| var resolvedSarifPath = ResolveSarifPathForPipeline(sarif, sarifPath); | ||
| if (string.IsNullOrWhiteSpace(resolvedSarifPath) && sarifOnFail) | ||
| resolvedSarifPath = ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "audit.sarif")); |
There was a problem hiding this comment.
Similarly, resolvedSarifPath is resolved under the pipeline root, but WebSiteAuditor requires SARIF output paths to be under effectiveSiteRoot and will throw when writing. Use a path relative to effectiveSiteRoot (e.g. .powerforge/audit.sarif) or adjust path validation to support pipeline-root outputs.
| resolvedSarifPath = ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "audit.sarif")); | |
| resolvedSarifPath = ResolvePathWithinRoot(effectiveSiteRoot!, null, Path.Combine(".powerforge", "audit.sarif")); |
| if (!string.IsNullOrWhiteSpace(profilePath) && profileEnabled) | ||
| { | ||
| WritePipelineProfile(profilePath, result, logger); | ||
| result.ProfilePath = profilePath; | ||
| } |
There was a problem hiding this comment.
On the non-exception completion path, the profile is only written when profileEnabled is true. This misses failures where no exception is thrown but result.Success ends up false (e.g. unknown task). To implement “write profile on failure”, include the failure condition here as well (e.g. profileEnabled || (profileWriteOnFail && !result.Success)).
Pull Request Review: Pipeline Visibility on FailureOverviewThis PR improves failure debugging in PowerForge pipelines by automatically writing diagnostic artifacts (profiles, audit summaries, SARIF reports, optimize reports) when pipeline steps fail, without requiring reruns or verbose console output. ✅ Strengths1. Smart Default BehaviorThe asymmetric profile-writing logic is well-designed:
This is clearly documented in the comment at 2. Consistent Naming ConventionsSupports both camelCase and kebab-case for all new options:
This maintains consistency with the existing codebase pattern. 3. Well-Structured Conditional LogicThe SummaryOnFailOnly = summaryOnFail && !summary,
SarifOnFailOnly = sarifOnFail && !sarif,This correctly ensures that:
4. Sensible Auto-PathsWhen optimize actions are enabled without an explicit report path, a default 🔍 Issues & Recommendations1. Critical: Logic Bug in Profile Writing (Success Path)Location: Issue: On the success path, the profile is only written when if (!string.IsNullOrWhiteSpace(profilePath) && profileEnabled)However, var profilePath = ResolvePathWithinRoot(baseDir, GetString(root, "profilePath") ?? GetString(root, "profile-path"), Path.Combine(".powerforge", "pipeline-profile.json"));Before this PR: After this PR: Problem: This means profiles will never be written on success, even when Recommendation:
Suggested fix: // Line 1008 - Make consistent with failure path
if (!string.IsNullOrWhiteSpace(profilePath) && (profileEnabled || profileWriteOnFail))Wait, actually looking more carefully - the success path should ONLY write when Let me reconsider: The failure path writes when However, there's still a concern: if Actually, I need to trace through the logic more carefully: Scenario 1:
Scenario 2:
Okay, the logic is actually correct! My initial concern was unfounded. The success path should only check 2. Minor: Inconsistent IndentationLocation: Multiple locations in the diff Several sections show indentation changes that appear to be whitespace-only. For example, lines 443-539 in the optimize section and lines 542-648 in the audit section show re-indentation. Impact: While this doesn't affect functionality, it makes the diff harder to review and could cause merge conflicts. Recommendation: If this is just an IDE auto-format issue, consider:
3. Potential: Race Condition in File WritingLocation: Observation: Multiple file writes happen during exception handling without explicit error handling around I/O operations. Scenario: If profile/summary/SARIF writing fails (disk full, permissions, network path unavailable), the exception would:
Recommendation: Consider wrapping file writes in try-catch blocks and logging warnings if they fail, rather than allowing I/O exceptions to propagate: try
{
WritePipelineProfile(profilePath, result, logger);
result.ProfilePath = profilePath;
}
catch (Exception ex)
{
logger?.Warning($"Failed to write profile to {profilePath}: {ex.Message}");
}This ensures the original failure reason is preserved even if artifact writing fails. 4. Documentation GapThe PR description mentions the new flags but doesn't specify:
Recommendation: Add these details to the PR description or inline code comments for future maintainers. 🔒 Security ConsiderationsPath Traversal Protection: The code uses File Overwrites: Profile/summary/SARIF files are written without checking if they exist. This is likely intentional (latest run should overwrite), but confirm this is desired behavior when multiple pipelines run concurrently. ⚡ Performance ConsiderationsMinimal Impact:
🧪 Test CoverageObservation: This PR doesn't include test updates. Recommendation: Consider adding tests for:
Existing test file 📝 SummaryOverall: This is a well-thought-out enhancement that addresses a real pain point in CI debugging. The implementation is clean and follows existing patterns. Critical Issues: Recommendations:
Approval Status: Once the Review completed by Claude Code |
Changes:\n- Always write pipeline profile to .powerforge/pipeline-profile.json on failure (configurable via profileOnFail, default true).\n- Audit: default �udit-summary.json + �udit.sarif written on failure only (new summaryOnFail/sarifOnFail, default true).\n- Optimize: default report written to .powerforge/optimize-report.json when any optimize action is enabled.\n\nWhy:\n- Makes �udit/optimize/pipeline failures actionable without reruns or huge console spam.