Fix default audit artifact paths to be under site root

[PrzemyslawKlys]

Fixes pipeline audit fallback artifact paths (summary/sarif) to be relative to the built site root, so audits don't fail with 'Path must resolve under site root' when defaults are used.

[claude]

Pull Request Review

Summary

This PR fixes the default audit artifact paths (summary/sarif) to be relative to the built site root rather than using ResolvePathWithinRoot, which was causing 'Path must resolve under site root' errors when defaults were used.

Code Quality: ✅ Good

Positive aspects:

• The fix correctly addresses the root cause by changing from absolute paths to relative paths
• The solution is minimal and focused - only changes what's necessary
• Consistent fix applied to both locations where the issue occurs (lines 736/740 and 965/969)

Observations:

1. Path Format Consistency: The change introduces a minor inconsistency:

   • Line 736: .powerforge/audit-summary.json (forward slash)
   • Line 740: .powerforge/audit.sarif.json (forward slash)

   Both use forward slashes which works cross-platform in .NET's Path.Combine, but the original code used Path.Combine(".powerforge", "audit-summary.json") which would use the platform-appropriate separator. The new approach is acceptable and simpler.

2. File Extension Discrepancy: Line 740 changes the SARIF filename from audit.sarif to audit.sarif.json. This appears to be a correction since:

   • The function ResolveSarifPathForPipeline (line 2343) returns audit.sarif.json as the default
   • Tests use audit.sarif.json (line 1903 in WebSiteAuditOptimizeBuildTests.cs)
   • This brings consistency with the codebase

Correctness: ✅ Verified

The fix is correct because:

1. Root Cause Analysis: The original code called ResolvePathWithinRoot(baseDir, null, Path.Combine(".powerforge", "audit-summary.json")) which:

   • Combines baseDir + .powerforge/audit-summary.json → full absolute path
   • Then validates it's within the pipeline root (baseDir)
   • But the paths are passed to WebSiteAuditor.Audit() which expects paths relative to siteRoot, not baseDir

2. The Fix: By using relative paths (.powerforge/audit-summary.json), these are later resolved correctly in WebSiteAuditor.ResolveSummaryPath() (line 1595-1606) which:

   • Takes the relative path
   • Combines it with siteRoot (line 1601)
   • Validates it's within siteRoot (line 1603-1604)

3. Alignment with Other Defaults: The fix aligns with how ResolveSummaryPathForPipeline and ResolveSarifPathForPipeline work (lines 2330-2344), which return simple relative paths without calling ResolvePathWithinRoot.

Potential Issues: ⚠️ Minor

1. No whitespace trimming: Unlike other uses of ResolvePathWithinRoot in the codebase (lines 92, 94, 528), these paths are hardcoded, so no trimming is needed. This is fine.

2. Consistency check: Verify that siteRoot and baseDir are the same in the pipeline context where this code runs. Looking at line 973, SiteRoot = effectiveSiteRoot! is passed to the audit, so the paths should resolve correctly.

Performance: ✅ No Impact

The change removes unnecessary path resolution and validation, which is a minor performance improvement.

Security: ✅ No Concerns

• The hardcoded paths .powerforge/audit-summary.json and .powerforge/audit.sarif.json are safe
• Path traversal is not a concern with these fixed relative paths
• The ResolveSummaryPath function still validates paths are within the site root (line 1603-1604)

Test Coverage: ⚠️ Consider Adding

Current state:

• No specific tests for the summaryOnFail/sarifOnFail fallback paths
• Existing test at line 1903 tests SARIF output but with an explicit path, not the default

Recommendation:
Consider adding a test that verifies the fallback behavior when summaryOnFail=true or sarifOnFail=true with no explicit path provided. This would:

• Validate the paths resolve correctly under the site root
• Prevent regression of the original bug
• Document the expected behavior

Example test case structure:

[Fact]
public void Pipeline_SarifOnFail_UsesCorrectDefaultPath()
{
    // Test that sarifOnFail creates .powerforge/audit.sarif.json under site root
}

Architecture: ✅ Good

The fix properly separates concerns:

• Pipeline layer (WebPipelineRunner.cs) provides relative paths
• Audit layer (WebSiteAuditor.cs) resolves them relative to site root
• This is cleaner than the previous approach

Recommendations

1. Consider adding integration test for the fallback path behavior (optional but recommended)
2. Verify CI passes - ensure the fix doesn't break any existing tests
3. Minor: The PR could benefit from a brief note in the commit message explaining why the filename changed from audit.sarif to audit.sarif.json (for consistency)

Conclusion

Verdict: ✅ Approved with minor suggestions

This is a well-targeted bug fix that correctly addresses the path resolution issue. The code quality is good, the fix is minimal and focused, and there are no security or performance concerns. The only suggestions are optional improvements around test coverage and documentation.

Great work identifying and fixing this issue! The change from using ResolvePathWithinRoot to simple relative paths is the right approach given that WebSiteAuditor.ResolveSummaryPath handles the validation.

---

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adjusts the CLI pipeline runner’s audit “on-fail” default artifact paths so they resolve under the built site root (as required by WebSiteAuditor), preventing failures when defaults are used.

Changes:

• Update audit fallback SummaryPath to a relative path under the site root (.powerforge/audit-summary.json) when summaryOnFail is enabled and no path is provided.
• Update audit fallback SarifPath similarly (.powerforge/audit.sarif.json) when sarifOnFail is enabled and no path is provided.
• Apply the same fallback behavior in both the pipeline audit step and the doctor task audit path.

---

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

[Copilot]

These default fallback paths are hardcoded with forward slashes. Elsewhere in this file the repo consistently uses Path.Combine(...) for filesystem paths (e.g., ResolvePathWithinRoot(..., Path.Combine(".powerforge", ...)) around WebPipelineRunner.cs:92-95). Consider switching these to Path.Combine(".powerforge", ...) (still relative to SiteRoot) to avoid mixed separators and keep path construction consistent/cross-platform.

[Copilot]

Same as the earlier audit step: these fallback paths are hardcoded with /. For consistency with the rest of this file (and to avoid mixed separators when later combined with SiteRoot), prefer Path.Combine(".powerforge", "audit-summary.json") / Path.Combine(".powerforge", "audit.sarif.json") or a shared constant used by both code paths.