Skip to content

[doc] Add sphinx-fix skill to classify doc-build warnings#64147

Merged
richardliaw merged 9 commits into
ray-project:masterfrom
dstrodtman:djs-260616-sphinx-fix
Jul 1, 2026
Merged

[doc] Add sphinx-fix skill to classify doc-build warnings#64147
richardliaw merged 9 commits into
ray-project:masterfrom
dstrodtman:djs-260616-sphinx-fix

Conversation

@dstrodtman

Copy link
Copy Markdown
Contributor

Why

The Ray docs build with fail_on_warning: true, so any single Sphinx warning fails the build. The expensive part of a docs change is usually not the edit — it's locating the warning, a manual, token-heavy chain (one RST→MyST conversion tier took three ~6-minute Read the Docs cycles just to find the failure). These failures are a small, classifiable set with mechanical, canonical fixes.

What

Adds sphinx-fix, a deterministic, read-only Claude Code skill under doc/.claude/skills/ that turns a Sphinx warning stream into a severity-ordered list of findings, each with its canonical fix:

  • rules.yaml — the canonical category/signature → cause → fix table (10 seed rules plus known-benign suppressions). It is the single source the rst-to-myst skill now points at for the shared MyST link classes.
  • sphinx_fix.py — a stdlib-only engine (prefers PyYAML, falls back to a bundled parser so it runs anywhere) that parses the warning stream (a Read the Docs log, a local build, or pasted text), classifies each warning, detects a hard-broken build first, triages by severity tier (fatal → structural → plain), segregates suppressed classes, and lists every unclassified warning so the table can grow from real misses. v0 is human-in-the-loop: it proposes fixes and never edits files.
  • SKILL.md — when and how to use it, the severity-tier iterate loop, applying fixes, and the unclassified→follow-up feedback loop.
  • Tests — eight fixtures plus golden files, run via python doc/.claude/skills/sphinx-fix/sphinx_fix.py --selftest (which also cross-checks the fallback parser against PyYAML).

The new files live under doc/.claude/skills/ and do not participate in the Sphinx build or doctest (those globs start at doc/source/). Verified end-to-end against a real Read the Docs build log.

Notes for reviewers

🤖 Generated with Claude Code

dstrodtman and others added 2 commits June 16, 2026 10:07
## Why

Ray is migrating doc/source/ pages from reStructuredText to MyST Markdown
(MyST is the standard for new pages; a lint check rejects newly-added
`.rst`). Many pages remain. The conversion has a handful of constructs
that silently break the `fail_on_warning` build or drop doctest coverage
if mishandled, and re-discovering them each time is error-prone.

## What

Add a user-invocable `rst-to-myst` skill under `doc/.claude/skills/` that
captures the conversion playbook learned migrating the `ray-contribute/`
guides:

- the RST→MyST directive mapping table;
- five build-breaking hard rules — label preservation, `{doc}` vs bare
  cross-extension links, heading levels by order-of-appearance, doctest
  literal-vs-executed, and the `doc/BUILD.bazel` doctest exclude entries;
- construct-by-construct notes, a reference-update checklist, reusable
  static-check snippets, and the build + doctest verification steps.

The skill is discovered when working in `doc/`.

Signed-off-by: Douglas Strodtman <douglas@anyscale.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Why

The Ray docs build with `fail_on_warning: true`, so any single Sphinx warning
fails the build. The expensive part of a docs change is usually not the edit —
it's locating the warning, a manual, token-heavy chain (one RST→MyST tier took
three ~6-minute Read the Docs cycles just to find the failure). The failures
are a small, classifiable set with mechanical, canonical fixes.

## What

Adds `sphinx-fix`, a deterministic, read-only skill that turns a Sphinx
warning stream into a severity-ordered list of findings, each with its
canonical fix:

- `rules.yaml` — the canonical category/signature → cause → fix table (10 seed
  rules plus known-benign suppressions). It is the single source the
  `rst-to-myst` skill now points at for the shared MyST link classes.
- `sphinx_fix.py` — a stdlib-only engine (prefers PyYAML, falls back to a
  bundled parser so it runs anywhere) that parses the warning stream (a Read
  the Docs log, a local build, or pasted text), classifies each warning,
  detects a hard-broken build first, triages by severity tier (fatal →
  structural → plain), segregates suppressed classes, and lists every
  unclassified warning so the table can grow from real misses. v0 is
  human-in-the-loop: it proposes fixes and never edits files.
- `SKILL.md` — when and how to use it, the severity-tier iterate loop, applying
  fixes, and the unclassified→ticket feedback loop.
- Tests: eight fixtures plus golden files, run via `--selftest` (which also
  cross-checks the fallback parser against PyYAML).

The new files live under `doc/.claude/skills/` and do not participate in the
Sphinx build or doctest. Verified end-to-end against a real Read the Docs
build log.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Douglas Strodtman <douglas@anyscale.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new documentation migration and diagnostic toolset, including a skill for converting reStructuredText to MyST Markdown, a diagnostic skill for Sphinx build warnings, a rules configuration file, and a Python script (sphinx_fix.py) to classify and suggest fixes for Sphinx warnings. The review feedback highlights a potential crash in the fallback YAML parser when processing empty or whitespace-only inputs, and recommends explicitly specifying encoding='utf-8' across multiple read_text() calls to ensure cross-platform compatibility on Windows.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +178 to +186
def _yaml_fallback(text: str):
items: list[list] = []
for raw in text.split("\n"):
content = _strip_comment(raw)
if not content.strip():
continue
indent = len(content) - len(content.lstrip(" "))
items.append([indent, content.strip()])
pos = [0]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If the input text is empty or contains only comments/whitespace, items will be empty. This causes peek() to return None, leading to a TypeError: 'NoneType' object is not subscriptable when cur[1] is accessed in parse_node. Adding a guard to return an empty dictionary when items is empty prevents this crash.

def _yaml_fallback(text: str):
    items: list[list] = []
    for raw in text.split('\n'):
        content = _strip_comment(raw)
        if not content.strip():
            continue
        indent = len(content) - len(content.lstrip(' '))
        items.append([indent, content.strip()])
    if not items:
        return {}
    pos = [0]


def load_rules(path: Path) -> tuple[list[Rule], list[Suppression], dict]:
try:
text = path.read_text()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Specify encoding='utf-8' when calling read_text() to prevent potential UnicodeDecodeError crashes on platforms where the default system encoding is not UTF-8 (such as Windows).

Suggested change
text = path.read_text()
text = path.read_text(encoding='utf-8')

import yaml
except ImportError:
return []
text = rules_path.read_text()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Specify encoding='utf-8' when calling read_text() to ensure consistent cross-platform behavior and avoid UnicodeDecodeError on Windows.

Suggested change
text = rules_path.read_text()
text = rules_path.read_text(encoding='utf-8')

for fx in fixtures:
report = build_report(
fx.read_text(), rules, supps, versions=None, baseline=baseline
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Specify encoding='utf-8' when calling read_text() to ensure consistent cross-platform behavior and avoid UnicodeDecodeError on Windows.

            fx.read_text(encoding='utf-8'), rules, supps, versions=None, baseline=baseline

if not gold.is_file():
errs.append(f"missing golden: {gold.name} (run --update-golden)")
continue
want = gold.read_text()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Specify encoding='utf-8' when calling read_text() to ensure consistent cross-platform behavior and avoid UnicodeDecodeError on Windows.

Suggested change
want = gold.read_text()
want = gold.read_text(encoding='utf-8')

def read_stream(args) -> str:
src = args.file or args.input
if src and src != "-":
return Path(src).read_text()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Specify encoding='utf-8' when calling read_text() to ensure consistent cross-platform behavior and avoid UnicodeDecodeError on Windows.

Suggested change
return Path(src).read_text()
return Path(src).read_text(encoding='utf-8')

…-fix

Signed-off-by: Douglas Strodtman <douglas@anyscale.com>

# Conflicts:
#	doc/.claude/skills/rst-to-myst/SKILL.md
Empty, whitespace-only, or comment-only input made the no-PyYAML fallback parser crash with a TypeError when parse_node dereferenced a None peek. Return None to match PyYAML's safe_load("") behavior.

Signed-off-by: Douglas Strodtman <douglas@anyscale.com>
@dstrodtman dstrodtman marked this pull request as ready for review June 24, 2026 17:52
@dstrodtman dstrodtman requested a review from a team as a code owner June 24, 2026 17:52
@ray-gardener ray-gardener Bot added docs An issue or change related to documentation core Issues that should be addressed in Ray Core devprod labels Jun 24, 2026
@dstrodtman dstrodtman added the go add ONLY when ready to merge, run all tests label Jun 29, 2026
… module

An autosummary `Extension error` at builder-inited names the first documented
object that failed to import, which is usually a decoy: an unrelated module's
import chain broke (commonly a dep mocked by autodoc_mock_imports but used at
import time), aborting a shared import. The engine already classifies this as a
tier-1 abort; this adds a decoy-aware `hint` to the abort banner (and JSON) so
the report points at the real fix -- trace the import and make the eager import
lazy -- rather than the named module. Covered by a new fixture and golden.

Real case: ray#63821 (read_lerobot) failed with
`no module named ray.air.integrations.comet`; the cause was an eager
LeRobotDatasource import pulling tensor_extension under mocked pandas, surfacing
on comet only because ray.air imports ray.data.

Signed-off-by: Douglas Strodtman <douglas@anyscale.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment thread doc/.claude/skills/sphinx-fix/sphinx_fix.py
When no build summary is present, detect_abort scanned every line for
abort signatures. The `severe` and `extension-error` patterns also match
ordinary warnings (`path:line: SEVERE: ...` and messages that merely
contain "Extension error") that parse_warnings already classifies, so a
pasted or `-w` warnings-only stream with no summary line got exit code 2
and an abort banner instead of tiered findings.

Skip lines that parse as ordinary Sphinx warnings when scanning for
aborts: a hard abort happens before the warning pass and is never
printed in the `path:line: LEVEL: msg` form. Genuine aborts (tracebacks,
"Extension error:", import failures) are unaffected -- the existing
abort fixtures still classify and exit 2.

Add a warnings_only_severe fixture and golden covering the regression.

Signed-off-by: Douglas Strodtman <douglas@anyscale.com>
Comment thread doc/.claude/skills/sphinx-fix/rules.yaml Outdated
Comment thread doc/.claude/skills/sphinx-fix/sphinx_fix.py
dstrodtman and others added 2 commits July 1, 2026 10:11
## Why

Premerge was red on the `trailing-whitespace` pre-commit hook, not a
master-side failure: the new `tests/golden/autosummary_import_abort.txt`
carries trailing whitespace on blank excerpt lines, and the hook rewrites
it. The root gap is that the top-level pre-commit `exclude` covers
`doc/source/` but not the newer `doc/.claude/` tooling tree, so the skill's
byte-exact test data gets whitespace-normalized. Fixtures capture real
Sphinx logs verbatim and goldens capture the tool's exact rendered output;
normalizing either corrupts the test data.

## What

- `.pre-commit-config.yaml`: add `doc/.claude/skills/sphinx-fix/tests/` to
  the top-level exclude so fixtures and goldens are preserved verbatim,
  alongside the existing test-data excludes (`rllib/offline/tests/data`,
  `python/ray/data/examples/data/`). `sphinx_fix.py` and `rules.yaml` stay
  linted.
- `sphinx_fix.py` render_human: rstrip each abort-excerpt line so a blank
  line no longer emits the 4-space indent as trailing whitespace. Cleaner
  output regardless of the hook. Golden regenerated.
- `sphinx_fix.py` run_selftest: `--update-golden` no longer returns 0 when
  YAML cross-check or schema errors were collected, so regenerating goldens
  can't paper over a broken rules.yaml (Cursor Bugbot).
- `rules.yaml` myst-xref-ambiguous: drop the bare `ambiguous` signature
  alternative so a `match: any` rule no longer classifies unrelated warnings
  that merely contain the word. Category matching (`myst.xref_ambiguous`)
  and the specific `not unique`/`matches more than one` phrases still fire
  the rule (Cursor Bugbot).

Selftest passes (10 fixtures, 10 rules, 4 suppressions).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Douglas Strodtman <douglas@anyscale.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit bd73add. Configure here.

f"Next: fix Tier {top} first (it masks others), then rebuild and "
"re-run — do not assume one pass is complete."
)
return "Next: apply the fixes above, then rebuild and re-run to confirm clean."

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict ignores unclassified warnings

Medium Severity

When a report has both classified findings and unclassified warnings, _verdict returns only the findings message (including “re-run to confirm clean”) because it checks report.findings before report.unclassified. Unclassified items still fail the docs build but are omitted from the closing guidance.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit bd73add. Configure here.

@richardliaw richardliaw merged commit b7d4022 into ray-project:master Jul 1, 2026
6 checks passed
richardliaw pushed a commit that referenced this pull request Jul 1, 2026
## Description

The `post_checkout` skip-guard in `.readthedocs.yaml` (added in #64277)
skips the RtD Sphinx build on PRs that touch no doc-affecting files. It
keys on a coarse pathspec:

```
git diff --quiet origin/master...HEAD -- doc/ python/ray/ rllib/ .readthedocs.yaml
```

Because it matches all of `doc/`, a PR whose only changes are under
`doc/.claude/` (Claude Code skills and agent files) counts as
doc-affecting and runs a full Sphinx build — even though those files
never participate in the build, which globs from `doc/source/`.

This surfaced on #64147 (the entire diff was under
`doc/.claude/skills/**` plus `.pre-commit-config.yaml`), which should
have been a no-op for RtD but ran a full build. While the doc build
cache was unavailable (from the Sphinx 8.2.3 upgrade until #64414), that
full build was a cold clean build that repeatedly timed out the RtD
check. A correctly-scoped guard skips the build entirely and sidesteps
the timeouts regardless of cache health.

This PR adds a `':(exclude)doc/.claude/'` git exclude pathspec to the
guard's `git diff` and to the doc-affecting-paths echo just below it, so
a PR touching only those paths skips the build. Both stay on single
lines with no backslash escapes, so RtD's job-runner preprocessor (which
silently drops scripts containing certain constructs) preserves the
script.

Verified locally: a staged change under only `doc/.claude/` makes `git
diff --quiet ...` return true (skip), while a change under `doc/source/`
still returns false (build).

## Related issues

Related to #64277, #64414.

## Additional information

The excluded set is intentionally conservative — `doc/.claude/` is the
clear build-irrelevant case today. A false skip would publish stale
output, so other non-source paths were left in scope.

---------

Signed-off-by: Douglas Strodtman <douglas@anyscale.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Issues that should be addressed in Ray Core devprod docs An issue or change related to documentation go add ONLY when ready to merge, run all tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants