feat(web): add templated route fallback pipeline task

[PrzemyslawKlys]

Summary

• add a first-class route-fallbacks pipeline task for manifest-driven SPA/Blazor route shells
• document and schema this task in PowerForge.Web
• cover route generation, missing manifest values, and site-root escape protection with tests

Validation

• dotnet test .\\PowerForge.Tests\\PowerForge.Tests.csproj --filter "FullyQualifiedName~RouteFallbacks" -c Release

[claude]

Code Review

Overall this is a well-structured addition. The feature is well-thought-out with good path-traversal protection, clear error messages, and solid documentation. A few items worth addressing:

---

Bugs / Correctness

1. htmlEncode applies to rootValues static strings too

In ExecuteRouteFallbacks, the root file is written as:

var rootContent = ApplyRouteFallbackReplacements(templateContent, rootReplacements, null, htmlEncode);

rootValues are static strings supplied by the user, not derived from untrusted JSON item data. Applying HTML encoding to them by default may double-encode values the user already encoded (e.g. & → &). Consider applying htmlEncode only to per-item values (the values dict), or document clearly that rootValues must be pre-encoding-aware.

2. Empty rootValues with rootOutput set leaves template tokens unreplaced

If a user sets rootOutput but omits rootValues, ApplyRouteFallbackReplacements is called with an empty map — template tokens from routeReplacements are not applied to the root file, so they survive verbatim in the output. ValidateRouteFallbackTokens only validates tokens are present in the template, not that the root file will actually have them substituted. This is a silent data-correctness issue worth either erroring on or documenting.

---

Performance

3. Regex re-compiled on every call in ExpandRouteFallbackTemplate

return Regex.Replace(template, "\\{(?<name>[^{}]+)\\}", match => { ... });

Regex.Replace(string, string, ...) compiles the pattern each invocation. For N route items this runs N+1 times. Use a static readonly Regex field or, if the project targets .NET 7+, a [GeneratedRegex] source-generated pattern:

[GeneratedRegex(@"\{(?<name>[^{}]+)\}", RegexOptions.CultureInvariant)]
private static partial Regex PlaceholderPattern();

4. JsonSerializerOptions instantiated per report

var json = JsonSerializer.Serialize(report, new JsonSerializerOptions { WriteIndented = true });

JsonSerializerOptions instances are not lightweight — they build internal caches on first use. Hoist this to a static readonly field shared across calls.

---

Test Coverage Gaps

5. No test for htmlEncode: false
There is no assertion that raw (non-encoded) values are emitted when htmlEncode: false, which is the only way to verify the opt-out path works.

6. No test for rootOutput without rootValues
The behaviour described in issue #2 above is untested.

7. No test for a flat JSON array (non-nested items)
The ResolveRouteFallbackItems method handles a root-level JSON array, but none of the tests exercise this path — only the { "routes": [...] } object form is tested.

8. No test for a custom itemsProperty key
itemsProperty / items-property / itemsKey / items-key variants are never exercised by tests.

9. Path-traversal test covers route items but not rootOutput
A malicious or misconfigured rootOutput: "../../etc/passwd" is not tested. The escape check in WriteRouteFallbackFile should protect it, but an explicit test would confirm that.

---

Code Quality

10. Test class has no namespace

public class WebPipelineRunnerRouteFallbacksTests
{

If the rest of the test project uses a namespace, this class is an outlier and may cause tooling confusion. Verify this is consistent with other test files in the project.

11. TryGetRouteFallbackProperty is a duplicate of likely existing helpers

The method does case-insensitive property lookup via EnumerateObject(). The existing codebase already has GetString(step, ...) which probably wraps the same pattern. Consider using or extending the shared helper rather than introducing a new RouteFallback-prefixed copy.

---

Minor Nits

• NormalizeRouteFallbackRelativePath strips leading / with a while loop and Substring(1) — TrimStart('/') is more idiomatic.
• The Report classes (RouteFallbackReport, RouteFallbackReportEntry) are private sealed inside the partial class, which is fine, but they could benefit from [JsonPropertyName] annotations if the report JSON naming convention needs to match other reports in the project.

---

The path-traversal protection, HTML-encode-by-default, and the three targeted tests are all highlights of this PR. The items above are mostly gaps to fill out rather than blockers.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 49b2f74ee8

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

Include route manifest/template files in cache fingerprint

This task consumes external files via template/items, but those property names are not part of the fingerprinted path key set used by pipeline caching. As a result, when cache: true is enabled, changing routes.json or the HTML template can still produce a cache hit and skip regeneration, leaving stale fallback metadata in _site. Please add these new input keys (and aliases) to the fingerprint path list, or mark this task as non-cacheable until fingerprinting is complete.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Verify concrete fallback outputs before accepting cache hit

The expected-output check for route-fallbacks only probes siteRoot (and optional reportPath). Because siteRoot can exist even when generated route files are missing, cache validation can report success and skip the step after route HTML files were deleted or partially cleaned, resulting in missing pages at runtime. Add deterministic output paths (for example rootOutput and/or a tracked generated-file list) so missing fallback files invalidate the cache.

Useful? React with 👍 / 👎.

[claude]

Code Review: feat(web): add templated route fallback pipeline task

Overall this is a well-structured addition. The path traversal protection is correct, HTML encoding is on by default, the alias coverage is comprehensive, and the three test cases hit the most important security and error paths. A few things worth addressing before merge:

---

Bugs / Correctness

1. WrittenCount includes the root file; ItemCount does not

RouteFallbackReport.ItemCount is set to items.Count (route items only), while WrittenCount is written (root file + route items). A consumer of the report JSON would see writtenCount exceed itemCount whenever rootOutput is set, which is surprising. Either increment ItemCount to reflect the root write, rename the field to RouteCount, or add a separate RootWritten flag.

2. Case-sensitivity inconsistency in step config property lookup

TryGetRouteFallbackProperty does a case-insensitive O(n) linear scan. It is used in ReadRouteFallbackStringMap to find "replacements" / "rootValues" etc. in the step config JSON. But every other step config property (siteRoot, template, items, destinationTemplate, …) goes through GetString, which calls TryGetProperty — a case-sensitive O(log n) lookup.

Result: a user who writes "Replacements": {} gets it picked up, but one who writes "SiteRoot": "…" gets a confusing "route-fallbacks requires siteRoot" error. Recommend using GetString consistently for step-config properties, and reserving the case-insensitive scan only for the items-manifest object lookup inside ResolveRouteFallbackItems.

3. rootValues tokens validated even when rootOutput is absent

ValidateRouteFallbackTokens merges tokens from both routeReplacements and rootReplacements into one HashSet and requires all of them to appear in the template. If a user specifies rootValues but omits rootOutput (perhaps as a future placeholder), the validation would fail on root-specific tokens that don't appear in the template, even though the root file will never be written. A simple guard — skip root token validation when rootOutput is absent — would prevent this surprise.

---

Performance

4. Non-compiled regex in a per-item hot loop

ExpandRouteFallbackTemplate calls Regex.Replace with a string pattern on every route item. This compiles the regex on each call. The pattern is fixed, so it should be a static readonly Regex field (or use [GeneratedRegex] on .NET 7+, which appears to be in scope given the C# 11 raw string literals in the tests).

[GeneratedRegex(@"\{(?<name>[^{}]+)\}", RegexOptions.CultureInvariant)]
private static partial Regex PlaceholderPattern();

---

Missing Test Coverage

The three existing tests are good, but these paths are untested:

Scenario	Why it matters
htmlEncode: false	The encoding path is non-trivial; confirm raw values land verbatim
Items file is a bare JSON array (not wrapped in object)	ResolveRouteFallbackItems has a special branch for this
itemsProperty override	The custom array key path in ResolveRouteFallbackItems is otherwise untested
Template missing a replacement token	ValidateRouteFallbackTokens throws — confirm message is actionable
rootValues without rootOutput	Exposes the validation inconsistency noted above

---

Workflow change (.github/workflows/powerforge-website-run.yml)

The fix — switching the CLI --powerforge-repository / --powerforge-ref arguments from the raw inputs.* values to steps.workflow_source.outputs.* — is correct and consistent with the actions/checkout step at line 132–133 which already used the resolved outputs. Previously the tool could receive an unresolved ref while the checkout used the resolved SHA, which would cause a mismatch. No issues here, just noting it's a good catch.

---

Nits

• NormalizeRouteFallbackRelativePath uses normalized.Substring(1) instead of normalized[1..]. Minor, just consistency with modern C#.
• ReadRouteFallbackStringMap duplicates the EnumerateObject / null-skip / GetString-or-ToString pattern that also appears in BuildRouteFallbackValueMap. A single private ReadStringDictionary(JsonElement obj) helper would remove the duplication.
• The RouteFallbackReport and RouteFallbackReportEntry private classes could be record types since they're only serialised, not mutated after construction — but this is a minor style point.

---

The path-traversal security check (IsPathWithinRoot after Path.GetFullPath) is solid, HTML encoding defaults are the right call for SPA route shells, and the schema allOf/anyOf required-field enforcement is thorough. Fixing items 1–3 above and adding a couple more tests would make this merge-ready.

[claude]

Code Review

Overall this is a well-structured addition — the feature is clearly scoped, the security-sensitive path traversal check is present, HTML encoding defaults to true, incremental writes avoid unnecessary I/O, and the three test cases cover the key failure modes. A few items worth discussing:

---

Notable issues

1. ShouldProbeHostAssemblyPaths — Microsoft.Extensions.* and similar assemblies fall through to true

WebApiDocsGenerator.Reflection.Attributes.cs lines ~527–544

The exclusion list covers System.* and Microsoft.Win32.*, but any other Microsoft.* assembly (e.g. Microsoft.Extensions.Logging, Microsoft.AspNetCore.*, Microsoft.CSharp) falls through to the default return true and gets probed from the host. If PowerShell's host ships a different version of those assemblies than what the loaded plugin expects, this could silently load the wrong version. Was intentionally allowing all non-Win32 Microsoft assemblies the goal, or should Microsoft.Extensions.* / Microsoft.AspNetCore.* / Microsoft.CSharp.* also be excluded?

// Current behaviour — Microsoft.Extensions.Logging → true (probed from host)
if (assemblyName.StartsWith("Microsoft.Win32.", ...))
    return false;

return true;   // catch-all

2. rootOutput without rootValues silently produces an unreplaced template

WebPipelineRunner.Tasks.RouteFallbacks.cs — the rootOutput block

When rootOutput is set but rootValues is omitted, ApplyRouteFallbackReplacements is called with an empty rootReplacements dict. The loop body is never entered, so the output file is written with every token (e.g. __PF_TITLE__) still in place. The docs say rootValues is optional, but a file full of unreplaced placeholder tokens is almost certainly wrong. Consider either:

• Requiring rootValues when rootOutput is set, or
• Emitting a warning log when the combination is used and rootReplacements is empty.

---

Minor observations

3. written counter is redundant with report.Files.Count

report.WrittenCount = written is always equal to report.Files.Count. The written local can be dropped and report.Files.Count used directly before serialisation.

4. Report-write failure after all files already committed

WriteRouteFallbackReport is called after the loop completes. If it throws (disk full, bad path), the step fails even though every output file was already written successfully. A caught exception with a warning message (or a non-fatal flag) would give callers a clearer picture of what actually happened.

5. Regex allocation in ExpandRouteFallbackTemplate

Regex.Replace with a runtime pattern string is called once per replacement-value per item. .NET caches the last N compiled regexes, so in practice this is fine for normal manifests. If very large manifests are expected, promoting the pattern to a [GeneratedRegex] static (available since .NET 7) would eliminate the lookup overhead entirely and is already the idiomatic pattern in newer C# code.

[GeneratedRegex(@"\{(?<name>[^{}]+)\}", RegexOptions.CultureInvariant)]
private static partial Regex RouteFallbackTokenRegex();

6. Test gaps

• No test exercises a bare JSON array ([{"slug":"x"}]) as the items file — only the object-with-routes-key format is covered.
• No test for htmlEncode: false — the flag is wired up but the non-encoding path has zero coverage.
• No test for rootOutput set without rootValues (relates to issue Customized Release Path #2 above).

---

Workflow fix (commit 3ccf923)

The change from ${{ inputs.powerforge_repository }} → ${{ steps.workflow_source.outputs.repository }} looks correct; using resolved source outputs rather than raw caller inputs is the right approach when the source engine has already normalised them.

---

Assembly load order change (commit fc6fbcf)

Moving the alreadyLoaded check before _hostProbePaths is a sensible fix — preferring an already-loaded assembly over re-loading from a host probe path avoids loading the same logical assembly into two different load contexts. The new test theory covers the important boundary cases well.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fc6fbcf85d

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

Preserve lock-based engine resolution

Passing steps.workflow_source.outputs.repository/ref into website-runner makes --powerforge-repository and --powerforge-ref non-empty on every run, so source mode no longer falls back to .powerforge/engine-lock.json (the fallback in WebWebsiteRunner.RunSourceMode only happens when either value is empty). In the common case where callers leave these inputs blank, this silently ignores the committed engine lock and forces the workflow SHA instead, which can run a different engine commit than the site’s pinned lock policy.

Useful? React with 👍 / 👎.

[claude]

Code Review: feat(web): add templated route fallback pipeline task

Overall this is a well-structured, production-quality addition. The feature is clearly documented, has solid test coverage, and fits the existing patterns in the codebase. A few things worth discussing:

---

Potential Bugs

1. rootOutput + empty rootValues silently emits un-replaced tokens

WebPipelineRunner.Tasks.RouteFallbacks.cs (~line 638):

var rootContent = ApplyRouteFallbackReplacements(templateContent, rootReplacements, null, htmlEncode);

If a user sets rootOutput but omits rootValues, rootReplacements is an empty dictionary. ApplyRouteFallbackReplacements returns the template unchanged — meaning __PF_TITLE__ etc. end up verbatim in the output HTML with no error or warning. Consider throwing (or at least warning) when rootOutput is set but rootValues is empty.

2. Assembly load context: alreadyLoaded now runs before host-path probing

WebApiDocsGenerator.Reflection.Attributes.cs (~line 958):
The alreadyLoaded check was moved before _hostProbePaths lookup. In .NET's AssemblyLoadContext model, AppDomain.CurrentDomain.GetAssemblies() can return assemblies loaded in a different context. Returning such an assembly can break type identity for isolated reflection scenarios. The new ShouldProbeHostAssemblyPaths filter is a good idea, but the alreadyLoaded early-return warrants a comment explaining why cross-context re-use is safe here (or ensuring it's guarded to the default ALC).

---

Code Quality

3. Bare catch in cache path hides I/O errors

WebPipelineRunner.Cache.cs (~line 519):

catch
{
    // Fall back to the coarse outputs so the task itself can report the real validation error.
}

The intent is clear, but a bare catch also swallows OutOfMemoryException, StackOverflowException, etc. Narrowing to catch (Exception) (or a specific set of expected types) would be safer. At minimum, it is worth adding a diagnostic trace here so unexpected errors are visible when debugging.

4. Generic property names "data" and "manifest" added to shared list

WebPipelineRunner.cs (~line 915):

"items", "itemsPath", "items-path", "manifest", "data",

This looks like a list of known path-type fields used for cache fingerprinting or resolution. "data" and "manifest" are common property names — if any other existing or future task uses them for non-path values, adding them here could silently corrupt cache keys or path resolution for those tasks. Consider scoping these aliases so they are only recognized in the context of route-fallbacks steps.

---

Performance

5. Regex.Replace called with an uncompiled pattern in the hot path

WebPipelineRunner.Tasks.RouteFallbacks.cs (~line 814):

return Regex.Replace(template, "\\{(?<name>[^{}]+)\\}", match => { ... }, RegexOptions.CultureInvariant);

ExpandRouteFallbackTemplate is called once per replacement token per manifest item. For large manifests this re-compiles the regex each call. The file already uses partial class for source-generated regex in other places — a [GeneratedRegex] constant here would be consistent and faster.

6. WriteRouteFallbackFile reads the whole existing file to check for changes

WebPipelineRunner.Tasks.RouteFallbacks.cs (~line 849):

var existing = File.Exists(fullPath) ? File.ReadAllText(fullPath) : null;
if (string.Equals(existing, content, StringComparison.Ordinal))
    return false;

For the typical SPA use-case (small HTML files) this is fine. But the approach doesn't short-circuit on size mismatch before doing a full string comparison, and reads the whole file into a string even when byte counts already differ. A quick FileInfo.Length pre-check or a byte-level hash compare would make this more efficient without changing semantics.

---

Minor Notes

• NormalizeRouteFallbackRelativePath strips leading slashes but does not collapse double slashes like tools//spf/. Path.GetFullPath handles this downstream, so it is not a correctness issue, but normalizing here would make paths in reports/logs cleaner.
• Test class at file scope (WebPipelineRunnerRouteFallbacksTests): Other test files in the project appear to use a namespace. Omitting it here is valid C# but inconsistent; worth aligning for grep-ability.
• The caching tests require exactly 3 runs before a step is considered cached — this is non-obvious. A short comment in CreateCachedRouteFallbackFixture explaining the two-warm-up runs would save the next reader a trip to the cache implementation.

---

Summary: No blockers found. The path-traversal guard, HTML encoding, token validation, and cache fingerprinting are all correctly implemented. The main things I'd address before relying on this in a large pipeline are the rootOutput+empty-rootValues silent passthrough (item 1) and the generic property names in the shared list (item 4).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2d24d850e6

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

Skip root token validation when rootOutput is absent

rootValues are always included in ValidateRouteFallbackTokens, even when rootOutput is not configured, so a step that only generates per-route files can fail with “template does not contain required token …” for unused root-only tokens. This makes optional root metadata effectively mandatory in mixed configs and breaks valid route-only runs whenever rootValues is present but not applicable.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Fail when configured itemsProperty is not a readable array

When itemsProperty is supplied, the resolver still falls back to default names (items, routes, entries) if that configured property is missing or not an array. In manifests that contain multiple arrays, a typo or schema drift in itemsProperty will silently select a different dataset and generate incorrect fallback pages instead of failing fast on the misconfiguration.

Useful? React with 👍 / 👎.