Conversation
📝 WalkthroughWalkthroughThe changes fix failing redirect validation tests by enhancing the test setup to handle redirects to arbitrary documentation files. The Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/authoring/Framework/Setup.fs (2)
75-79: Redundant pattern branches.Both
Nonecases returnouterFromKey, making thewhen hasAnchorsguard unnecessary:let effectiveTo = match toVal with | Some t -> t - | None when hasAnchors -> outerFromKey | None -> outerFromKey🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/authoring/Framework/Setup.fs` around lines 75 - 79, The match for computing effectiveTo is redundant because both None branches return outerFromKey; simplify the logic by removing the unnecessary guard and collapse the branches: match toVal with Some t -> t | None -> outerFromKey, keeping symbols effectiveTo, toVal, hasAnchors, and outerFromKey in mind when updating the code.
95-100: Unsafe downcast could throw if YAML structure is unexpected.The direct cast
:?> YamlMappingNodewill throwInvalidCastExceptionifkv.Valueis not a mapping node. Given this parses a controlled production file, the risk is low, but a defensive pattern match would be safer:♻️ Optional defensive fix
match rootMap.Children |> Seq.tryPick (fun kv -> match kv.Key with - | :? YamlScalarNode as k when k.Value = "redirects" -> Some(kv.Value :?> YamlMappingNode) + | :? YamlScalarNode as k when k.Value = "redirects" -> + match kv.Value with + | :? YamlMappingNode as m -> Some m + | _ -> None | _ -> None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/authoring/Framework/Setup.fs` around lines 95 - 100, The match uses an unsafe downcast (kv.Value :?> YamlMappingNode) which can throw if the YAML node isn't a mapping; update the pattern to defensively match kv.Value as a YamlMappingNode (e.g., match kv.Value with :? YamlMappingNode as m -> Some m | _ -> None) so the Seq.tryPick branch only returns a mapping node when it's safe; adjust the expression around rootMap.Children and the kv match accordingly to avoid InvalidCastException.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/authoring/Framework/Setup.fs`:
- Around line 75-79: The match for computing effectiveTo is redundant because
both None branches return outerFromKey; simplify the logic by removing the
unnecessary guard and collapse the branches: match toVal with Some t -> t | None
-> outerFromKey, keeping symbols effectiveTo, toVal, hasAnchors, and
outerFromKey in mind when updating the code.
- Around line 95-100: The match uses an unsafe downcast (kv.Value :?>
YamlMappingNode) which can throw if the YAML node isn't a mapping; update the
pattern to defensively match kv.Value as a YamlMappingNode (e.g., match kv.Value
with :? YamlMappingNode as m -> Some m | _ -> None) so the Seq.tryPick branch
only returns a mapping node when it's safe; adjust the expression around
rootMap.Children and the kv match accordingly to avoid InvalidCastException.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce7570e4-629b-4206-b96c-c653201e01d2
📒 Files selected for processing (3)
tests/authoring/Framework/Setup.fstests/authoring/Generator/LinkReferenceFile.fstests/authoring/authoring.fsproj
* Fix redirect tests * Remove changelog redirects implemented elsewhere
* Fix redirect tests * Remove changelog redirects implemented elsewhere
* Add skip-labels to evaluate-pr's output * Add tests * Resolve conflicts * Fix docs-builder redirect tests (#3008) * Fix redirect tests * Remove changelog redirects implemented elsewhere * Search: Use default semantic_text inference, remove Jina mappings (#3014) Elasticsearch Serverless now defaults semantic_text to Jina, making the explicit Jina sub-fields redundant and the ELSER inference ID unnecessary. This removes both inference ID constants, all .jina field mappings, and lets semantic_text fields use the platform default. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: Update config/versions.yml eck 3.3.2 (#3019) Made with ❤️️ by updatecli Co-authored-by: elastic-observability-automation[bot] <180520183+elastic-observability-automation[bot]@users.noreply.github.com> * Deploy: Use write-scoped filesystem for apply command (#3021) The deploy apply command used RealRead which lacks AllowedSpecialFolder.Temp, causing ScopedFileSystemException when AwsS3SyncApplyStrategy stages files in /tmp/ for S3 upload. Switch to RealWrite which permits temp directory access. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Update Azure EDOT CF version (#3022) +CC @zmoog * Enable AOT/trim analyzers on library projects and skip AOT publish on PRs (#2971) Add IsAotCompatible to 12 library projects referenced by docs-builder so Roslyn's trim/AOT analyzers (IL2026/IL3050) run during regular builds. This catches AOT issues at compile time, removing the need for the expensive native ILC publish step on pull requests. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * HTML: Omit version meta tags for versionless pages (#3020) * HTML: Omit version meta tags for versionless pages Versionless pages (serverless, cloud, etc.) were rendering the sentinel value 99999.0+ in product_version and DC.identifier meta tags. Now these tags are omitted entirely when the page's versioning system is versionless. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * HTML: Restore required modifier on CurrentVersion property Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Layout: Adapt to static elastic-nav by making secondary nav sticky (#3025) * Layout: Adapt to static elastic-nav by making secondary nav sticky The global elastic-nav.js (v2026-03) changed the header from fixed to static positioning. This makes the secondary docs nav sticky at the top and simplifies --offset-top to match its height. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Layout: Only make secondary nav sticky on md+ viewports On mobile the sticky secondary nav takes too much vertical space. Keep it static on small screens (--offset-top: 0) and only sticky on md+ where it provides persistent navigation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: Upgrade lodash to 4.18.x to fix high severity vulnerabilities Fixes code injection via _.template (GHSA-r5fr-rjxr-66jc) and prototype pollution via _.unset/_.omit (GHSA-f23m-r3pf-42rh). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Layout: Add bottom border to secondary nav The visual separator was coming from #main-container's border-top, which scrolls away. Adding border-b to the nav itself keeps the line visible while sticky. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Layout: Remove border-top from main-container The secondary nav now has border-b, so the main-container border-t was causing a double border. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * lint --------- Co-authored-by: Lisa Cawley <lcawley@elastic.co> Co-authored-by: Jan Calanog <jan.calanog@elastic.co> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: elastic-observability-automation[bot] <180520183+elastic-observability-automation[bot]@users.noreply.github.com> Co-authored-by: Fabrizio Ferri-Benedetti <fabri.ferribenedetti@elastic.co>
* Fix redirect tests * Remove changelog redirects implemented elsewhere
* Add skip-labels to evaluate-pr's output * Add tests * Resolve conflicts * Fix docs-builder redirect tests (#3008) * Fix redirect tests * Remove changelog redirects implemented elsewhere * Search: Use default semantic_text inference, remove Jina mappings (#3014) Elasticsearch Serverless now defaults semantic_text to Jina, making the explicit Jina sub-fields redundant and the ELSER inference ID unnecessary. This removes both inference ID constants, all .jina field mappings, and lets semantic_text fields use the platform default. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: Update config/versions.yml eck 3.3.2 (#3019) Made with ❤️️ by updatecli Co-authored-by: elastic-observability-automation[bot] <180520183+elastic-observability-automation[bot]@users.noreply.github.com> * Deploy: Use write-scoped filesystem for apply command (#3021) The deploy apply command used RealRead which lacks AllowedSpecialFolder.Temp, causing ScopedFileSystemException when AwsS3SyncApplyStrategy stages files in /tmp/ for S3 upload. Switch to RealWrite which permits temp directory access. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Update Azure EDOT CF version (#3022) +CC @zmoog * Enable AOT/trim analyzers on library projects and skip AOT publish on PRs (#2971) Add IsAotCompatible to 12 library projects referenced by docs-builder so Roslyn's trim/AOT analyzers (IL2026/IL3050) run during regular builds. This catches AOT issues at compile time, removing the need for the expensive native ILC publish step on pull requests. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * HTML: Omit version meta tags for versionless pages (#3020) * HTML: Omit version meta tags for versionless pages Versionless pages (serverless, cloud, etc.) were rendering the sentinel value 99999.0+ in product_version and DC.identifier meta tags. Now these tags are omitted entirely when the page's versioning system is versionless. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * HTML: Restore required modifier on CurrentVersion property Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Layout: Adapt to static elastic-nav by making secondary nav sticky (#3025) * Layout: Adapt to static elastic-nav by making secondary nav sticky The global elastic-nav.js (v2026-03) changed the header from fixed to static positioning. This makes the secondary docs nav sticky at the top and simplifies --offset-top to match its height. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Layout: Only make secondary nav sticky on md+ viewports On mobile the sticky secondary nav takes too much vertical space. Keep it static on small screens (--offset-top: 0) and only sticky on md+ where it provides persistent navigation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: Upgrade lodash to 4.18.x to fix high severity vulnerabilities Fixes code injection via _.template (GHSA-r5fr-rjxr-66jc) and prototype pollution via _.unset/_.omit (GHSA-f23m-r3pf-42rh). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Layout: Add bottom border to secondary nav The visual separator was coming from #main-container's border-top, which scrolls away. Adding border-b to the nav itself keeps the line visible while sticky. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Layout: Remove border-top from main-container The secondary nav now has border-b, so the main-container border-t was causing a double border. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * lint --------- Co-authored-by: Lisa Cawley <lcawley@elastic.co> Co-authored-by: Jan Calanog <jan.calanog@elastic.co> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: elastic-observability-automation[bot] <180520183+elastic-observability-automation[bot]@users.noreply.github.com> Co-authored-by: Fabrizio Ferri-Benedetti <fabri.ferribenedetti@elastic.co>
* Add skip-labels to evaluate-pr's output * Add tests * Resolve conflicts * Fix docs-builder redirect tests (#3008) * Fix redirect tests * Remove changelog redirects implemented elsewhere * Search: Use default semantic_text inference, remove Jina mappings (#3014) Elasticsearch Serverless now defaults semantic_text to Jina, making the explicit Jina sub-fields redundant and the ELSER inference ID unnecessary. This removes both inference ID constants, all .jina field mappings, and lets semantic_text fields use the platform default. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: Update config/versions.yml eck 3.3.2 (#3019) Made with ❤️️ by updatecli Co-authored-by: elastic-observability-automation[bot] <180520183+elastic-observability-automation[bot]@users.noreply.github.com> * Deploy: Use write-scoped filesystem for apply command (#3021) The deploy apply command used RealRead which lacks AllowedSpecialFolder.Temp, causing ScopedFileSystemException when AwsS3SyncApplyStrategy stages files in /tmp/ for S3 upload. Switch to RealWrite which permits temp directory access. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Update Azure EDOT CF version (#3022) +CC @zmoog * Enable AOT/trim analyzers on library projects and skip AOT publish on PRs (#2971) Add IsAotCompatible to 12 library projects referenced by docs-builder so Roslyn's trim/AOT analyzers (IL2026/IL3050) run during regular builds. This catches AOT issues at compile time, removing the need for the expensive native ILC publish step on pull requests. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * HTML: Omit version meta tags for versionless pages (#3020) * HTML: Omit version meta tags for versionless pages Versionless pages (serverless, cloud, etc.) were rendering the sentinel value 99999.0+ in product_version and DC.identifier meta tags. Now these tags are omitted entirely when the page's versioning system is versionless. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * HTML: Restore required modifier on CurrentVersion property Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Layout: Adapt to static elastic-nav by making secondary nav sticky (#3025) * Layout: Adapt to static elastic-nav by making secondary nav sticky The global elastic-nav.js (v2026-03) changed the header from fixed to static positioning. This makes the secondary docs nav sticky at the top and simplifies --offset-top to match its height. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Layout: Only make secondary nav sticky on md+ viewports On mobile the sticky secondary nav takes too much vertical space. Keep it static on small screens (--offset-top: 0) and only sticky on md+ where it provides persistent navigation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: Upgrade lodash to 4.18.x to fix high severity vulnerabilities Fixes code injection via _.template (GHSA-r5fr-rjxr-66jc) and prototype pollution via _.unset/_.omit (GHSA-f23m-r3pf-42rh). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Layout: Add bottom border to secondary nav The visual separator was coming from #main-container's border-top, which scrolls away. Adding border-b to the nav itself keeps the line visible while sticky. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Layout: Remove border-top from main-container The secondary nav now has border-b, so the main-container border-t was causing a double border. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * lint --------- Co-authored-by: Lisa Cawley <lcawley@elastic.co> Co-authored-by: Jan Calanog <jan.calanog@elastic.co> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: elastic-observability-automation[bot] <180520183+elastic-observability-automation[bot]@users.noreply.github.com> Co-authored-by: Fabrizio Ferri-Benedetti <fabri.ferribenedetti@elastic.co>
* Move S3 upload to a new integrations project and reuse it for changelog uploads. Add tests. * Update tests/Elastic.Documentation.Integrations.Tests/S3/S3EtagCalculatorTests.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Ne'er forget using * Adjust entities being used * Update src/services/Elastic.Changelog/Uploading/ChangelogUploadService.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Add changelog bundle support for hiding private links (#3002) Co-authored-by: Felipe Cotti <felipe.cotti@elastic.co> * Fix ApplicationData path collision with workspace root in Docker/CI containers (#3012) * Reapply "Wrap all IFileSystem usage in ScopedFileSystem (#3001)" (#3011) This reverts commit 108282e. * Fix ApplicationData scope-root collision when HOME is unset in Docker In Docker/CI containers where neither XDG_DATA_HOME nor HOME is set, Environment.GetFolderPath(LocalApplicationData) returns "". Path.Join("", "elastic", "docs-builder") then produces a relative path that resolves to {CWD}/elastic/docs-builder — a subdirectory of WorkingDirectoryRoot. ScopedFileSystem 0.4.0's ValidateRootsAreDisjoint check then throws in FileSystemFactory's static constructor, crashing the process with TypeInitializationException before anything runs. Observed in elastic/elasticsearch (workspace /github/workspace) and elastic/chainguard-image-sync where the resolved ApplicationData path fell inside the runner workspace. Fix: fall back to Path.GetTempPath() when LocalApplicationData is empty so the ApplicationData path is always an absolute non-workspace path. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * Misceleanous fixes, one old pending coderabbit task * Address code review feedback on ScopedFileSystem usage - Use FileSystemFactory.AppData for default checkout dirs (CodexContext, AssembleContext) instead of ReadFileSystem to avoid scope mismatches - Fix RealGitRootForPathWrite to use BuildWriteOptions so AllowedSpecialFolders.Temp is included - Fix DetermineWorkingDirectoryRoot: *.slnx check now respects depth guard (was bypassing it entirely) - Fix GitLinkIndexReader.CloneDirectory to use Paths.ApplicationData instead of raw LocalApplicationData (handles Docker/CI empty HOME) - Normalize ExternalScopeRoots in DetectionRulesDocsBuilderExtension to absolute paths - Replace static Directory.GetCurrentDirectory() with fileSystem.Directory.GetCurrentDirectory() in DeployUpdateRedirectsService - Replace new FileStream / static File.ReadAllTextAsync with scoped fileSystem equivalents in IncrementalDeployService - Fix StaticWebHost to pass _contentRoot to RealGitRootForPath for consistent scoping - Split AssemblerBuildService.BuildAll fs param into readFs/writeFs; update AssemblerCommands callers - Fix test paths in CodexNavigationTestBase to fall within WorkingDirectoryRoot/ApplicationData scopes - Make changelog test reportPath unique to prevent parallel-test file collisions Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * Fix build errors: AssemblerIndexService and MockFileSystem constructor - Pass fileSystem as both readFs/writeFs in AssemblerIndexService.Index() to match the updated BuildAll(readFs, writeFs) signature - Fix MockFileSystem construction in CodexNavigationTestBase: use constructor argument for currentDirectory instead of missing property Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * Fix FindGitRoot file-path fallback and split AssemblerIndexService read/write scopes - Paths.FindGitRoot: capture startDir before the loop so fallback returns always return a directory path, not the raw startPath which can be a file - AssemblerIndexService.Index: replace single fileSystem param with separate readFs/writeFs to avoid forwarding the read scope into write operations; update AssemblerIndexCommand to pass RealRead and RealWrite respectively Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * Restore unlimited-depth *.slnx anchor in DetermineWorkingDirectoryRoot The previous commit applied a depth guard to *.slnx in release builds, but this broke both authoring tests and the assembler integration tests: binaries run by Aspire and dotnet test start from the build output or project directory (4+ levels below the solution root), so restricting *.slnx to depth <= 1 caused WorkingDirectoryRoot to resolve to the build output directory instead of the repo root, making docs/ and all other repo paths fall outside the scoped filesystem roots. *.slnx is intentionally a depth-unlimited anchor for DetermineWorkingDirectoryRoot — it is the primary way to locate the solution root regardless of where the binary is launched from. The .git depth guard is kept unchanged. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * Fix docs-builder redirect tests (#3008) * Fix redirect tests * Remove changelog redirects implemented elsewhere * Search: Use default semantic_text inference, remove Jina mappings (#3014) Elasticsearch Serverless now defaults semantic_text to Jina, making the explicit Jina sub-fields redundant and the ELSER inference ID unnecessary. This removes both inference ID constants, all .jina field mappings, and lets semantic_text fields use the platform default. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: Update config/versions.yml eck 3.3.2 (#3019) Made with ❤️️ by updatecli Co-authored-by: elastic-observability-automation[bot] <180520183+elastic-observability-automation[bot]@users.noreply.github.com> * Deploy: Use write-scoped filesystem for apply command (#3021) The deploy apply command used RealRead which lacks AllowedSpecialFolder.Temp, causing ScopedFileSystemException when AwsS3SyncApplyStrategy stages files in /tmp/ for S3 upload. Switch to RealWrite which permits temp directory access. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Use ScopedFileSystem and inject interfaces. * Fix exception filtering --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Lisa Cawley <lcawley@elastic.co> Co-authored-by: Martijn Laarman <Mpdreamz@gmail.com> Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Jan Calanog <jan.calanog@elastic.co> Co-authored-by: elastic-observability-automation[bot] <180520183+elastic-observability-automation[bot]@users.noreply.github.com>
Fixes #3006
Summary
Implemented fix in
tests/authoring/Framework/Setup.fs:RedirectMockTargetsmoduledocs/_redirects.ymlwith YamlDotNet and collects local targets (same shapes as the real redirect model: scalars,to,many, empty →index.md, anchors-only →fromkey).docs/and the mock does not already have that path, adds a small stub markdown file (not a full copy of production content).Order of operations
_redirects.ymlfrom disk and runs injection before the generated TOC is written so stub paths are included in navigation like other files.Why stubs instead of
File.ReadAllTextindex.md, broke tests that rely on a synthetic index.cli/changelog/*.mdcontent also triggered navigation / processing failures (GetNavigationFor) and noisy diagnostics.Dependencies
YamlDotNettotests/authoring/authoring.fsproj(central versions inDirectory.Packages.props).Golden test
tests/authoring/Generator/LinkReferenceFile.fssolinks.jsonexpectations include the newcli/changelog/*.mdlink entries ({}), thecli/release/*→cli/changelog/*redirects from the current_redirects.yml, and redirect key order matching serialization (YAML order).Check:
dotnet run --project build -c release -- unit-test— passed (all projects including 304 authoring tests).Tests
I checked out #2980 and cherry-picked this fix this re-ran
dotnet run --project build -c release -- unit-test. It now completes successfully.Generative AI disclosure
Tool(s) and model(s) used: composer-2