Surface deprecated prebuilt detection rules as a sibling nav page#3167
Open
Surface deprecated prebuilt detection rules as a sibling nav page#3167
Conversation
- Auto-detects _deprecated/ subfolders in detection rule folders and generates
a sibling "Deprecated prebuilt detection rules" page in the nav
- Optional deprecated_file: field in docset.yml names a .md file whose content
prefixes the auto-generated deprecated rules listing
- Each deprecated rule page shows a deprecation warning with the deprecation date
- Fixed stateful TomlParser causing serve-mode parse failures on repeated builds
- Fixed serve --path scope: in-memory write FS now scoped to git root of --path
so output at {externalRepo}/.artifacts is within allowed write scope
- Fixed serve sourcePath: ReloadGeneratorService now passes DocumentationCheckoutDirectory
(git root) rather than DocumentationSourceDirectory (docs/ subfolder)
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
When a page is a hidden nav leaf (e.g. individual detection rule pages),
the JavaScript active-state logic couldn't find a matching nav link and
left the sidebar with nothing selected.
Fix: emit <meta name="docs:nav-active" content="{parentUrl}"> in the
page <head> when current.Hidden is true, pointing to the nearest visible
ancestor. pages-nav.ts reads this tag and uses that URL to mark the
correct nav item as current instead of window.location.pathname.
This works for both deprecated rules (/deprecated-detection-rules parent)
and active prebuilt rules (/ parent = detection rules overview at index.md).
Visible pages are unaffected (no meta tag emitted).
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds deprecated-detection-rules support across the docs pipeline: a scoped in-memory filesystem anchored to the repository root; TOC parsing and YAML converters that detect Sequence Diagram(s)sequenceDiagram
participant Builder as Build System
participant TOC as TOC Resolver
participant FS as InMemory FileSystem
participant Disk as Disk
participant Parser as Tomlyn Parser
participant Renderer as HTML Renderer
participant Browser as Browser/JS
Builder->>FS: InMemoryForSourceRoot(sourcePath)
Builder->>TOC: ResolveTableOfContents()
TOC->>Disk: Scan DetectionRuleFolders
Disk-->>TOC: entries (include _deprecated?)
alt _deprecated present
TOC->>TOC: Create DeprecatedSiblingRef (deprecated overview FileRef)
TOC->>Builder: Register deprecated overview (physical or synthetic)
end
Builder->>Parser: Parse .toml detection rule files
Parser-->>Builder: DetectionRule (includes Maturity, DeprecationDate)
Builder->>Renderer: Render pages (insert deprecation warning if Maturity=deprecated)
Renderer->>Renderer: Compute NavigationActiveUrl for hidden items
Renderer->>Browser: Emit docs:nav-active meta tag
Browser->>Browser: JS reads meta tag and marks nav item current
Suggested labelsfeature, docs, tests 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
nastasha-solomon
approved these changes
Apr 23, 2026
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Elastic.Documentation.Configuration/Toc/TableOfContentsYamlConverters.cs (1)
75-87:⚠️ Potential issue | 🟡 MinorRemove unused
deprecated_detection_rulesfrom parsing logic.The
deprecated_detection_ruleskey is parsed into the dictionary but never read anywhere in the codebase. The PR auto-discovers deprecated rules via folder scanning, making this key dead code. Remove it from the condition or document why it's retained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Elastic.Documentation.Configuration/Toc/TableOfContentsYamlConverters.cs` around lines 75 - 87, In the YAML parsing branch inside TableOfContentsYamlConverters (the else-if that checks key.Value against "detection_rules" or "exclude" or "deprecated_detection_rules"), remove "deprecated_detection_rules" from that condition and any related parsing branch so the parser no longer adds that key into the dictionary; if you intentionally want to keep it, add a comment explaining why and where it's consumed. Update or remove any tests or usages that expect deprecated_detection_rules to be present and ensure the parsing of the remaining keys ("detection_rules", "exclude") is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/Elastic.Documentation.Configuration/Toc/TableOfContentsYamlConverters.cs`:
- Around line 75-87: In the YAML parsing branch inside
TableOfContentsYamlConverters (the else-if that checks key.Value against
"detection_rules" or "exclude" or "deprecated_detection_rules"), remove
"deprecated_detection_rules" from that condition and any related parsing branch
so the parser no longer adds that key into the dictionary; if you intentionally
want to keep it, add a comment explaining why and where it's consumed. Update or
remove any tests or usages that expect deprecated_detection_rules to be present
and ensure the parsing of the remaining keys ("detection_rules", "exclude") is
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 81e46472-e23f-4459-8cba-6b14b17737ff
📒 Files selected for processing (15)
src/Elastic.Documentation.Configuration/FileSystemFactory.cssrc/Elastic.Documentation.Configuration/Toc/DetectionRules/DetectionRuleOverviewRef.cssrc/Elastic.Documentation.Configuration/Toc/DocumentationSetFile.cssrc/Elastic.Documentation.Configuration/Toc/TableOfContentsYamlConverters.cssrc/Elastic.Documentation.Site/Assets/pages-nav.tssrc/Elastic.Documentation.Site/Layout/_Head.cshtmlsrc/Elastic.Documentation.Site/_ViewModels.cssrc/Elastic.Markdown/Extensions/DetectionRules/DetectionRule.cssrc/Elastic.Markdown/Extensions/DetectionRules/DetectionRuleFile.cssrc/Elastic.Markdown/Extensions/DetectionRules/DetectionRulesDocsBuilderExtension.cssrc/Elastic.Markdown/HtmlWriter.cssrc/Elastic.Markdown/Page/Index.cshtmlsrc/Elastic.Markdown/Page/IndexViewModel.cssrc/tooling/docs-builder/Http/InMemoryBuildState.cssrc/tooling/docs-builder/Http/ReloadGeneratorService.cs
GHSA-g94r-2vxg-569j OpenTelemetry.Api 1.13.1 GHSA-mr8r-92fq-pj8p OpenTelemetry.Exporter.OpenTelemetryProtocol 1.13.1 GHSA-q834-8qmm-v933 OpenTelemetry.Exporter.OpenTelemetryProtocol 1.13.1 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
cotti
previously approved these changes
Apr 30, 2026
cotti
dismissed
their stale review
Saw an issue with TOML parsing unrelated to these changes, but still affecting parsing as a whole
Contributor
|
I sent #3214 as a possible fix - it works better as a PR for this branch. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Elastic.Markdown/Extensions/DetectionRules/DetectionRule.cs (1)
292-293: 💤 Low value
GetStringthrows unhelpful exception on missing keys.Direct indexing
table[key]throwsKeyNotFoundExceptionwithout context about which field is missing. For better debuggability in malformed TOML, consider wrapping with a clearer message.Optional improvement
-private static string GetString(TomlTable table, string key) => - (string)table[key]; +private static string GetString(TomlTable table, string key) => + table.TryGetValue(key, out var value) && value is string s + ? s + : throw new InvalidOperationException($"Required field '{key}' is missing or not a string");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Elastic.Markdown/Extensions/DetectionRules/DetectionRule.cs` around lines 292 - 293, The GetString helper currently indexes TomlTable directly causing a KeyNotFoundException with no context; update GetString (the private static method) to first check for the key (e.g. ContainsKey or TryGetValue) and if missing throw an ArgumentException or KeyNotFoundException with a clear message that includes the missing key name (and optionally the table name/context) and, when present, safely convert the value to string (with null checks) so malformed or null values produce a descriptive error instead of an unhelpful exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Elastic.Markdown/Extensions/DetectionRules/DetectionRule.cs`:
- Around line 281-290: ReadTactic currently indexes threatTable["tactic"] and
will throw if the key is missing; change it to use
threatTable.TryGetValue("tactic", out var t) and ensure t is a TomlTable before
casting, returning null (or a safe default) if the key is absent or not a
TomlTable; then call GetString(tacticTable, "...") only when tacticTable is
non-null so DetectionRuleTactic construction and GetString calls are guarded
against missing or malformed tactic entries.
---
Nitpick comments:
In `@src/Elastic.Markdown/Extensions/DetectionRules/DetectionRule.cs`:
- Around line 292-293: The GetString helper currently indexes TomlTable directly
causing a KeyNotFoundException with no context; update GetString (the private
static method) to first check for the key (e.g. ContainsKey or TryGetValue) and
if missing throw an ArgumentException or KeyNotFoundException with a clear
message that includes the missing key name (and optionally the table
name/context) and, when present, safely convert the value to string (with null
checks) so malformed or null values produce a descriptive error instead of an
unhelpful exception.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 87f9e368-6350-4e26-af0d-9198120ce3a6
📒 Files selected for processing (7)
Directory.Packages.propssrc/Elastic.Documentation.Configuration/Elastic.Documentation.Configuration.csprojsrc/Elastic.Documentation.Configuration/Toc/DocumentationSetFile.cssrc/Elastic.Markdown/Elastic.Markdown.csprojsrc/Elastic.Markdown/Extensions/DetectionRules/DetectionRule.cssrc/Elastic.Markdown/HtmlWriter.cstests/Elastic.Markdown.Tests/DetectionRules/DetectionRuleParsingTests.cs
✅ Files skipped from review due to trivial changes (1)
- src/Elastic.Documentation.Configuration/Elastic.Documentation.Configuration.csproj
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Elastic.Markdown/HtmlWriter.cs
cotti
approved these changes
Apr 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Elastic's prebuilt detection rules repo contains a
_deprecated/folder of rules that have been retired. These rules were previously invisible to users browsing the docs — there was no way to discover them, understand why they exist, or know what replaced them. Users navigating directly to a deprecated rule page (e.g. from a bookmark or external link) also lost their nav context entirely: nothing in the sidebar was highlighted.What
Deprecated rules page
When the
detection-rulesplugin processes a rule folder that contains a_deprecated/subfolder, it automatically generates a sibling "Deprecated prebuilt detection rules" page in the navigation alongside the active rules overview. No docset.yml changes are required for the page to appear.The page lists all deprecated rules alphabetically by domain, linking to individual rule pages. Each individual rule page shows a deprecation warning admonition with the date the rule was deprecated.
Optional preface
The security team can supply introductory content for the deprecated rules page by adding a markdown file to the detection-rules docs folder and referencing it in
docset.yml:When
deprecated_fileis set, the content of that file prefixes the auto-generated rules listing. This lets the team explain what deprecated rules are, what users should do with them, and link to related guidance — all without touching the docs-builder codebase.Follow-up: elastic/detection-rules#5968 adds this preface file (draft, cannot merge until this PR is released to production).
Nav active state for hidden pages
Individual rule pages (both active and deprecated) are hidden nav leaves — they exist in the nav tree for ordering/breadcrumb purposes but don't appear as visible links in the sidebar. Before this change, landing on a rule page left the sidebar with nothing selected.
The fix emits a
<meta name="docs:nav-active" content="{parentUrl}">tag server-side when the current page is a hidden nav item. The client reads this and highlights the nearest visible ancestor instead. This restores nav context for rule pages and fixes the same issue on the live site for any hidden page.Verification
Run locally against the detection-rules repo:
http://localhost:3000/deprecated-detection-rules— lists deprecated ruleshttp://localhost:3000/rules/_deprecated/<rule>— deprecation warning, sidebar highlights deprecated rules pagehttp://localhost:3000/rules/<category>/<rule>— sidebar highlights prebuilt rules overview