py/envoy.docs.sphinx_runner: Code review notes

[Copilot]

Adds REVIEW.md — a structured second-pass code-quality report for py/envoy.docs.sphinx_runner. No source changes.

Coverage (12 categories, 28 findings)

• Architectural — monolithic SphinxRunner (arg parsing, version math, sphinx invocation, HTML archiving all in one class); dead _build_dir = "." class var; validate_args raises SphinxBuildError for a pre-flight arg check; intersphinx_mapping typed optional but always populated
• Sphinx integration — html_dir hard-coded to generated/html regardless of --build_target; sphinx exit code boolean-coerced (actual code discarded); dead Sphinx <3.5 compat guards in vendored sphinx_tabs; module-level LEXER_MAP built at import time
• Async correctness — run() is async with zero awaits; entire Sphinx build (potentially 30 min) blocks the event loop; check_env() does blocking read_text() on the loop
• Filesystem I/O — FileNotFoundError escapes check_env() unhandled; save_html() destroys existing output before new build is verified; rst_tar truthiness relies on pathlib.Path("") being falsy
• Caching — rst_dir and config_file @cached_property do side-effecting I/O (tar extraction, YAML write) at access time; colors cached for a trivially cheap dict construction
• Error handling — print(e) in run() bypasses self.log; _validate() loses exception context (raise … from e missing); debug() prints timing in finally even on failure
• Config hygiene — py_compatible checks >= 3.8 but setup.cfg requires >=3.12 (guard is effectively dead); wrong filename in check_env() error message; envoyproxy.io URL hard-coded inline
• Logging — build_summary() entirely print()-based; Sphinx warnings swallowed after -W --keep-going with no structured record
• Types — versions_path missing return annotation; colors → dict unparameterised; blanket # type:ignore without justification on colorama, sphinx.cmd.build, sphinxcontrib.httpdomain; sphinx_tabs/tabs.py entirely unannotated
• Testing — tests mirror implementation 1:1 rather than asserting behaviour; cmd.py and sphinx_tabs untested; FileNotFoundError case in check_env() missing; test_sphinx_runner_run is async without @pytest.mark.asyncio
• Dead code — debug() has carried a # TODO: remove this for multiple release cycles; two commented-out pkg_resources namespace declarations; stale # TODO: Use packaging.version.Version
• Docs — release_level docstring copy-pasted verbatim from py_compatible; README.rst is one sentence with no CLI reference or extension inventory

Follow-up PRs

Eight prioritised, self-contained PRs are proposed at the end of the report, from quick wins (S effort: error handling, dead-code removal, annotation gaps) through to medium-effort items (async remediation, caching refactor, test quality).

Original prompt

Deep code-quality review of py/envoy.docs.sphinx_runner

Deliverable: a single PR that adds one new markdown file containing a structured code-quality / code-smell report for py/envoy.docs.sphinx_runner. No code changes — report only.

Where to put the file

py/envoy.docs.sphinx_runner/REVIEW.md

What the report must cover

This is a deeper second-pass review that follows the in-flight initial cleanup PR (packaging metadata, lower dep bounds, mechanical Py3.12+ typing/syntax modernisation). Explicitly skip anything already covered by that initial pass — i.e. do NOT re-list:

• setup.cfg metadata fixes (url, classifiers, python_requires, py_modules, packages.find, package_data, the stale commented-out # sphinx-tabs… lines).
• Adding lower bounds to deps already covered.
• typing.List→list, Optional[X]→X | None, collections.abc import migrations.

Investigate and categorise findings under at least these headings (omit a heading if you genuinely find nothing under it, but be thorough — explore every module and test file):

1. Architectural / API-surface smells — class hierarchies that don't earn their abstraction, leaky abstractions, public API that should be private, modules doing too much / too little, indirection that obscures behaviour.
2. Sphinx integration — custom builders/extensions correctness, conf.py overrides, theme handling, intersphinx config, build-warning handling (treat-as-error?), parallelism/-j auto usage.
3. Async correctness — missing await, accidental coroutine objects, sync I/O on the event loop, blocking subprocess calls inside async functions, async-for-async's-sake.
4. Subprocess / filesystem I/O — shell=True, unsanitised paths, missing cwd discipline, temp-file/temp-dir leaks, race conditions, output-tree assumptions, missing cleanup on failure.
5. Caching / memoisation — @cached_property / functools.cache usage, cache-key bugs, caches that don't actually cache or grow unboundedly.
6. Error handling — bare except:, swallowed exceptions, raise … from None hiding root causes, missing context in re-raises, log-and-return-success antipatterns.
7. Configuration / data hygiene — magic strings/numbers that should be constants, hard-coded paths/URLs, schema validation gaps for input YAML/JSON/RST configuration.
8. Logging / observability — print calls, inconsistent log levels, f-string-formatted logs vs %s lazy formatting, missing context fields, swallowed sphinx warnings.
9. Type-annotation correctness (beyond mechanical 3.12 syntax) — Any-soup, missing return annotations on public functions, structural types that should be Protocols, # type: ignore without justification, mypy errors being suppressed.
10. Testing smells — over-mocking, tests mirroring implementation 1:1 instead of asserting behaviour, missing edge-case coverage (empty inputs, sphinx build failures, malformed config, missing files), assert_called_once_with overuse, slow/flaky tests, tests sharing mutable fixtures.
11. Dead / duplicated / commented-out code — references to removed functionality, commented-out lines (besides the sphinx-tabs ones already being removed), obvious copy-paste, unused private helpers, files imported by nothing.
12. Documentation — missing/stale docstrings on public API, README.rst accuracy, examples that no longer work.

Format of the report

Use this structure (markdown):

# `envoy.docs.sphinx_runner` — code review

_Generated <date>. Follow-up to the initial packaging cleanup pass._

## Summary

<one-paragraph high-level take: overall health, biggest risk areas, rough estimate of how much work the follow-ups represent>

## Findings

### 1. Architectural / API-surface smells

#### 1.1 <short title>
- **Where**: `path/to/file.py:L10-L42` (link with full GitHub permalink to the commit SHA you reviewed)
- **What**: <concise description of the problem>
- **Why it matters**: <impact — correctness / maintainability / performance / security>
- **Suggested fix**: <concrete, actionable suggestion; link sibling files if a pattern from elsewhere should be borrowed>
- **Effort**: S / M / L
- **Risk**: low / medium / high

#### 1.2 ...

### 2. Sphinx integration

...

(etc. for each heading)

## Recommended follow-up PRs

Prioritised list of follow-up PRs that could be spun out, each one self-contained and reviewable in isolation. For each:

1. **<title>** — <one-line description> — bundles findings <ids>. Effort <S/M/L>. Risk <low/medium/high>.
2. ...

(Aim for ~5–10 follow-up PRs, sized so each is reviewable in one sitting.)

Rules

• Every finding must include a GitHub permalink with line anchors (#L10-L42) pointing at the commit SHA you reviewed.
• Be concrete. "Some functions could use better names" is useless; "SphinxRunner._build_dir at swallows ...

This pull request was created from Copilot chat.

[netlify]

✅ Deploy Preview for nifty-bassi-e26446 ready!

Name	Link
🔨 Latest commit	d8393ca
🔍 Latest deploy log	https://app.netlify.com/projects/nifty-bassi-e26446/deploys/6a01ade7ce774c0008bbd48f
😎 Deploy Preview	https://deploy-preview-4423--nifty-bassi-e26446.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.