Let a TOC entry source its content from outside the docset root - #4020
Let a TOC entry source its content from outside the docset root#4020clintandrewhall wants to merge 3 commits into
Conversation
A `source:` key on a `file:` or `hidden:` entry points at a markdown file elsewhere in the repository, so a page can live next to the code it documents without moving the documentation set root. `file:` stays the virtual position that drives the URL, navigation, output path and link reference; `source:` only says which file to read. Relative links, images and includes inside a sourced page resolve from that virtual position, so a page cannot smuggle `../` segments into generated URLs or write output outside the output directory. The "Edit this page" link now derives from the file actually read rather than from the position in the set, which also corrects it for detection rule pages. Closes #3798 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…straint `SourceFullPath` is derived and absolute, so serializing a table of contents would leak a machine path into generated YAML. Assembled builds clone a repository with a partial checkout of `docs` only, so a `source:` above that path needs `sparse_paths` or `checkout_strategy` widened for that repository. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Docs preview (local build)Handbook preview: https://docs-v3-preview.elastic.dev/elastic/docs-builder/pull/4020/ Sample changed pages: |
There was a problem hiding this comment.
🟡 Changes recommended
Virtual-path traversal, symlink escape, root-index validation, and generated-page collisions remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds off-root Markdown sourcing while preserving each page’s virtual docset position.
Changes:
- Adds
source:parsing, validation, and registration. - Separates virtual paths from physical content paths.
- Updates documentation and test coverage.
File summaries
| File | Description |
|---|---|
tests/Elastic.Markdown.Tests/DocSet/ExternalSourceTests.cs |
Tests rendering and registration. |
tests/Elastic.Documentation.Configuration.Tests/TocSourceTests.cs |
Tests TOC parsing and resolution. |
src/Elastic.Markdown/Myst/MarkdownParser.cs |
Supports virtual parse paths. |
src/Elastic.Markdown/IO/MarkdownFileFactory.cs |
Registers sourced files. |
src/Elastic.Markdown/IO/MarkdownFile.cs |
Accepts virtual relative paths. |
src/Elastic.Markdown/IO/ExternallySourcedMarkdownFile.cs |
Models sourced Markdown pages. |
src/Elastic.Markdown/IO/DocumentationFile.cs |
Separates position from source. |
src/Elastic.Markdown/HtmlWriter.cs |
Corrects edit links. |
src/Elastic.Documentation/Extensions/IFileInfoExtensions.cs |
Adds containment checking. |
src/Elastic.Documentation.Navigation/Isolated/Node/DocumentationSetNavigation.cs |
Improves sourced-file diagnostics. |
src/Elastic.Documentation.Configuration/Toc/TableOfContentsYamlConverters.cs |
Parses source: entries. |
src/Elastic.Documentation.Configuration/Toc/TableOfContentsItems.cs |
Stores source paths. |
src/Elastic.Documentation.Configuration/Toc/DocumentationSetFile.cs |
Resolves and collects sources. |
docs/documentation/isolated/navigation.md |
Documents navigation usage. |
docs/documentation/isolated/configure/index.md |
Documents configuration rules. |
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| var sourceFile = context.ReadFileSystem.FileInfo.New(fileRef.SourceFullPath!); | ||
| if (!sourceFile.Exists) | ||
| { | ||
| context.Collector.EmitError(fileRef.Context, $"'source: {fileRef.Source}' does not exist."); | ||
| continue; | ||
| } |
| var virtualRelativePath = fileRef.PathRelativeToDocumentationSet.OptionalWindowsReplace(); | ||
| var file = new ExternallySourcedMarkdownFile(sourceFile, virtualRelativePath, _markdownParser, context); |
| { | ||
| if (fileOnly == "index.md") | ||
| return new IndexFileRef(fileOnly, fileOnly, false, children, placeholderContext); | ||
| return new IndexFileRef(fileOnly, fileOnly, false, children, placeholderContext) { Source = contentSource }; |
| Files = files | ||
| .Concat(additionalSources) | ||
| .Concat(externallySourced) | ||
| .Where(t => t.Item2 is not ExcludedFile) | ||
| .ToDictionary(kv => new FilePath(kv.Item1, context.DocumentationSourceDirectory), kv => kv.Item2) |
| private Task<MarkdownDocument> ParseFromFile( | ||
| IFileInfo path, | ||
| IFileInfo contentFile, | ||
| YamlFrontMatter? matter, | ||
| MarkdownPipeline pipeline, |
`TocSourceTests` compared a mock filesystem path against a real `Path.GetFullPath`, which resolves on whichever drive the tests run from. Windows saw `C:\repo\...` against `D:\repo\...`. Assertions now compute their expected paths from the same filesystem the resolution used. Four gaps in registering a sourced page: A symlink inside the checkout passes the lexical containment check while resolving outside it. `SymlinkValidator.ValidateFileAccess` now guards the source and its ancestors, matching what the ordinary file scan and the control files already do. `file:` is not itself checked for containment, so a virtual path could carry `../` into URL generation and write output outside the output directory. A sourced entry whose position escapes the documentation set root is now refused. Collision detection compared the virtual path against files on disk and earlier `source:` entries, but not against pages an extension generates — those exist nowhere on disk, so a clash reached `ToDictionary` and threw. Sourced pages are now registered last and checked against every position already taken. `ValidateRootIndexExists` only stats `<docset>/index.md`, so a non-public docset whose root index is sourced always failed. It now accepts the registered position too. `ParseFromFile` groups its pipeline and validation flag into a `ParseMode`, which keeps it within the parameter limit and stops a caller pairing the minimal pipeline with full validation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
That risk section is spot on and exactly why I am weary of this change. We also do redirect detection for removed/renamed files. This kind of breaks our contract that the url is the file. There are quite a few assumptions around that first principle. The system is more designed around docset/toc.yml as the composable building blocks from which you can re-arange/assemble your final site. Codex is much simpler it's fixed url pattern. But the system has to work for both. As I understand it packages want their docs to live in the package repository? I wonder if we should allow packages to define their own toc.yml files and the dev-docs can then reference those instead to compose it's overal nav? Codex should be doing sparse clones, especially for Kibana. Although it's been forever since I benchmarked that vs shallow clones, @reakaleek can you remember? We still need a way to ensure/declare this is a codex only thing? Codex docsets.yml have to set registry: key so we can determine if a doc set is intended for codex publishing. |
|
@Mpdreamz I would certainly defer to your expertise and preference here. I was putting forward a "what about this?" agent-assisted PR to spur the conversation. But yes: the purpose of this is have a docset that can reach out and collect documents from other places. In this case, You could opt for |
|
No worries about submitting the PR, super helpful indeed to spur the conversation: working code > issue but in this case both are great :) If that works for y'all too, let's explore what toc.yml in packages looks like to unlock package docs collocation. |
|
Lemme see if I can revise this PR, but I may close it (and the issue) in favor of another. Stay tuned! |
|
Closing in favour of the package-hosted Why the What a package-hosted Codex is not doing sparse clones today. One correction to this PR's description. It claimed the edit-link change also fixed detection rule pages. It does not. Issue #3798 stays open and is being reframed around this direction — the problem is unchanged, only the proposed mechanism moves. |
A
source:key on afile:orhidden:table of contents entry reads the page's content from a markdown file elsewhere in the repository. A page can now live next to the code it documents without moving the documentation set root.Affects:
Navigation,Authoring,ConfigurationPrompt summary: Implement #3798. Kibana publishes internal developer docs to Codex from a
docs-dev/docset while the packages those docs describe live undersrc/platform/, and the maintainers want each page checked in beside the code it documents so a component change and its documentation land in the same review. The ask is to let a table of contents entry keep its docset-relative position while sourcing content from above the root.Why
All table of contents paths resolve relative to the documentation set root, and a repository supports one docset per registry. Colocating a page with its code therefore means moving the docset root inside a package directory, which concedes the repository to a single internal docset. Keeping the docset where it is means the documentation cannot sit beside the code.
What
The new
source:keyAn entry keeps
file:as its position in the documentation set and gainssource:for where the content is read:file:still drives the URL, the navigation entry, the output path and the cross-repository link reference.source:is resolved against the directory holding thedocset.ymlortoc.ymlthat declares the entry, and nothing else. It works on ahidden:entry and on thefolder:+file:form.Separating position from content
DocumentationFile.RelativePathis the page's position in the set and no longer has to equalSourceFilerelative to the root. A sourced page parses as though it lived at that position, so relative links, images and includes inside it resolve from the virtual path rather than from where the file sits on disk. Because generated URLs and output paths derive from a path that cannot contain../, no page can write outside the output directory or emit../in a URL.Registering a page the file scan never sees
The scan walks the documentation set root, so a sourced page has to be registered from the table of contents.
MarkdownFileFactorykeys it under its virtual path — the same path navigation resolves against — and registers it after everything else so it can be refused with a diagnostic when:file:position escapes the root, or is already held by a scanned file, an extension-generated page, or another sourced entryA non-public docset can also declare its root
index.mdwithsource:; the root-index requirement accepts the registered position.The edit link
The "Edit this page" link now derives from the file that was actually read instead of joining the docset directory with the position in the set. That is required for a sourced page, and it also corrects detection rule pages, whose
.tomlsource produced a path with../segments in it.ResolveFileRefrebuilds refs withwithResolution previously reconstructed each ref through a three-arm
newswitch, which silently drops any key added toFileReflater. Awithexpression preserves the concrete ref type and every off-path key, sosource:cannot be lost between parsing and navigation.Verify
Then try it against this repository's own docs, pointing an entry at a file above
docs/:Out of scope: single markdown files only. There is no
source:onfolder:,listing:ortoc:, and no glob form, per the issue. A sourced entry'sfile:is now checked for containment, but the general case the issue describes under Alternative Solutions — a bare- file: ../../x.mdwith nosource:— is still accepted with no check. The author offered to track that separately and this PR leaves it alone.Risk: assembled builds clone a repository with a partial checkout of
docsonly. Asource:above that path needs the repository'ssparse_pathsorcheckout_strategywidened inconfig/assembler.yml, the same requirement the detection rules extension already has. Codex checkouts are not sparse, so the case in the issue works without a config change.