Feature/fix docs nav hierarchy#597
Conversation
Three small UI fixes on the Sphinx docs site (`pytorch_sphinx_theme2`): - Cookie banner: paint a visible `×` on the theme's empty `<button class="close-button">` via CSS. The theme's bundled `theme.js` already wires the click handler through jQuery, but the button ships with a transparent background and a border that matches the banner background — so users had no visible affordance to close it. Also add a tiny vanilla-JS fallback in `_static/cookie-banner.js` with a namespaced `localStorage` key so the banner stays dismissed even if jQuery / theme.js fail to load. - Version switcher: drop `version-switcher` from `navbar_start` in `conf.py` and hide `.version-switcher__container` via CSS. With only `main` in `versions.json` the dropdown opens to an empty list, and the theme renders it in both the top navbar (desktop) and the mobile primary sidebar, so both placements needed to go. - Secondary sidebar chrome: override `library_links`, `community_links`, and `language_bindings_links` in `html_context` to empty lists. The theme's default pulls PyTorch-wide link collections (ExecuTorch, torchtitan, torchao, etc.) into the OpenEnv sidebar, which is noise for readers of this project. The remaining UI complaint — the flat top-nav hierarchy where individual pages are hoisted instead of toctree captions, including a duplicate "Quick Start" entry — is out of scope here and will be addressed in a follow-up PR once the right fix (theme config vs toctree restructure) is decided. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `pytorch_sphinx_theme2` top nav promotes every toctree entry from the root document (index.md) into a flat top-level item. Our root had ~25 entries across the Get Started / Guides / Tutorials / Environments / API Reference / Community captions, which produced a disorienting top bar like: Quick Start · Installation · Core Concepts · Quick Start (duplicate) · Guides · Auto-Discovery · Connecting to Servers · More… · Tutorials · Environments · API Reference Two underlying issues: 1. Every child doc was listed directly on the root toctree instead of under its section hub. 2. `getting_started/README.rst` (Sphinx Gallery landing) was titled "Quick Start", colliding with `quickstart.md`. Fix: - Reduce each root toctree to a single hub entry (captions unchanged: Get Started, Guides, Tutorials, Environments, API Reference, Community). Children are now pulled from each hub's own toctree, so the theme renders one dropdown per caption. - Add toctrees to `guides/index.md`, `reference/index.md`, and `contributing.md` so their children appear under the right dropdown. - Pull `auto_getting_started/index` into `tutorials/index.md`'s toctree — interactive tutorials belong under Tutorials, not as a duplicate top-level item. - Rename `getting_started/README.rst` title from "Quick Start" to "Interactive Tutorials" to remove the label collision. - Bump `header_links_before_dropdown` from 7 to 8 so the last hub (Community) stays visible instead of collapsing under "More". Result: 8 top-level items (3 leaves + 5 dropdown hubs), every doc reachable in at most two clicks, no duplicate labels. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
meta-cla
Bot
added
the
CLA Signed
Greptile SummaryThis docs-only PR fixes the flat top-nav produced by Confidence Score: 5/5Safe to merge — docs-only restructure with no Python runtime changes. All findings are P2 style/cleanup items. No Python logic is changed, and the PR author reports a clean make clean html build. docs/source/contributing.md — verify no Sphinx duplicate-toctree warning for contributing-envs on a fresh build. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
ROOT["index.md (root)"]
ROOT -->|"Get Started"| GS["quickstart / installation / concepts"]
ROOT -->|"Guides"| GHUB["guides/index.md"]
ROOT -->|"Tutorials"| THUB["tutorials/index.md"]
ROOT -->|"Environments"| ENV["environments"]
ROOT -->|"API Reference"| RHUB["reference/index.md"]
ROOT -->|"Community"| CHUB["contributing.md"]
GHUB --> GC["auto-discovery · connecting · async-sync\nsimulation-vs-production · mcp-lifecycle\nfirst-environment · anatomy · deployment\nrl-integration · rewards · customizing-web-ui"]
THUB --> TC["auto_getting_started/index\nopenenv-tutorial · wordle-grpo · rl-training-2048"]
RHUB --> RC["core · cli · auto_discovery"]
CHUB --> CC["auto_getting_started/contributing-envs"]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| :hidden: | ||
| :maxdepth: 1 | ||
|
|
||
| auto_getting_started/contributing-envs |
There was a problem hiding this comment.
contributing-envs may end up in two toctrees
auto_getting_started/contributing-envs is also listed in the toctree inside docs/source/getting_started/README.rst (line 62), which sphinx-gallery converts into auto_getting_started/index. The remove_orphan_and_duplicate_toctree hook in conf.py only strips the sphinx-gallery-generated hidden .. toctree:: whose entries match plot_\d+_\w+; it leaves the explicit toctree from README.rst (caption "Quick Start") untouched. Once the :orphan: flag is removed from auto_getting_started/index, Sphinx may emit a duplicate-toctree warning for contributing-envs. It is worth verifying no new warnings appear on a fresh make clean html run, and if they do, the README.rst toctree entry for contributing-envs should be removed there.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/source/contributing.md
Line: 84
Comment:
**`contributing-envs` may end up in two toctrees**
`auto_getting_started/contributing-envs` is also listed in the toctree inside `docs/source/getting_started/README.rst` (line 62), which sphinx-gallery converts into `auto_getting_started/index`. The `remove_orphan_and_duplicate_toctree` hook in `conf.py` only strips the sphinx-gallery-generated *hidden* `.. toctree::` whose entries match `plot_\d+_\w+`; it leaves the explicit toctree from `README.rst` (caption "Quick Start") untouched. Once the `:orphan:` flag is removed from `auto_getting_started/index`, Sphinx may emit a duplicate-toctree warning for `contributing-envs`. It is worth verifying no new warnings appear on a fresh `make clean html` run, and if they do, the `README.rst` toctree entry for `contributing-envs` should be removed there.
How can I resolve this? If you propose a fix, please make it concise.Greptile flagged that `auto_getting_started/contributing-envs` now lives in two toctrees: the explicit one in `docs/source/getting_started/README.rst` (sphinx-gallery landing) and the one I added to `docs/source/contributing.md`. A fresh `make clean html` did not emit a duplicate-toctree warning, but the doc still has two navigation parents, which is confusing. Canonical home for "how to contribute a new environment" is the Community section, so drop the entry from the gallery's README.rst. The gallery still lists `environment-builder` (a legitimate tutorial for new contributors building envs). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
PR #597 — Fix docs nav hierarchy
- Pure documentation restructuring: toctree hierarchy, Sphinx
conf.py, one CSS file, one small JS file for cookie banner persistence. No runtime Python touched. - Lint (usort + ruff) passes cleanly on
src/andtests/. Debug-code findings flagged by hooks are all pre-existing and unrelated to this PR. - No alignment concerns: zero impact on API boundaries, client-server separation, agent isolation, or reward/reset invariants.
- The approach is sound — moving child entries from the root
index.mdtoctrees into their respective hubindex.mdfiles is the correct Sphinx pattern for collapsing a flat top-nav into per-section dropdowns. Theheader_links_before_dropdown: 8bump and removal of the version-switcher fromnavbar_startare both well-justified in the PR description.
One non-blocking note: docs/source/_static/cookie-banner.js uses a bare catch (_) {} that silently swallows all errors on the localStorage.setItem call, not just SecurityError. Acceptable for a UI nicety but worth a comment if anyone extends this file later.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report — PR #597: Feature/fix docs nav hierarchy
Automated Checks
- Lint: PASS —
conf.pyis the only Python file changed; ruff reports no issues. All other changed files are RST, Markdown, JS, and CSS. - Debug code: CLEAN — no debug artifacts introduced in this PR.
Tier 1: Fixes Required
No mechanical issues identified. The complete toctree coverage audit (27 pages traced) confirms zero pages are orphaned or unreachable after the restructure.
Tier 2: Alignment Discussion
None identified. This PR touches only Sphinx configuration and documentation source; no API, type, security, or architectural invariants are involved.
Detailed Review
Toctree Coverage — PASS
Every page reachable before the PR remains reachable after. The mapping is:
| Page | Pre-PR parent toctree | Post-PR parent toctree |
|---|---|---|
quickstart, installation, concepts |
root Get Started | root Get Started (unchanged) |
auto_getting_started/index |
root Get Started | tutorials/index.md (moved to Tutorials) |
guides/* (11 pages) |
root Guides (flat) | guides/index.md toctree |
simulation-vs-production, mcp-environment-lifecycle, customizing-web-ui |
root Guides (flat) | guides/index.md (as ../page) |
tutorials/openenv-tutorial, wordle-grpo, rl-training-2048 |
tutorials/index.md |
tutorials/index.md (unchanged) |
environments |
root Environments | root Environments (unchanged) |
core, cli, auto_discovery |
root API Reference (flat) | reference/index.md (as ../page) |
contributing |
root Community | root Community (unchanged) |
auto_getting_started/contributing-envs |
root Community (flat) | contributing.md toctree |
Cross-directory path resolution — PASS
All ../-prefixed toctree entries resolve correctly relative to their hub document:
guides/index.md(docs/source/guides/) references../simulation-vs-production,../mcp-environment-lifecycle,../customizing-web-ui— all resolve todocs/source/*.md.reference/index.md(docs/source/reference/) references../core,../cli,../auto_discovery— all resolve todocs/source/*.md.tutorials/index.md(docs/source/tutorials/) references../auto_getting_started/index— resolves to the Sphinx Gallery output directory.
Sphinx Gallery integration — PASS
The contributing-envs.md entry in contributing.md's toctree references auto_getting_started/contributing-envs. This is a generated path: conf.py's copy_md_pages_to_gallery function (priority 900) copies all getting_started/*.md files into auto_getting_started/ before Sphinx processes documents, so the file exists at build time. The exclude_patterns for getting_started/*.md and getting_started/README.rst prevents double-processing. The remove_orphan_and_duplicate_toctree hook correctly strips the :orphan: directive and the gallery-generated hidden toctree from auto_getting_started/index once tutorials/index.md provides a parent.
The last commit (dd0a1db) correctly resolves the dual-parent issue for contributing-envs by removing it from README.rst's toctree, leaving contributing.md as the sole toctree parent. This matches the canonical home described in the commit message.
Label collision fix — PASS
docs/source/getting_started/README.rst title changed from "Quick Start" to "Interactive Tutorials". This eliminates the collision with quickstart.md's nav label, which was producing a confusing duplicate in the rendered top bar.
conf.py changes — PASS, with one minor note
header_links_before_dropdown: 8— correct; the restructured root produces exactly 8 items (3 leaves: Quick Start, Installation, Core Concepts; 5 dropdowns: Guides, Tutorials, Environments, API Reference, Contributing to OpenEnv).navbar_startremovingversion-switcher— intentional and documented;openenv-overrides.cssadds belt-and-suspenders CSS coverage for the mobile sidebar placement, and_static/versions.jsonconfirms onlymainexists.library_links,community_links,language_bindings_linkshardcoded to[]— intentional; suppresses PyTorch-ecosystem sidebar blocks irrelevant to OpenEnv. The comment in the diff explains the rationale.
Minor note (not a blocker): tutorials/index.md's toctree retains maxdepth: 2 (unchanged from pre-PR). Adding ../auto_getting_started/index under that toctree means the gallery's sub-pages (plot_01_introduction_quickstart, plot_02_using_environments, etc.) will render one level deep in the sidebar under Tutorials. This is cosmetically more expanded than the hub pages but is consistent with the PR's goal of surfacing the gallery content under the Tutorials section. Worth monitoring in a rendered preview if the sidebar feels over-populated.
Hub-page pattern consistency — PASS
The three new hub toctrees added to guides/index.md, reference/index.md, and contributing.md all use :hidden: and :maxdepth: 1, consistent with each other and with the root index pattern. The one exception is tutorials/index.md which uses :maxdepth: 2, but that predates this PR and is unchanged.
Static assets — PASS
cookie-banner.js is well-scoped (IIFE wrapper), handles localStorage unavailability (private mode) with a try/catch, and degrades gracefully. openenv-overrides.css uses standard CSS custom properties (--pst-color-*) from the theme's design system and :focus-visible for the close button (appropriate for a documentation site targeting modern browsers).
Summary
- 0 mechanical issues introduced by this PR.
- 0 alignment flags — docs-only restructure, no runtime code affected.
- The toctree flattening is well-executed: every previously reachable page remains reachable, cross-directory paths are correct, the gallery integration is sound, and the label collision is cleanly resolved.
The one item worth a second look before merging is the maxdepth: 2 on tutorials/index.md now that auto_getting_started/index (with its own sub-pages) lives under it — verify the rendered sidebar is not over-expanded.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: PASS - No Python source files changed; JS/CSS/RST/MD changes are outside ruff scope.
- Debug code: CLEAN - No debug artifacts introduced by this PR.
Tier 1: Fixes Required
-
docs/source/guides/index.md(toctree entries) - The toctree added toguides/index.mdreferences ten files (auto-discovery,connecting,async-sync,../simulation-vs-production,../mcp-environment-lifecycle,first-environment,environment-anatomy,deployment,rl-integration,rewards,../customizing-web-ui) that are not created anywhere in this diff and do not exist in the current Sphinxdocs/source/tree. These dangling toctree entries will cause Sphinx to emit warnings and the nav sidebar to show broken entries. Either the content files need to land in the same PR, or these entries must be removed until the files exist. -
docs/source/reference/index.md(toctree entries) - References../core,../cli, and../auto_discovery. The filescli.mdandauto_discovery.mddo not appear in the Sphinxdocs/source/layer (they exist only in the MkDocsdocs/tree). Confirm these files are present or will be generated before this PR merges. -
docs/source/contributing.md- The toctree appended at the bottom referencesauto_getting_started/contributing-envs. Verify this generated file (auto_getting_started/contributing-envs) is produced by Sphinx Gallery before wiring it into the nav; the gallery build must run first.
Tier 2: Alignment Discussion
None identified. This is a docs-only change that does not touch environment APIs, agent interfaces, reward computation, or client-server boundaries. No OpenEnv invariants or design principles are implicated.
Summary
- 3 mechanical issues to fix (dangling toctree references that will cause Sphinx build warnings or failures)
- 0 alignment points for human review
The nav restructuring intent is sound - moving child toctree entries from index.md into each section's own index.md is the correct Sphinx pattern. The only problem is that the referenced content files are not present in this diff.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: PASS — no Python files outside
docs/source/conf.pywere modified;conf.pychanges are Sphinx config only (no import-sortable or ruff-applicable patterns). - Debug code: CLEAN — no debug statements introduced by this PR. Pre-existing findings in
src/openenv/cli/andsrc/openenv/core/are unrelated.
Tier 1: Fixes Required
None. The diff is docs-only; no Python runtime code is touched.
Path verification: Every path removed from docs/source/index.md is re-added to the appropriate hub toctree (guides/index.md, reference/index.md, contributing.md, tutorials/index.md). The delegation is symmetric and complete. The auto_getting_started/contributing-envs entry is correctly moved: removed from getting_started/README.rst and added to contributing.md. The auto_getting_started/index entry is correctly moved from the root toctree into tutorials/index.md.
conf.py changes: html_css_files/html_js_files additions are valid Sphinx config keys. header_links_before_dropdown bump from 7 to 8 matches the stated 8 top-level items. Removing version-switcher from navbar_start is consistent with the CSS rule in openenv-overrides.css. Hardcoding library_links/community_links/language_bindings_links to [] removes PyTorch-wide sidebar links — appropriate for an independent project.
Tier 2: Alignment Discussion
None identified. This PR does not touch Python runtime code, API contracts, environment isolation, MCP tooling, or any system invariant defined in INVARIANTS.md.
Summary
- 0 mechanical issues to fix
- 0 alignment points for human review
The restructuring is internally consistent and well-described. Approving.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review
Automated Checks
- Lint: No runtime Python touched; only
docs/source/conf.py(Sphinx config) among.pyfiles. Pre-existing lint noise inenvs/carla_env/is unrelated. - Debug code: No debug artifacts introduced. Pre-existing findings in
src/openenv/cli/predate this change.
Tier 1: Fixes Required
None. The toctree delegation is symmetric and complete:
- Every entry removed from
docs/source/index.mdis re-added to the corresponding hub toctree (guides/index.md,reference/index.md,tutorials/index.md,contributing.md). auto_getting_started/contributing-envsis correctly removed fromgetting_started/README.rstand given a single parent incontributing.md, eliminating a potential duplicate-toctree warning.- Cross-directory
../-prefixed paths resolve correctly relative to their hub documents. conf.pyadditions (html_css_files,html_js_files,header_links_before_dropdown: 8) are valid and consistent with the stated 8 top-level nav items.
Tier 2: Alignment Discussion
None identified. Sphinx configuration + Markdown/RST + two static assets. No API contracts, agent isolation boundaries, reward computation, or client-server separation implicated.
Notes (non-blocking)
cookie-banner.jsuses a barecatch (_) {}onlocalStorage.setItem— acceptable for a UI nicety.tutorials/index.mdretainsmaxdepth: 2(pre-existing). Sidebar may render more expanded than other hubs now thatauto_getting_startedis nested there. Worth a quick visual check but not a blocker.
Summary
- 0 mechanical issues introduced.
- 0 alignment flags.
- The nav restructuring is well-executed.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review
Automated Checks
- Lint: FAIL —
ruff F601duplicate dict key inhtml_theme_options - Debug code: CLEAN (for this PR's files)
Tier 1: Fix Required
docs/source/conf.py — duplicate key "navbar_center" (ruff F601)
Line 113 (pre-existing) sets:
"navbar_center": "navbar-nav",Line 123 (added by this PR) sets:
"navbar_center": ["navbar-nav"],Both are in the same html_theme_options dict literal. Python silently uses the second value, but ruff flags this as F601 (repeated dict key). Fix: remove line 113. The list form on line 123 is the correct format for pytorch_sphinx_theme2.
File-existence Spot-Check
All toctree entries introduced by the PR were verified against the checked-out branch — every referenced file exists or is generated at build time by copy_md_pages_to_gallery / sphinx_gallery. No broken links found.
Tier 2: Alignment
None. This is a docs-only restructure; no invariants from INVARIANTS.md are touched.
Summary
- 1 mechanical issue to fix before merge
- 0 alignment concerns
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: FAIL (pre-existing violations in
envs/carla_env/— zero files touched by this PR trigger them) - Debug code: CLEAN for this PR's changes (pre-existing
printstatements insrc/openenv/cli/)
Tier 1: Fixes Required
None introduced by this PR.
Tier 2: Alignment Discussion
None identified. Pure documentation navigation change — no impact on API signatures, client-server separation, agent isolation, reward placement, or WebSocket/MCP boundaries.
Notes
- Hub-and-spoke toctree restructure is correct: 25 flat root entries collapsed to 8 logical top-nav items, all pages still reachable in at most two clicks.
conf.pycleanup (removing PyTorchlibrary_links/community_links/language_bindings_linkssidebar blocks) is the right call for an OpenEnv-specific docs site.- Cookie-banner JS is localStorage-safe, IIFE-scoped, and handles private-mode gracefully.
auto_getting_started/toctree references are Sphinx Gallery build artifacts — correct to omit from source tree.header_links_before_dropdown: 8matches the new hub count exactly.
Approve.
Automated review by Claude Code | Learn more
Pre-existing duplicate: line 113 set `"navbar_center": "navbar-nav"` and the PR's earlier commit re-set the same key as a list ten lines below. Python silently kept the second value; ruff flags this as F601. Keep the list form (the shape pytorch_sphinx_theme2 expects) and remove the first. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Docs nav hierarchy fix is internally correct for the Sphinx source tree, but the PR targets a branch where that tree is not deployed.
Tier 1: Fixes Required
-
docs/source/does not exist onmainand is not built or deployed by the repo's CI. Thedocs.ymlworkflow exclusively runsmkdocs build --config-file docs/mkdocs.yml, which reads fromdocs/(MkDocs tree). The Sphinxdocs/source/tree was reverted frommainin commit3e99d68e("Revert PR #401: Sphinx docs changes (not ready for merge)"). This PR'sbaseRefNameismain, so merging it as-is adds dead files — the nav changes will have zero effect on the published site. The PR should either (a) target the Sphinx feature branch (likelysphinxv3) that carriesdocs/source/, or (b) apply equivalent nav fixes todocs/mkdocs.yml. -
docs/source/conf.py— pre-existing bug that this PR incidentally exposes but does not fix: the base file has"navbar_center"defined twice in thehtml_theme_optionsdict. The PR removes only the first (string) occurrence and keeps the second (list). This is a net improvement, but the fix is inadvertent and the commit description does not mention it. Worth noting explicitly in the PR so reviewers know it's intentional.
Tier 2: Alignment Discussion
ALIGNMENT FLAG: Two parallel documentation systems with no merge strategy
- Principle at stake: "Be hands-on — provide ready-to-use implementations, not just specs" (PRINCIPLES.md); documentation published to users should reflect the actual state of the repository.
- The concern: The repo maintains two documentation systems:
docs/mkdocs.yml(deployed to GitHub Pages viadocs.yml) anddocs/source/conf.py(Sphinx, reverted from main, living only on thesphinxv3branch). This PR fixes the Sphinx tree but leaves the MkDocs tree (docs/mkdocs.yml) unchanged. Users reading the published site see none of these improvements. There is no documented policy on which system is canonical or when/whether Sphinx will replace MkDocs on main. - Suggested reviewer: @burtenshaw
Internal correctness of the Sphinx changes (if targeting the right branch)
The path logic inside docs/source/ is sound:
- All 11 guide children removed from
index.md's Guides toctree are re-declared indocs/source/guides/index.md's new toctree with correct relative paths. reference/index.md's new toctree correctly resolves to the source root from insidereference/.contributing.mdreferencingauto_getting_started/contributing-envsis correct.tutorials/index.mdadding../auto_getting_started/indexis correct from insidetutorials/.- Moving
auto_getting_started/indexfrom Get Started to Tutorials is semantically correct. - The
header_links_before_dropdown: 8bump is correct: 3 leaf entries + 5 hub entries = 8. - The
navbar_centerduplicate-key bug (pre-existing) is resolved as a side effect. - CSS and JS additions are clean and well-commented.
Recommendation: Re-target this PR against the sphinxv3 branch (or whichever branch holds the active Sphinx docs effort), not main.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: FAIL — pre-existing failures in
envs/carla_env/(usort/ruff format). Zero.pyfiles insrc/ortests/touched by this PR; all lint noise predates it. - Debug code: CLEAN for this PR — all hits are pre-existing docstring examples in
src/openenv/.
Tier 1: Fixes Required
- Wrong base branch / missing prerequisite:
docs/source/does not exist onmain. Confirmed viagit ls-files docs/source/(empty) andgit merge-base --is-ancestor 15cafebb HEAD(false). The Sphinx documentation system (PR #408, commit15cafebb) lives only in feature branches (feature/issue-384-eval-support,feature/move-llmclient-to-core, etc.) and has never landed onmain. This PR will not apply cleanly tomainand the modified files (docs/source/conf.py,docs/source/index.md, etc.) do not exist in the target branch. Action required: Rebase onto whichever branch carries the Sphinx docs, or wait until the Sphinx base is merged tomainfirst.
Tier 2: Alignment Discussion
None identified. The changes are purely doc-navigation restructuring:
- Toctree hierarchy consolidation is a correct UX fix (one hub per top-nav caption).
- Removing duplicate
navbar_centerkey fromconf.pyis a legitimate bug fix (the original has twonavbar_centerentries in the same dict). - Hiding the version switcher via CSS + conf.py until versioned releases exist is sensible and does not affect any runtime invariant.
- No Python runtime code, API surfaces, or architectural patterns are modified.
Summary
- 1 blocking mechanical issue: PR targets
docs/source/which is absent frommain— needs a dependency branch or a retarget. - 0 alignment points for human review.
- The nav restructuring logic itself is sound and well-explained; approve once the base-branch dependency is resolved.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: FAIL — pre-existing violations in
envs/carla_env/only; zero files touched by this PR trigger them. - Debug code: CLEAN — no debug artifacts introduced.
Tier 1: Fixes Required
-
docs/source/does not exist onmain—git ls-files docs/source/returns nothing on the target branch. The Sphinx docs tree was reverted frommain(commit3e99d68e) and lives only on feature branches. Merging this PR as-is adds nine files that are completely unreachable by deployed CI (docs.ymlrunsmkdocs build --config-file docs/mkdocs.yml; it never touchesdocs/source/). Fix: re-target this PR against whichever branch carries the active Sphinx docs effort, or land that branch onmainfirst.
Tier 2: Alignment Discussion
ALIGNMENT FLAG: Two documentation systems with no documented convergence strategy
- Principle at stake: "Be hands-on — provide ready-to-use implementations, not just specs" (PRINCIPLES.md). Published documentation should reflect actual project state.
- The concern: The repo maintains two parallel doc systems —
docs/mkdocs.yml(deployed, live on GitHub Pages) anddocs/source/conf.py(Sphinx, reverted, absent frommain). This PR improves nav only in the Sphinx tree; the live MkDocs site is unchanged. No documented policy exists on which system is canonical or when Sphinx will replace MkDocs onmain. - Suggested reviewer: @burtenshaw
Internal Correctness (conditional on correct base branch)
The Sphinx-internal logic is sound: all 25 flat root toctree entries are correctly re-delegated to hub index.md files, cross-directory ../-prefixed paths resolve correctly, the contributing-envs dual-parent issue is resolved in the latest commit, and the pre-existing navbar_center duplicate-key (ruff F601) is cleaned up. CSS/JS additions are well-scoped.
Summary
- 1 blocking issue: PR targets
mainbutdocs/source/is absent frommain. - 1 alignment point: parallel doc systems with no documented convergence plan.
- The nav restructuring logic itself is excellent — approve once the base-branch dependency is resolved.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: FAIL — pre-existing failures in
envs/carla_env/andenvs/chat_env/unrelated to this PR (zero.pyfiles edited by this PR feed those suites). Not a blocker for this change. - Debug code: FOUND — pre-existing
printstatements insrc/openenv/core/containers/test_local_docker_provider.pyand TODO insrc/openenv/cli/. Pre-date this PR, not introduced here.
Tier 1: Fixes Required
-
docs/source/_static/openenv-overrides.css—.cookie-banner-wrapper .close-button:focus-visible { outline: none; }removes the focus ring on a keyboard-interactive control. This breaks WCAG 2.4.7 (Focus Visible). Recommend replacing with a visible outline in the brand color instead ofnone:.cookie-banner-wrapper .close-button:focus-visible { color: var(--pst-color-primary); outline: 2px solid var(--pst-color-primary); outline-offset: 2px; }
Tier 2: Alignment Discussion
None identified. This PR is a docs-only restructure. No API signatures, type contracts, reward logic, client-server boundaries, or architectural invariants are touched. The changes are fully within the Sphinx configuration and documentation content layer.
Positive Notes
cookie-banner.js is clean. The script stores only a binary flag ("1") in localStorage under a namespaced key. No PII, no network requests (no fetch, XMLHttpRequest, or beacon), no eval, no external resources loaded, no global namespace pollution (IIFE). The localStorage failure path is handled correctly for private-mode browsers.
conf.py carries a meaningful bug fix: the original configuration had a duplicate "navbar_center" key (first as a bare string "navbar-nav", then as the correct list ["navbar-nav"]). In Python, the second key wins, so the string form was silently ignored. Removing the dead string-valued key is correct.
Sidebar link suppression is reasonable. Replacing theme_variables.get("library_links", []) with [] explicitly prevents unrelated PyTorch ecosystem links from appearing in the OpenEnv sidebar. The intent is clearly explained in the inline comment.
Toctree hierarchy is coherent. The root index.md now delegates to hub pages (guides/index, reference/index, contributing) which own their own child toctrees. This matches Sphinx best practice and should eliminate the duplicate top-nav labels described in the PR.
getting_started/README.rst rename from "Quick Start" to "Interactive Tutorials" is correct — "Quick Start" collided with quickstart.md's nav label.
Summary
- 1 mechanical issue to fix: restore
focus-visibleoutline on the cookie banner close button (accessibility regression). - 0 alignment points for human review.
- Author's claim of a clean
make clean htmlbuild and manual nav verification is credible given the logical consistency of the toctree changes.
Automated review by Claude Code | Learn more
3 participants
Summary
The
pytorch_sphinx_theme2top nav promotes every toctree entry from the root document (docs/source/index.md) into a flat top-level item. The root had ~25 entries across the six captions (Get Started / Guides / Tutorials / Environments / API Reference / Community), which produced a disorienting top bar like:Two underlying issues:
docs/source/getting_started/README.rst(the Sphinx Gallery landing) was titled "Quick Start", colliding withdocs/source/quickstart.md.Changes:
docs/source/guides/index.md,docs/source/reference/index.md, anddocs/source/contributing.mdso their children appear under the right dropdown.auto_getting_started/indexintodocs/source/tutorials/index.md's toctree — interactive tutorials belong under Tutorials, not as a duplicate top-level item.docs/source/getting_started/README.rsttitle from "Quick Start" to "Interactive Tutorials" to remove the label collision.header_links_before_dropdownfrom 7 to 8 so the last hub (Community) stays visible instead of collapsing under "More".Result: 8 top-level items (3 leaves + 5 dropdown hubs), every doc reachable in at most two clicks, no duplicate labels.
Type of Change
Alignment Checklist
Before submitting, verify:
.claude/docs/PRINCIPLES.mdand this PR aligns with our principles.claude/docs/INVARIANTS.mdand no invariants are violated/pre-submit-pr(orbash .claude/hooks/lint.shand tests) and addressed all issues(Docs-only change, no Python runtime code edited — only Sphinx configuration and Markdown/RST. Verified via
cd docs && make clean htmland by opening the generated HTML in a browser.)RFC Status
Test Plan
cd docs && make clean htmlsucceeds with no new warnings attributable to this PR.docs/_build/html/locally and verified the rendered top nav on multiple pages: 8 items —Quick Start,Installation,Core Concepts,Guides ▾,Tutorials ▾,Environments ▾,API Reference ▾,Contributing to OpenEnv ▾.sphinx_sitemapstill emits one URL per page and no internal links broke.Claude Code Review
Output of
/alignment-reviewon this branch:Automated Checks
envs/chat_env/,envs/repl_env/,envs/textarena_env/). This PR changes zero.pyfiles that feed those suites (onlydocs/source/conf.py, which is Sphinx configuration).src/openenv/cli/. All pre-date this PR.Open RFCs Context
RFCs 000, 001, 002, 003, 004, 005 all In Review; none cover docs navigation, toctree structure, or theme configuration.
Tier 1: Fixes Required
None applicable (docs-only restructure; hook noise is pre-existing).
Tier 2: Alignment Discussion
Principle Conflicts: None. The change only reorganises toctree entries and renames one page title. No API, type, isolation, or architectural invariant is touched. All previously reachable pages remain reachable; only the nav grouping changes.
RFC Conflicts: None.
Summary
@burtenshaw