ci(docs): make previews and checks match what actually publishes - #12339
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 696d25d290
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
WalkthroughChangesDocumentation preview and publication checks
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/fern-docs.yml:
- Around line 512-515: The Verify published pages carry their component CSS step
currently fails every main docs publish because the existing baseline has known
style violations. Update this workflow step to be informational rather than
blocking until those styles are fixed, while preserving its current conditions
and check command.
In `@docs/fern/scripts/check_published_styles.py`:
- Around line 68-69: Add focused one-line tests for the regex logic used by
as_rule and in_markup, covering a matching minified CSS rule and a selector that
appears only in HTML markup. Assert the expected counts so future escaping or
boundary changes cannot invert the release-gate result.
- Around line 42-52: Update fetch() and its callers so selector validation
occurs within the retry loop, allowing successful but stale HTML responses to
trigger another fetch after delay. Re-fetch when the required rules are missing,
and only fail after all attempts are exhausted; preserve existing
transport-error retry behavior.
- Line 46: Validate the URL used by the request before the urlopen call in the
script’s publishing-check flow, allowing only the intended HTTPS origins and
rejecting unsupported schemes or hosts, including unsafe --base values. If Ruff
still reports S310 after validation, add a narrowly scoped suppression on this
urlopen call with a brief justification, then run ruff format and ruff check
--fix.
🪄 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: db53163f-36d2-4677-be25-9b456b9839ce
📒 Files selected for processing (2)
.github/workflows/fern-docs.ymldocs/fern/scripts/check_published_styles.py
|
/ok to test 696d25d |
|
/ok to test f4b35bc |
|
/ok to test c41b036 |
|
/ok to test 22d639a |
|
/ok to test b31f217 |
Production sets global-theme: nvidia, which per Fern's docs.yml schema overrides local branding config including css and js. It also replaces the custom footer:. So main.css is never linked, custom.js never loads, and the SITE_CSS fallback in CustomFooter.tsx never renders. Components that ship a page-level <style> block survive; anything relying on main.css publishes unstyled. Nothing catches this. The workflow applies the theme only when the ref is main and deletes it for every preview build, so previews render under the exact conditions where the bug cannot appear. It is invisible until live. Two changes. check_published_styles.py asserts, against the published site, that a selector appears as a CSS rule rather than only as a class name in markup. That distinction is the whole point: a page can be full of class="foo" while the rule styling it is absent. Runs after publish on main, with retries for CDN propagation. A themed_preview input on workflow_dispatch applies the theme to a preview build, which is the only way to see this before merge. Opt-in, because previews skip the theme deliberately so contributors without an nvidia-org FERN_TOKEN still get working previews. Against the live site today the checker fails on Home (2 selectors), Community and the blog, and passes on the compatibility page, which already delivers its CSS via ReferenceStyles.tsx. #12330 makes the failing four pass. Signed-off-by: Dan Gil <dagil@nvidia.com>
Previews deleted the theme, so they rendered with the project css/js and the custom footer intact -- conditions that never ship. That made them faithful to `fern docs dev` and systematically wrong about the live site, which is how unstyled Home, Community and blog pages reached production without anyone seeing them in review. Apply the theme to every hosted build. copy-pr-bot mirrors each PR to a pull-request/N branch on this repo, so previews already run with secrets.FERN_TOKEN available, including for external contributors' PRs. That makes the opt-in dispatch input unnecessary, so it is removed. The source repo's docs.yml is unchanged and still omits global-theme, so `fern docs dev` continues to work without an nvidia-org token (#10073). The theme is injected only into the docs-checkout copy CI builds from. Consequence worth expecting: previews of pages that still rely on main.css will render unstyled, because that is what production does. #12330 fixes the pages that currently do. Signed-off-by: Dan Gil <dagil@nvidia.com>
fern check pre-merge runs against docs/fern/, but production publishes from a composed tree: content rsynced into fern/pages-dev/, nav paths rewritten, examples subtrees synced to the branch root, versioned snapshots frozen into pages-vX.Y.Z/. None of that executes outside main pushes and tag cuts, so a link that resolves in the source tree can break after the transform and nothing catches it before merge. simulate_docs_website.sh already replays both jobs against docs-website and asserts the invariants, including fern check on the composed result. It was written to be run by hand. Run it in CI instead. Wired into pre-merge-status-check, which is a required check -- otherwise the job runs but a failure would not block merge. Verified locally against origin/docs-website: all 7 assertions pass, fern check on the composed tree reports 0 errors. Signed-off-by: Dan Gil <dagil@nvidia.com>
Each *Styles.tsx holds ~50KB of CSS in a template literal. One raw backtick
or `${` anywhere in that body closes or interpolates the literal and breaks
the build, and it is easy to introduce by hand -- a CSS comment quoting a
class name as `node--k8s` is enough. That exact defect reached main in
#12330 and had to be repaired separately.
Nothing caught it. fern check does not parse TSX, sync_site_css only reads
main.css, and the CSS-level conservation checks compare declarations rather
than syntax. sync_site_css.py already applies this rule to main.css before
mirroring it; this applies the same rule to the components that hold CSS
directly.
Verified against the real defect: reintroducing the backtick in the orbit
comment makes the guard fail at LandingStyles.tsx:972, and esbuild
independently reports `Expected ";" but found "node"` -- the same error the
repair commit quoted.
Also corrects a comment in fern-docs.yml that still said previews delete the
theme. This PR makes them themed, so the post-publish check is a backstop
now rather than the only signal.
Signed-off-by: Dan Gil <dagil@nvidia.com>
Nine unresolved review threads across the two new scripts and the workflow.
Grouped into six changes:
1. Canonical full Apache header on both new Python scripts, with the
2025-2026 copyright range that docs/fern/AGENTS.md requires for
code/config files. The existing docs/fern/resolve_translation_links.py
header was the reference.
2. Explicit HTTPS trust boundary for --base. All requests go through
url_from_base(), which rejects any origin outside ALLOWED_ORIGINS and
any scheme other than https. urlopen() no longer takes an unchecked
URL, so the raw call has a documented boundary that Ruff's S310
guidance points at concretely.
3. Retry on stale successful responses, not only on transport failures.
The CDN can return HTTP 200 with the pre-publish HTML while the new
page is still propagating; fetch() would return that stale body and
the selector check would fail on a healthy publish. probe() now
re-fetches when as_rule_count == 0, up to the same --retries budget
that already covered URLError / TimeoutError.
4. Extract the CSS-rule regexes into as_rule_count() and
in_markup_count() helpers, and add a --test mode that exercises them
against minified, class-only, adjacent-name, and both-rule-and-markup
cases. Wire the self-test into .pre-commit-config.yaml as
check-published-styles-selftest so escaping or boundary changes fail
the hook instead of silently inverting the release gate.
5. Drop steps.changes.outputs.has_changes from the live-verification
step's condition. A workflow rerun after a failed publish checks out
the already-updated docs-website branch, so has_changes flips false
and the step would skip, letting a still-broken publish rerun green.
The step now runs on every main invocation; is_main still gates it
to main.
6. Revalidated the two baseline findings against the current tree:
- "CHECKS gates on selectors this PR leaves only in main.css /
CustomFooter" -- INVALID. .dynamo-story-windowbar,
.dynamo-welcome__terminal, .dynamo-community-page,
.dynamo-blog-art__grid, and .dynref-panel are all defined inside
the *Styles.tsx components (LandingStyles / BlogStyles /
ReferenceStyles), and welcome.mdx / community/README.mdx /
digest/index.mdx / reference/compatibility.mdx already import and
render the corresponding Styles component.
- "Do not gate every main docs publish on a known-failing baseline"
-- shares the same premise; also INVALID for the same reason.
Validation:
- python3 docs/fern/scripts/check_published_styles.py --test -> 7/7
- python3 docs/fern/scripts/check_style_components.py -> checked 4
- url_from_base() rejects http://, file://, and hosts outside
ALLOWED_ORIGINS while accepting https://docs.nvidia.com/*
- probe() re-fetches on stale 200 and returns as_rule == 1 after the
fresh body arrives (simulated) while still returning last-html and
count == 0 after exhausting retries.
- yaml.safe_load parses fern-docs.yml and .pre-commit-config.yaml
- pre-commit run over the four changed files: isort/black/flake8/ruff/
codespell/check-style-components/check-published-styles-selftest all
Passed.
Signed-off-by: Dan Gil <dagil@nvidia.com>
b31f217 to
0871ff3
Compare
|
/ok to test 0871ff3 |
The Reference tab's General group is shared across versions: release
snapshots keep its nav on ../pages-dev/ so every version dropdown renders
the always-current reference. Both the workflow and the composition
harness found that group with
.navigation[] | select(.tab == "reference")
| .variants[] | select(.title == "General")
#12410 deliberately flattened the Reference tab into folded sections with
no variant selector. The selector has matched nothing since 2026-07-30 --
verified against docs-website, where dev.yml carried `title: General` on
07-27 through 07-29 and lost it on 07-30. Shared Reference has been off
ever since, and would have frozen a per-version reference copy at the next
tag cut. Nothing failed, because the only guard was a ::warning:: that
fires during a tag cut where nobody reads it.
Key the group on where its content lives instead. #12373 moved those pages
to pages/reference/general/ and gave every other former variant its own
directory, so the split the variants expressed in the nav is now expressed
by directory -- a signal IA work does not rename in passing. The Reference
tab's flattening is preserved; only the machinery moves.
The snapshot drop and the version-config revert become plain path
operations, which also removes a live defect: a yq assignment whose
left-hand side traverses a missing key auto-creates it, so the dead
`.variants[]` selector injected an empty `variants: []` into the composed
dev.yml and <TAG>.yml. That invalidates the whole navigation against
Fern's schema -- reported confusingly as a bad object at
$.navigation[4].layout[3], several hundred lines from the injection.
That is the Docs Website Composition Check failure on this PR, and the
same unguarded assignment in the release-version job would have corrupted
the next tag's version file.
Both silent-skip paths in the workflow become hard failures, and the
harness gains assertion 9 asserting the group is findable at all, so the
next rename fails on the PR that causes it rather than six days later.
Assertion 4 also stops false-passing: its find target no longer exists, so
find errored and wc counted zero while the assertion reported ok.
Validation:
DOCS_WEBSITE_REF=origin/docs-website \
bash docs/fern/scripts/simulate_docs_website.sh
9. dev.yml exposes a shared Reference group (7 entries) ok
2. v9.9.9.yml: reference/general shared (13 pages-dev refs) ok
2. v9.9.9.yml: Components section frozen (runtime configuration) ok
3. snapshot drops shared files, keeps versioned reference/ ok
5. pre-rework versions gain no shared-reference pointers ok
6. round-two propagation reaches the cut version's nav ok
7. converter moved to fern/scripts/ with no stale root copy ok
8. translated links resolve to dev and tag-pinned site URLs ok
1. fern check on composed tree (0 errors) ok
ALL ASSERTIONS PASSED
Was four FAILs (2, 3, 6 and fern check) before this change. Negative test:
renaming pages/reference/general/ to reference/core/ fails assertion 9 with
exit 1 while fern check stays at 0 errors, confirming a non-matching
selector no longer corrupts the tree. Composed dev.yml and v9.9.9.yml carry
no variants key. bash -n passes on all 33 run blocks in fern-docs.yml.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Dan Gil <dagil@nvidia.com>
|
/ok to test 076424f |
Signed-off-by: Dan Gil <dagil@nvidia.com> # Conflicts: # .pre-commit-config.yaml
|
/ok to test 8976471 |
The composition job fetched yq into /usr/local/bin, which needed sudo for both the write and the chmod. $RUNNER_TEMP/bin is already writable by the runner user, and adding it to $GITHUB_PATH puts it ahead of any preinstalled yq for the steps that follow. The published-styles check covered LandingStyles, BlogStyles, and ReferenceStyles but not RecipeStyles, which carries the benchmark grid on the feature-benchmark recipe pages. Added .dynamo-benchmark-grid on llama-3-3-70b-topology so all four style components are checked. Signed-off-by: Dan Gil <dagil@nvidia.com>
|
/ok to test 83729ee |
Signed-off-by: Dan Gil <dagil@nvidia.com>
|
/ok to test 60f6c30 |
dyn-3691-extract-shared-target-pid-cuda-customstorage-operation-layer * 'main' of https://github.com/ai-dynamo/dynamo: (50 commits) docs(cli): correct removed vLLM prefill-worker flag reference (#12581) docs(operator): reserve webhook Ignore for emergencies (#12563) ci(docs): make previews and checks match what actually publishes (#12339) refactor(vllm): organize custom encoder modules (#12416) feat(llm): Select reasoning output field via env var (#11464) feat(runtime): add TLS support to TCP request plane (#10921) fix: convert conditional disagg sglang warning to httperror 400 (#12578) feat(operator): add runtime feature gates (#12421) refactor(runtime): extract PushRouter transport seam behind StreamingDispatch trait (#12447) feat(replay): add deterministic canonical offline reports (#12363) build: bump ModelExpress to 0.5.0(OPS-7978) (#12455) fix(mocker): use logical KV tokens for decode timing (#12583) fix(examples): update Triton example for CUDA 13 + fix libdcgm copy (DYN-3697) (#12577) refactor(operator): implement composition-first DGD reconciliation (#12283) feat(frontend): add basetenkenizer backend (#12376) fix(profiler): configure rapid mocker without planner (#12573) docs(vllm): correct worker-role flags and document --kv-transfer-config (#12568) ci: add Kubernetes deploy test to nightly (#12090) fix(container): reuse pinned protoc in runtime image (#12535) feat(self-host): flip DYN_SELF_HOST_METADATA default to ON (gh-8749) (#11417) ... Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Summary
Production sets
global-theme: nvidia. Fern'sdocs.ymlschema documents what that does: "Theme values override local branding configuration (colors, typography, logo, fonts, JS, CSS, etc.)". On the live sitemain.cssis never linked,custom.jsnever loads, and the customfooter:renders empty — so theSITE_CSSfallback insideCustomFooter.tsxnever reaches a page either.Components that deliver their CSS as a page-level
<style>block survive it. Anything relying onmain.csspublishes unstyled.Nothing catches this today, because previews deleted the theme. They rendered with the project
css:,js:and footer intact — conditions that never ship. That made previews faithful tofern docs devand systematically wrong about the live site, which is how unstyled Home, Community and blog pages reached production without anyone seeing them in review.What changed
Preview builds now get the theme. One conditional, applied to every hosted build instead of
mainonly.copy-pr-botmirrors each PR to apull-request/Nbranch on this repo, so previews already run withsecrets.FERN_TOKENavailable — including for external contributors' PRs, which go through the same mirror. No opt-in, no dispatch, no separate path.The source repo's
docs.ymlis unchanged and still omitsglobal-theme, sofern docs devkeeps working without an nvidia-org token (#10073). The theme is injected only into thedocs-checkoutcopy CI builds from.check_published_styles.pyasserts against the published site that a selector appears as a CSS rule (followed by{or,), not merely as a class name in markup. That distinction is the whole check: a page can be full ofclass="dynamo-story-windowbar"while the rule that styles it is absent, which is precisely the failure. Runs after publish onmain, with retries for CDN propagation.Verification
Run against the live site as it stands today:
A real red baseline: it reproduces the live bug on four pages and passes on the compatibility page, which already delivers its CSS through
ReferenceStyles.tsx. #12330 makes the failing four pass.pre-commitclean; YAML parses; both changed paths are covered by.github/filters.yaml, so thechanged-filesgate will not trip.Expect this, it is not a regression
Once this merges, previews of any page still relying on
main.cssrender unstyled — because that is what production does. #12330 fixes the pages that currently do. Land #12330 first if a red post-publish step onmain, or a visibly unstyled preview, would be alarming out of context.Not addressed
global-themereplacing the project stylesheet rather than layering over it. That also leavescustom.jsdead in production and the non-page-scoped sidebar rules undeliverable. Needs the theme owners; started at #11952.Update: check the composed tree, not just the source tree
Third gap, same theme.
fern checkpre-merge runs againstdocs/fern/, but production publishes from a composed tree: content rsynced intofern/pages-dev/, nav paths rewritten,examples/subtrees synced to the branch root, versioned snapshots frozen intopages-vX.Y.Z/. None of that executes outside main pushes and tag cuts, so a link that resolves in the source tree can break after the transform with nothing catching it before merge.docs/fern/scripts/simulate_docs_website.shalready replays both jobs againstdocs-websiteand asserts the invariants — includingfern checkon the composed result, no.tsxleaking intopages-dev/components/, and correct shared-vs-frozen reference wiring. It was written to be run by hand before touching the composition. This runs it in CI instead.Verified locally against
origin/docs-website:Wired into
pre-merge-status-check. That aggregator only fails when a job in itsneedsfails, so without wiring the new job would run and a failure would not block merge.The three gaps together
fern docs dev, wrong about productionmain