Add native link-service tasks and SEO doctor route support#326
Add native link-service tasks and SEO doctor route support#326PrzemyslawKlys merged 23 commits intomainfrom
Conversation
![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: 0e32dd550f
ℹ️ 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 exactPath = path == "/" ? "/" : path.TrimEnd('/'); | ||
| var literal = exactPath + "?" + sourceQuery; | ||
| return "\"^" + Regex.Escape(literal) + "$\""; |
There was a problem hiding this comment.
The nginx branch for query-specific redirects strips the normalized path's trailing slash before building the $request_uri regex, so a rule from a typical route like /docs/?v=1 is emitted to match only /docs?v=1. Because NormalizeRedirectSourcePath normalizes non-root routes with a trailing slash, real requests that include that slash (/docs/?v=1) will bypass the redirect entirely in nginx while Apache/IIS equivalents still match.
Useful? React with 👍 / 👎.
| var decodedUrl = System.Web.HttpUtility.HtmlDecode(url); | ||
| var replaced = ApplyRewriteRules(decodedUrl, rewrites); | ||
| return replaced == url ? match.Value : $"{match.Groups["attr"].Value}=\"{replaced}\""; |
There was a problem hiding this comment.
After decoding HTML entities in href/src, the code compares the rewritten URL against the original encoded string and then writes the decoded value back unescaped. With rewrites enabled, links containing entity-encoded query chars (for example &) are treated as changed even when no rule matches, so output HTML is mutated and may end up with unescaped attribute content.
Useful? React with 👍 / 👎.
Code Review — PR #326: Add native link-service tasks and SEO doctor route supportOverviewThis PR adds three major capabilities:
All changes are backward-compatible (new parameters have safe defaults). The architecture follows the project's established service-first pattern correctly. Issues Worth AddressingMedium priority1. Silent CSS strategy fallback on typos ( When // Suggestion: add an else-log here
default:
_logger.LogWarning("Unknown CssStrategy '{Strategy}', falling back to blocking", strategy);
return "blocking";2. Potential double-decode of HTML entities (
3. The line-counting helper iterates the full document substring for each regex match. For a document with many matches this compounds — a 5 000-line file with 200 flagged patterns triggers 200 substring scans. Consider pre-splitting lines once per file: var lines = content.Split('\n');
// then use Array.IndexOf or a pre-built offset tableFor typical Markdown file sizes this is fine, but could become noticeable if users run SEO doctor over very large generated API reference pages. 4. Removed The simplified pattern reduces false positives in API doc pages (which is the intent and is validated by the new test). However, the trade-off is that actual content leaks whose first "leaked" token looks like a Markdown heading will no longer be flagged. The new frontmatter-delimiter guard ( Low priority / informational5. No timeout on The 6. Silent swallowing of AMP probe failures ( Multiple 7. Redirect CSV status codes not validated as 3xx ( The status-code column is parsed with 8. Language code values are not validated (pipeline task handler)
Test CoverageNew tests are well-targeted and cover the primary happy paths:
Gaps to consider (not blocking):
SecurityNo significant concerns. The HTTP-only asset download path correctly guards against non-HTTP(S) schemes. The SummaryThis is a solid, well-structured addition. The code follows the project's service-first conventions, the backward-compatibility story is clean, and the test suite covers the new behavior meaningfully. The medium-priority items above (CSS strategy fallback, possible double-decode, O(n) line counting, and the leak-pattern trade-off) are worth a second look before merge, but none are blocking. The low-priority items can be addressed as follow-ups.
|
PR #326 Code Review — Add native link-service tasks and SEO doctor route supportOverview: This PR adds legacy WordPress AMP redirect generation, Apache redirect pipeline tasks, SEO Doctor language-root-host support, improved editorial image metadata precedence, and a standalone Bugs / CorrectnessHigh: Bare catch // should be: catch (Exception ex) { ... }
{
return relativePath;
}Suggest scoping to Medium: Medium: Trailing-slash inconsistency in Low: Inconsistent path format in Test ConcernsWeakened social card assertion // Before
Assert.Equal(68, logoFrame!.Value.Y);
// After
Assert.True(logoFrame!.Value.Y > 68);Changing from an exact equality check to a lower-bound assertion hides potential regressions. If the layout algorithm changes again, tests would still pass while the output drifts. Please comment explaining why the value is now non-deterministic (e.g., font metrics vary by platform), or restore the exact check. No tests for
SecurityXXE risk in sitemap XML parsing (Compare-WebSitemaps.ps1 ~L84-97) $response = Invoke-WebRequest -Uri $Url -UseBasicParsing
[xml]$xml = $response.ContentPowerShell's $settings = [System.Xml.XmlReaderSettings]::new()
$settings.DtdProcessing = [System.Xml.DtdProcessing]::Prohibit
$reader = [System.Xml.XmlReader]::Create([System.IO.StringReader]::new($response.Content), $settings)
$xml = [xml]::new(); $xml.Load($reader)Even if the URLs are trusted inputs in practice, defense-in-depth is worthwhile here. Silent exception swallowing in AMP discovery (Compare-WebSitemaps.ps1 ~L382-387) catch { return $null }Network errors, DNS failures, and unexpected HTML structures all surface as Code QualityDuplicate "try-read-JSON" pattern
Nitpicks
Summary
The SEO Doctor language-root-host work and image metadata precedence changes look well-thought-out. The main asks before merge are: (1) explain or restore the weakened social card assertions, (2) add exception types to the bare catch blocks, (3) address the XXE risk in the sitemap script, and (4) add at least minimal Pester tests for |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb5ee9fb75
ℹ️ 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 baseUrl = localization.ByCode.TryGetValue(effectiveLanguage, out var language) && | ||
| !string.IsNullOrWhiteSpace(language.BaseUrl) | ||
| ? language.BaseUrl | ||
| : null; |
There was a problem hiding this comment.
ResolveAbsoluteLanguageRoute now bypasses ResolveLanguageBaseUrl and only uses the language entry's BaseUrl. In the fallback-to-default path (ResolveLocalizedPageUrl), if the default language omits BaseUrl but spec.BaseUrl is set, this returns a relative route; that route is then reinterpreted as target-language output and can generate hreflang URLs to non-existent localized pages instead of the default-language URL. This regresses split-language fallback behavior for common configs that only set site.baseUrl.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| var path = row.LegacyUrl.Trim(); | ||
| var path = source.PathOnly.Trim(); |
There was a problem hiding this comment.
For absolute legacy_url entries that include query parameters (other than ?p=/?page_id=), export now builds rules from source.PathOnly, which drops the query. That means a source like https://example.com/docs/?v=1 becomes a generic /docs rewrite and will redirect unrelated requests on that host. This broadens redirect scope and can hijack valid traffic; the query constraint needs to be retained for these rows.
Useful? React with 👍 / 👎.
Move sitemap migration analysis into reusable PowerForge.Web services, wire it into the web pipeline, and keep the legacy PowerShell comparison script as a thin wrapper with safer parsing. Address PR review fixes for redirect generation, SEO/link handling, asset rewriting, localization fallbacks, and stabilize the full solution test suite.
Warn on unknown CSS strategy values, make default-language site-base fallback explicit in localized URL resolution, and document query-preserving Apache redirect generation.
Use ResolveLanguageBaseUrl directly for fallback default-language absolute routes and clarify Apache redirect query preservation by separating RewriteRule path from SourceQuery.
Code Review: PR #326 — "Add native link-service tasks and SEO doctor route support"OverviewThis PR has four main thrusts:
Overall the PR is well-structured and test coverage is solid. The issues below are ordered by severity. Bugs / Correctness🔴 public static string NormalizeUrl(string url)
{
var uri = new Uri(url); // throws if url is malformed
🟡 SkippedCount = Math.Max(0, Math.Max(0, lines.Length - 1) - generated.Count)This conflates intentionally-skipped rows (non-3xx status, already-AMP, query URLs) with parse failures. A dedicated 🟡 ReleaseCount = TryReadReleaseHubDocument(existingOutputContent)?.Releases.Count ?? 0,
AssetCount = TryReadReleaseHubDocument(existingOutputContent)?.Releases.Sum(…) ?? 0,Cache the result of 🟡 No max recursion depth for nested sitemaps in C# implementation
Security🟡 using var client = new HttpClient { Timeout = TimeSpan.FromSeconds(...) };
var content = client.GetStringAsync(location).GetAwaiter().GetResult();
var handler = new HttpClientHandler { MaxAutomaticRedirections = 5 };
using var client = new HttpClient(handler) { MaxResponseContentBufferSize = 50 * 1024 * 1024, ... };🟡 catch
{
continue;
}A bare catch (Exception ex) when (ex is IOException or UnauthorizedAccessException)Design / Code Quality🟡
var englishHost = string.IsNullOrWhiteSpace(options.DefaultEnglishHost) ? "evotec.xyz" : ...;
var polishHost = string.IsNullOrWhiteSpace(options.DefaultPolishHost) ? "evotec.pl" : ...;These are also repeated in the pipeline runner. Embedding specific domain names as fallbacks in a reusable library tightly couples it to one deployment. The library should throw 🟡 A new 🟢
🟢 The compatibility shim silently accepts 🟢 Minor: missing newline at end of Performance🟡 Regex.Replace(current, @"-\d+$", string.Empty, RegexOptions.CultureInvariant)Calling 🟡 The final summary block calls 🟢
Test CoverageTest coverage is thorough — each new feature has unit and pipeline integration tests. Minor gaps worth filling:
Summary
The core logic and test suite are well-written. Addressing the 🔴 Review generated with Claude Code |
Code Review — PR #326OverviewThis PR makes three focused additions:
The overall shape is good: concerns are well-separated, documentation is updated, and nearly all new behaviour is exercised by tests. Comments below are mostly observations and a few actionable suggestions. Code QualityPositive
Observations
Potential Bugs / Issues
Performance Considerations
Security
Test CoverageCoverage is strong. New behaviour is exercised end-to-end through One gap: there is no test for SummarySolid, well-structured PR. The main actionable items are:
Everything else is low-priority or informational. |
Code Review — PR #326: Native link-service tasks and SEO doctor route supportOverviewThis is a large, well-structured PR (~4 500 additions, ~760 deletions) that adds three related capabilities:
The PR also includes a good set of companion tests, backward-compatibility cleanups (deprecated Code Quality & StylePositives
Minor style observations
Potential Bugs / Issues
if (host.Contains('/', StringComparison.Ordinal) ||
host.Contains('\\', StringComparison.Ordinal) ||
string.IsNullOrWhiteSpace(host)) // ← always false hereAfter the scheme-strip block,
The function strips
var existing = TryReadReleaseHubDocument(existingJson); // existingJson = pre-generate content
var generated = TryReadReleaseHubDocument(File.ReadAllText(outputPath)); // outputPath = post-generate fileThe caller in
if (!hasMessage)
message = current.GetType().Name;
message ??= current.GetType().Name; // ← always null-safe here; dead codeAfter the
When a local sitemap index references a Performance Considerations
Security
Test CoverageTest coverage is thorough:
Gaps worth considering:
SummaryThis is a solid, well-tested PR. The main concerns are:
Items 1, 3, and 4 are straightforward fixes. Item 2 is a design trade-off — adding specificity would improve correctness but requires knowing the exact warning strings the generator emits. |
Code Review — PR #326: Add native link-service tasks and SEO doctor route supportOverviewThis is a large, well-scoped PR that adds three main capabilities:
Additionally it fixes false positives in the front-matter/Markdown content-leak detector by making the heading-only signal require a front-matter delimiter, and extends GitHub retry logic to cover 401 alongside 403. Overall the code is well-structured, security-conscious, and ships with strong test coverage. The notes below are mostly minor. Security ✅The PR demonstrates consistently good security hygiene:
Potential Issues1. Sync-over-async in var content = httpClient.GetStringAsync(location).GetAwaiter().GetResult();This blocks the calling thread synchronously. In a CLI process with no 2. Inline regex in var matches = Regex.Matches(
content,
@"!\[\]\((?<target>[^)\r\n]+)\)",
RegexOptions.CultureInvariant,
TimeSpan.FromMilliseconds(250));This is called once per 3. var position = Array.BinarySearch(lineStartOffsets, boundedIndex);
if (position >= 0)
return position + 1;
return Math.Max(1, ~position);When the match index falls exactly on a newline boundary (i.e., the Code QualityPositive highlights:
Minor nits:
Test Coverage ✅Coverage for new functionality is thorough:
The one gap is that SummaryThis PR is production-quality. The three concerns worth addressing before merge:
Everything else looks solid. The security posture is strong, the architecture is clean, and the test suite gives good confidence in the new paths. |
Code Review — PR #326: Native link-service tasks and SEO doctor route supportOverviewThis PR delivers three major additions:
The scope is large (4,657 additions), but changes are well-contained and the test coverage is solid. Below are findings by category. SecurityPositive: XXE protection added to PowerShell sitemap parser. $settings.DtdProcessing = [System.Xml.DtdProcessing]::Prohibit
$settings.XmlResolver = $null
$xml.XmlResolver = $nullThis is a meaningful improvement, especially for remote sitemap URLs. Positive: Path traversal guard added to Concern: lines.Add($"RewriteRule ^{escapedPath}/?$ {row.TargetUrl} [R={row.Status},L,QSD]");If a target URL contains spaces or other Apache config metacharacters, the generated Minor: CorrectnessPositive: Nginx URI matcher trailing-slash fix is correct. // Before: "^\/path\?v\=1$" — strict, won't match /path/?v=1
// After: "^\/path/?\?v\=1$" — handles both /path?v=1 and /path/?v=1The Positive: Potential issue: Minor: --{2,3} # matches -- or ---Two consecutive dashes appear commonly in prose (em-dash approximation, code comments, SQL). If the content scanner encounters
if ($current -match '^(.*?)-(pl|fr|de|es)$' ...)Only four languages are handled. Adding a new language to the site would require a code change here. Passing the known language codes as a parameter would avoid the coupling. TestingTest coverage is comprehensive and well-structured:
Concern: var preserveMethod = typeof(WebPipelineRunner)
.GetMethod("TryPreserveExistingReleaseHub", BindingFlags.NonPublic | BindingFlags.Static);
Assert.NotNull(preserveMethod);If this method is renamed, the Code QualityPositive: Refactoring the 669-line helper blob out of Positive: Positive: Graceful deprecation of Minor: Minor: Multiple JSON property name aliases in Summary
Overall this is well-executed work. The XXE hardening and path traversal fix are welcome security improvements. The main suggestions worth acting on before merge are: (1) evaluate whether |
Code Review — PR #326: Native link-service tasks and SEO doctor route supportOverviewThis is a large, well-structured PR (+4881/-766) that:
The PR comes with broad test coverage and passing CI. Overall quality is high. Notes below range from blocking to advisory. SecurityApache target validation is narrower than its name implies (
The Good: Code QualitySynchronous HTTP in pipeline runner ( var content = httpClient.GetStringAsync(location).GetAwaiter().GetResult();
Reflection against private methods in tests ( Both test files invoke private static methods via
return JsonSerializer.Deserialize<WebReleaseHubDocument>(json, new JsonSerializerOptions { PropertyNameCaseInsensitive = true });
Inconsistent CSV escaping helpers
The helper allocates and immediately releases N consecutive listeners before returning Potential Bugs / Semantic ChangesModule manifest test flip ( Several assertions flip from
The new
When
The test change moves the English base URL from Minor Notes
SummaryThe core logic is solid and the security boundaries (path traversal, DTD processing disabled, Apache target validation, response size cap) are well considered. The main action items are:
|
Code Review — PR #326: Native link-service tasks and SEO doctor language-root supportOverviewThis is a substantial, well-organized PR (+4889/-766). It delivers:
Test coverage is broad and the commit history is clean. The points below are in priority order. Bugs / Correctness1. Unhandled
// WebSeoDoctor.cs – ScanSourceMarkdownEmptyAlt
var matches = EmptyMarkdownImageAltPattern.Matches(content);A pathological markdown file will propagate the exception through 2. Nginx query-string regex: raw // WebSiteBuilder.Redirects.cs
var exactPath = path.TrimEnd('/');
return "\"^" + Regex.Escape(exactPath) + "/?" + Regex.Escape("?" + sourceQuery) + "$\"";The 3. // WebLinkService.LegacyAmp.cs
private static string NormalizeCanonicalLegacyPath(string path)
{
...
return normalized + "/";
}This is the opposite convention from every other path normalization in the codebase (which strips trailing slashes). If a target URL produced by Design / Maintainability4.
5. # Compare-WebSitemaps.Helpers.ps1
if ($current -match '^(.*?)-(pl|fr|de|es)$' ...)This is domain-specific to Evotec's content. The C# counterpart 6. private static readonly Regex FrontMatterDelimiterPattern = new(
@"(?:^|\s)---(?:\s|$)",
RegexOptions.IgnoreCase | RegexOptions.CultureInvariant | RegexOptions.Compiled);Without 7. Removing Positive Highlights
Minor / Nits
Summary: The PR is solid and well-tested. The one issue worth fixing before merge is the unhandled |
Code Review — PR #326: Native link-service tasks and SEO doctor route supportThis is a substantial, well-structured PR (+4907 / -766) that promotes migration tooling from a project-local PowerShell script into first-class pipeline tasks and adds meaningful SEO doctor capabilities. Overall the code quality is high and test coverage is thorough. Below are observations by area. OverviewThree main areas of change:
Positives
Issues and SuggestionsMedium
Minor / Style
Security
Test CoverageCoverage is strong. A few gaps worth noting:
SummaryThis is a high-quality contribution. The main code path is well tested, security boundaries are in place, and the deprecation strategy is clean. The issues above are mostly minor clarity/robustness notes; none block merging. The hardcoded language-suffix list in the PowerShell helper and the boolean CSV serialisation format are the items most worth addressing before wider adoption. |
Code Review: PR #326 — Native link-service tasks & SEO doctor route supportOverall this is a well-structured PR with good XML security hardening and thoughtful deprecation patterns. A few items need attention before merging. Overview~5,700 lines of new code across 30+ files:
🔴 CriticalPath traversal check could be strengthened ( The current var relative = Path.GetRelativePath(basePath, fullNestedPath);
if (relative.StartsWith("..", StringComparison.Ordinal))
throw new InvalidOperationException(
$"Nested sitemap path '{fullNestedPath}' escapes the base directory '{basePath}'.");
🟠 High PrioritySlug variant regex is now significantly more permissive ( The old pattern was Side effect: slugs like
🟡 Medium PriorityBroad catch (Exception) // masks ArgumentException, NullReferenceException, etc.Prefer a filtered catch: catch (Exception ex) when (ex is ArgumentException
or NotSupportedException or PathTooLongException
or IOException or UnauthorizedAccessException)Error chain format change ( CSV column index validation ( ✅ What's Done Well
Test Coverage AssessmentGood coverage on the happy paths. Gaps worth noting:
The SEO language-root host mapping tests and the markdown empty-alt regex tests are thorough — the SummaryConditional approval. The path traversal hardening and slug-variant behavioral documentation are the two items that should be addressed before merge. Everything else is medium/low priority and could follow in a small cleanup PR. 🤖 Reviewed with Claude Code |
Code Review — PR #326: Add native link-service tasks and SEO doctor route supportOverviewThis PR adds production-grade sitemap migration tooling, legacy WordPress AMP redirect generation, and SEO doctor enhancements. The scope is large (4,935 additions / 766 deletions across 30+ files) but well-organized. No breaking changes detected; all new functionality is additive with backward-compatibility maintained. Strengths
Issues & SuggestionsMedium — Blocking HTTP call in async contextIn Suggestion: Wrap with Medium — 50 MB response buffer is generous for untrusted inputsThe sitemap HTTP fetch caps the response buffer at 50 MB. For an internal build tool this is likely fine, but if the tool is ever pointed at third-party sitemaps, a slow/large response can exhaust memory before the timeout fires. Suggestion: Add a note in the XML doc / task help text that this limit exists, or enforce a hard byte cap via Low — Overly broad
|
Code Review — PR #326: Native Link-Service Tasks & SEO Doctor EnhancementsOverall assessment: This is a well-structured, feature-complete PR with solid test coverage and good security awareness. The code is production-ready with a few items worth addressing before or after merge. OverviewThree major additions:
Architecture is clean: analysis logic sits in the library layer, CLI tasks orchestrate I/O, models are immutable records. PowerShell and C# implementations are appropriately parallel rather than sharing code across boundaries. Security — Good Baseline, One Gap Worth ClosingXXE prevention ✓ — URL scheme validation ✓ — All URL construction goes through Local path traversal ✓ — Potential gap — nested remote sitemaps In // Suggested guard in ResolveNestedSitemapLocation():
if (IsRemoteSitemapLocation(nestedPath))
{
var nestedUri = new Uri(nestedPath);
var rootUri = new Uri(baseLocation);
if (!nestedUri.Host.Equals(rootUri.Host, StringComparison.OrdinalIgnoreCase))
throw new InvalidOperationException($"Nested sitemap URL crosses origins: {nestedPath}");
}Potential BugsMissing header error handling — Medium In if (legacyIndex < 0)
throw new InvalidOperationException("Source CSV does not contain a 'legacy_url' column.");Regex backtracking in PowerShell — Low
Code QualityStrengths:
Opportunities:
Test CoverageGood coverage:
Gaps:
PerformanceNo significant concerns for typical usage. One note: Summary
The nested remote sitemap origin check and the CSV header error are the two items I'd most recommend addressing. Everything else is polish. Nice work on the XXE prevention and the thorough URL/host normalization. |
Code Review — PR #326OverviewThis PR adds three major capabilities to the PowerForge web pipeline:
The refactor also extracts Security ✅The security-sensitive areas look well handled:
Bug:
|
Code Review — PR #326: Native link-service tasks + SEO doctor language root supportOverviewThis is a large, well-structured PR (~5 039 additions / 766 deletions) spanning three distinct areas:
Overall the code is clean, purposeful, and well-tested. The observations below are ranked from most actionable to cosmetic. Potential Issues1. File.WriteAllText(resolvedPath, JsonSerializer.Serialize(payload, new JsonSerializerOptions { WriteIndented = true }));Constructing a new 2. var match = Regex.Match(legacyPath, pattern, RegexOptions.CultureInvariant | RegexOptions.IgnoreCase);Paths come from trusted sitemap inputs so the ReDOS risk is low in practice, but every other pattern-intensive method in the file uses compiled static regexes. Promoting the five special-case patterns to static 3. Behavioral change in 4. Duplicate 5. Positive observations
Minor nits
SummaryThe PR is merge-ready from a correctness standpoint. The most actionable items are the 🤖 Review generated with Claude Code |
Code Review — PR #326: Native link-service tasks and SEO doctor route supportOverviewThis is a substantial PR (~5 000 additions / ~766 deletions) that:
Code Quality & StylePositive highlights:
Minor style notes:
Potential Bugs / Issues
Security
Performance
Test CoverageWell covered:
Gaps to consider:
SummaryThe refactor is well-structured and the security hardening (XXE, path traversal, cross-origin) is genuine and tested. The main items worth addressing before merge:
Everything else is minor style or low-priority robustness notes. Good work overall. |
Code Review — PR #326: Link-service tasks & SEO doctor improvementsOverviewThis is a substantial PR (~5,000 net additions) delivering three distinct feature areas:
The code is well-structured and the test coverage for the new behaviour is solid. Below are specific findings. Bugs / Correctness
The allow-list also includes Code QualityDuplicated host-key normalisation helpers Three CSV serialisation helpers
Three separate implementations with subtly different escaping semantics are worth unifying for consistency and maintainability.
CSV header quoting inconsistency Performance
sync-over-async in Security
Test CoverageTest coverage for the new functionality is strong:
Missing coverage suggestions:
Minor
SummaryThis is a solid, well-tested PR. The main actionable items before merge are:
Everything else is non-blocking. |
Code Review — PR #326: Native link-service tasks & SEO doctor route supportOverviewThis PR ships three major features:
The overall code quality is high. The security practices are solid, the naming is consistent, and the new test suites are well-structured. Notes below are mostly refinements, not blockers. Security — Positive findings
Issues / Suggestions1. Unbounded regex — potential ReDoS risk
Suggestion: Apply the same timeout pattern: private static readonly Regex FrontMatterDelimiterPattern = new(
@"...",
RegexOptions.Multiline | RegexOptions.Compiled,
TimeSpan.FromMilliseconds(250));And catch 2. No retry logic for remote sitemap fetches
Suggestion: Even a simple two-attempt retry with a short delay (or using 3. CSV parsing robustness
Suggestion: If source CSVs are fully controlled and will never contain quoted commas, add an assertion or validation comment. Otherwise, consider using 4. Diacritic normalization is Polish-only
Suggestion: At minimum, document the assumption ("This is intentionally scoped to Latin/Polish slug normalization") so future contributors don't expect broader Unicode coverage. 5.
|
| Priority | Item |
|---|---|
| Medium | Add MatchTimeout to FrontMatterDelimiterPattern |
| Medium | Mark DefaultEnglishHost/DefaultPolishHost as [Obsolete] |
| Low | Add retry logic for remote sitemap HTTP fetches |
| Low | Validate or document CSV quoting assumptions |
| Low | Narrow the broad catch (Exception) in TryReadManifestVersion |
No blockers found. Ready to merge after addressing the regex timeout gap.
Reviewed by Claude Sonnet 4.6
Code Review — PR #326Overview: This is a substantial, well-structured PR (+5237 / -766) that adds three main features: native link-service pipeline tasks ( ✅ Strengths
🐛 Potential Issues1. Duplicate write in test setup (likely accidental) In 2. Page metrics computed unconditionally — minor perf regression In 3. Slug-variant language regex ordering ( In 4. Private-method reflection in tests
|
Code Review — PR #326: Add native link-service tasks and SEO doctor route supportOverviewThis is a substantial PR (~5,200 additions, ~740 deletions) that:
The overall direction is good: moving logic from loose PowerShell scripts into typed, testable C# classes with tests to match. Code QualityPositives:
Specific Issues & Suggestions1.
|
1 participant
Summary
evotec.plmapping back to/pl/...Validation
dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --no-restore --logger "console;verbosity=normal" --logger "trx;LogFileName=powerforge-tests-full-20260423.trx" --blame-hang --blame-hang-timeout 5m