fix: exclude nested bundle mount roots - #784
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //src:license-checkStatus: Click to expand output |
|
Documentation preview for this pull request is available at: |
05ae1e4 to
56d8ef9
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Explicit file-list mounts can still be discovered twice, violating the single-owner invariant.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds directory-based ownership boundaries to prevent duplicate Sphinx discovery for nested documentation bundles.
Changes:
- Excludes nested bundle roots from primary and parent directory walks.
- Adds nested-package regression coverage and documentation.
- Preserves explicit file-list mounting.
Critical issue: Explicit file-list mounts are omitted from ownership exclusions, allowing containing directory walks to rediscover their files and cause duplicate document names or Need IDs.
File summaries
| File | Description |
|---|---|
src/tests/docs_bzl/test_subdirectory_bundle.py |
Updates nested-bundle coverage. |
src/tests/docs_bzl/test_reference_integration.py |
Tests nested components. |
src/tests/docs_bzl/scenarios/subdirectory_bundle/producer/docs/embedded/content/index.rst |
Updates fixture content. |
src/tests/docs_bzl/scenarios/subdirectory_bundle/producer/docs/embedded/BUILD |
Defines the nested bundle. |
src/tests/docs_bzl/scenarios/subdirectory_bundle/producer/BUILD |
Relocates the embedded bundle fixture. |
src/tests/docs_bzl/scenarios/reference_integration/modern_module/docs/components/component/index.rst |
Updates modern component content. |
src/tests/docs_bzl/scenarios/reference_integration/modern_module/docs/components/component/BUILD |
Defines the modern component package. |
src/tests/docs_bzl/scenarios/reference_integration/modern_module/BUILD |
Mounts the modern component. |
src/tests/docs_bzl/scenarios/reference_integration/legacy_module/docs/components/component/index.rst |
Adds legacy component documentation. |
src/tests/docs_bzl/scenarios/reference_integration/legacy_module/docs/components/component/BUILD |
Defines the legacy component package. |
src/tests/docs_bzl/scenarios/reference_integration/legacy_module/BUILD |
Mounts the legacy component. |
src/tests/docs_bzl/scenarios/reference_integration/BUILD |
Describes the nested integration layout. |
src/extensions/score_mounts/tests/test_excludes.py |
Tests exclusion generation and serialization. |
src/extensions/score_mounts/__init__.py |
Implements structural exclusions, but omits file-list mounts from ownership processing. |
src/extensions/docs/mounts_internals.rst |
Documents internal exclusion behavior. |
docs/concepts/mounts/index.rst |
Documents source ownership boundaries. |
Review details
- Files reviewed: 15/16 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
56d8ef9 to
a492b20
Compare
| def _primary_mount_excludes( | ||
| source_dir: Path, | ||
| source_mounts: list[tuple[MountSpec, Path]], | ||
| ) -> list[str]: | ||
| """Return primary-walk exclusions for mounts below the app source root. | ||
|
|
||
| Without these patterns, Sphinx's normal walk of ``source_dir`` would also | ||
| discover documents that ``sphinx_mounts`` is about to register at the bundle's | ||
| ``mount_at`` location. The mounted directory must be the sole owner of those | ||
| documents. | ||
| """ | ||
| patterns: set[str] = set() | ||
| for _, mount_dir in source_mounts: | ||
| pattern = _nested_mount_pattern(source_dir, mount_dir) | ||
| if pattern is not None: | ||
| patterns.add(pattern) | ||
| return sorted(patterns) | ||
|
|
||
|
|
||
| def _nested_mount_excludes( | ||
| parent_index: int, | ||
| source_mounts: list[tuple[MountSpec, Path]], | ||
| ) -> list[str]: | ||
| """Return all descendant mount roots excluded from one directory mount. | ||
|
|
||
| The result includes direct and deeper descendants. Excluding every descendant | ||
| makes the ownership boundary independent of manifest order: each nested mount | ||
| receives its own documents, while the containing mount keeps the rest. | ||
| """ | ||
| _, parent_dir = source_mounts[parent_index] | ||
| patterns: set[str] = set() | ||
| for child_index, (_, child_dir) in enumerate(source_mounts): | ||
| if child_index == parent_index: | ||
| continue | ||
| pattern = _nested_mount_pattern(parent_dir, child_dir) | ||
| if pattern is not None: | ||
| patterns.add(pattern) | ||
| return sorted(patterns) |
There was a problem hiding this comment.
Wonder if this could be done in one, so we save some iterations here.
There was a problem hiding this comment.
Done. I combined the primary and nested exclusion calculation into one precomputation. The runtime mount loop now reuses the calculated results instead of rescanning the complete mount list for every parent.
| _make_mount_entry( | ||
| walk_dir, | ||
| spec, | ||
| tuple(_nested_mount_excludes(index, source_mounts)), |
There was a problem hiding this comment.
You only use this nested_mount_excludes here, so why not make the return type a tuple directly, instead of a list?
There was a problem hiding this comment.
Done. The per-mount exclusion results are now returned as tuples directly, so the runtime code no longer needs to convert the result with tuple(...).
MaximilianSoerenPollak
left a comment
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
My issues with the code have been addressed.
From my side this is alright now.
Why this is needed
When a directory-backed documentation bundle is mounted below another Sphinx source tree, the same documents can be discovered twice: once by the containing directory walk and once by the bundle mount. That creates duplicate document names and Sphinx-Needs IDs, and makes nested module/component layouts unreliable. The ownership boundary must be based on directories rather than the manifest's current file list so files created during live preview remain discoverable.
What this PR achieves
This fixes #781 by making each directory-mounted bundle own its physical source subtree while excluding nested bundle roots from containing walks. For directory mounts, every document is therefore discovered by exactly one Sphinx source: the primary tree or its owning bundle mount.
Changes
docs_bundle(srcs = [...])mounts in file-list mode.docsdirectory, covering both legacydataand modernexternal_needsAPIs.Known gap
Explicit
docs_bundle(srcs = [...])mounts are intended for generated documentation sources outside the primary source tree and remain in file-list mode. They are not included in the structural directory-exclusion calculation. If a workspace source file is explicitly mounted from below the primary source tree or another directory mount, it can still be discovered twice; handling that case with exact-file exclusions is left for a follow-up.The regression coverage verifies the directory exclusion rules directly and renders the nested reference integration, including the module-level and integration-level component paths. Existing subdirectory-bundle coverage continues to verify nested bundle composition without duplicate Needs.