fix: honor root_path in offline-docs redoc handler

docs/superpowers/specs/2026-05-31-audit-implementation-sequencing.md

@@ -0,0 +1,368 @@
+# Audit Implementation Sequencing
+
+**Date:** 2026-05-31
+**Parent spec:** [2026-05-31-bug-refactor-audit.md](2026-05-31-bug-refactor-audit.md)
+**Scope:** Criticals (CRIT-1..3) + Design issues (DES-1..5) + paired tests (TEST-1/2/3/5/6)
+**Deliverable:** 7 sequenced PRs. Sequencing rationale, per-PR scope, decisions.
+
+This document sequences a subset of the audit findings into 7 small, reviewable
+PRs. Refactor opportunities (REF-*), test gaps not paired with criticals
+(TEST-4/7/8), and low-priority items (LOW-*) are deferred to a future planning
+pass.
+
+---
+
+## Sequencing rationale
+
+PRs are ordered to minimize rebase pain and let later PRs build on the cleaner
+ground established by earlier ones. The two principles:
+
+1. **Criticals first** — highest user impact, smallest diffs, most regression-proof.
+2. **Big refactor last** — DES-1 touches every bootstrapper. Landing it after
+   the bug fixes means it rebases over nothing.
+
+Order:
+
+| # | PR | Findings | Risk | Diff size |
+|---|-----|----------|------|-----------|
+| 1 | Redoc root_path | CRIT-1, TEST-1 | Low | Tiny |
+| 2 | OTel tracer shutdown | CRIT-2, TEST-2 | Low | Small |
+| 3 | Idempotent teardown | CRIT-3, TEST-3 | Low | Tiny |
+| 4 | Small cleanups | DES-4, DES-5 | Very low | Tiny |
+| 5 | Config method semantics | DES-3, TEST-5, TEST-6 | Very low | Small |
+| 6 | OTel service-fields mixin | DES-2 | Low–Medium | Small |
+| 7 | Generic BaseInstrument | DES-1 | Medium | Large |
+
+Each PR merges (squash) to `main` before the next branches off, so PR2 builds
+on PR1's merged state and so on. Branch naming: `fix/<finding-id>-<slug>`
+(e.g. `fix/crit-1-redoc-root-path`).
+
+---
+
+## PR1: CRIT-1 + TEST-1 — Redoc `root_path` fix
+
+**Branch:** `fix/crit-1-redoc-root-path`
+
+**Scope:**
+- Refactor `redoc_html` in `lite_bootstrap/helpers/fastapi_helpers.py` to accept
+  `request: Request` (mirroring `custom_swagger_ui_html`).
+- Read `request.scope.get("root_path", "").rstrip("/")` and prepend it to:
+  - `redoc_js_url` (currently uses bare `{static_path}/redoc.standalone.js`)
+  - `openapi_url` (currently uses bare `app_openapi_url` — also missing the prefix)
+- In `tests/test_fastapi_offline_docs.py::test_fastapi_offline_docs_root_path`,
+  fetch `/custom_docs/../redoc` (or whatever the redoc URL resolves to under
+  `root_path`) and assert `/some-root-path/static/redoc.standalone.js` appears
+  in the response.
+
+**Files:** `lite_bootstrap/helpers/fastapi_helpers.py`,
+`tests/test_fastapi_offline_docs.py`.
+
+**Verification:** `just test` (the new assertion should fail on `main`, pass on branch).
+
+---
+
+## PR2: CRIT-2 + TEST-2 — OTel tracer provider shutdown
+
+**Branch:** `fix/crit-2-otel-shutdown`
+
+**Scope:**
+- In `OpenTelemetryInstrument.bootstrap()`, after creating `tracer_provider`,
+  store it on the instance via `object.__setattr__(self, "_tracer_provider", tracer_provider)`.
+  Declare `_tracer_provider: "TracerProvider | None" = dataclasses.field(default=None, init=False, repr=False, compare=False)`
+  on the dataclass (pattern matches `LoggingInstrument._logger_factory`).
+- In `OpenTelemetryInstrument.teardown()`, after the existing `uninstrument()`
+  loop, call `if self._tracer_provider is not None: self._tracer_provider.shutdown()`
+  and reset to `None`.
+- Add `tests/instruments/test_opentelemetry_instrument.py::test_teardown_shuts_down_tracer_provider`:
+  build an `OpenTelemetryInstrument` with `opentelemetry_log_traces=True`,
+  bootstrap, capture the tracer provider, teardown, assert
+  `tracer_provider.shutdown()` was invoked (via patch or by detecting the
+  shutdown state on the SDK).
+
+**Decision (locked):** Use `object.__setattr__` to preserve `frozen=True`.
+Matches existing precedent. REF-6 will revisit frozen-with-mutable-state
+holistically later.
+
+**Files:** `lite_bootstrap/instruments/opentelemetry_instrument.py`,
+`tests/instruments/test_opentelemetry_instrument.py`.
+
+**Sets up PR3:** without idempotent teardown (PR3), a double teardown would
+call `shutdown()` twice. That's not catastrophic but it would emit warnings.
+Land PR3 close behind.
+
+---
+
+## PR3: CRIT-3 + TEST-3 — Idempotent teardown
+
+**Branch:** `fix/crit-3-idempotent-teardown`
+
+**Scope:**
+- In `lite_bootstrap/bootstrappers/base.py`, modify `BaseBootstrapper.teardown()`:
+
+  ```python
+  def teardown(self) -> None:
+      if not self.is_bootstrapped:
+          return
+      self.is_bootstrapped = False
+      errors: list[tuple[str, BaseException]] = []
+      for one_instrument in reversed(self.instruments):
+          ...
+  ```
+
+  The guard fires first; second teardown returns immediately. This single
+  change fixes Litestar and FastStream simultaneously (both register
+  `self.teardown` on framework shutdown hooks while also being callable
+  manually).
+
+- Add `tests/test_free_bootstrap.py::test_teardown_is_idempotent`: bootstrap,
+  teardown, teardown again, assert no exception and instrument teardowns are
+  not double-invoked.
+
+- Optionally add a Litestar-specific test in `test_litestar_bootstrap.py` that
+  exercises the manual + shutdown-event path explicitly.
+
+**Decision (locked):** Test placement extends `test_free_bootstrap.py` —
+co-located with the existing `test_teardown_error_isolation` and
+`test_teardown_error_aggregates_all_failures`.
+
+**Files:** `lite_bootstrap/bootstrappers/base.py`, `tests/test_free_bootstrap.py`.
+
+---
+
+## PR4: DES-4 + DES-5 — Small cleanups
+
+**Branch:** `fix/des-4-5-small-cleanups`
+
+**Scope:**
+
+DES-4:
+- In `lite_bootstrap/instruments/sentry_instrument.py`, add `"skip_sentry"` to
+  `IGNORED_STRUCTLOG_ATTRIBUTES`.
+- Add a test in `tests/instruments/test_sentry_instrument.py` asserting that
+  when a structlog event includes `skip_sentry=False`, the resulting Sentry
+  `event["contexts"]["structlog"]` does **not** contain the `skip_sentry` key.
+
+DES-5:
+- Delete the dead `and import_checker.is_X_installed` conjunct from `is_ready()`
+  in:
+  - `SentryInstrument.is_ready()` (sentry_instrument.py:100-101)
+  - `LoggingInstrument.is_ready()` (logging_instrument.py:139-140)
+  - `OpenTelemetryInstrument.is_ready()` (opentelemetry_instrument.py:82-86)
+  - `PyroscopeInstrument.is_ready()` (pyroscope_instrument.py:28-29)
+- Commit message documents the invariant: `is_ready` runs only after
+  `check_dependencies` has returned True, so the conjunct is provably
+  redundant.
+
+**Files:** 4 instrument files + 1 test file.
+
+**Risk:** Very low. Pure deletions and a one-line additive change.
+
+---
+
+## PR5: DES-3 + TEST-5 + TEST-6 — Config method semantics
+
+**Branch:** `fix/des-3-config-method-semantics`
+
+**Scope:**
+
+- In `lite_bootstrap/instruments/base.py`, add docstrings to `from_dict` and
+  `from_object` documenting:
+  - `from_dict` includes any key present in the dict (including explicit `None`).
+    Unknown keys are silently dropped.
+  - `from_object` includes any attribute that is **not None**. Attributes set
+    to `None` or missing from the source object fall back to the dataclass
+    default. Falsy non-None values (`False`, `""`, `[]`) are preserved.
+
+- In `tests/test_config.py`, add tests pinning the contract:
+  - `test_from_object_skips_none_attribute` — source has `service_name=None`,
+    result uses the default.
+  - `test_from_object_preserves_falsy_values` — source has `service_debug=False`,
+    result has `service_debug=False`.
+  - `test_from_object_missing_attribute` — source object lacks an attribute
+    entirely (e.g. plain object with only `service_name`), result has defaults
+    for everything else.
+  - `test_from_dict_drops_unknown_keys_silently` — explicitly assert no
+    exception is raised when unknown keys are present.
+
+**Decision (locked):** Keep the current "skip None" behavior for `from_object`.
+Minimal change. Preserves any user code that already depends on it. Document
+and pin with tests.
+
+**Files:** `lite_bootstrap/instruments/base.py`, `tests/test_config.py`.
+
+---
+
+## PR6: DES-2 — OpenTelemetry service-fields mixin
+
+**Branch:** `fix/des-2-otel-fields-mixin`
+
+**Scope:**
+
+- In `lite_bootstrap/instruments/opentelemetry_instrument.py`, declare a small
+  mixin dataclass at module scope (above `OpentelemetryConfig`):
+
+  ```python
+  @dataclasses.dataclass(kw_only=True, frozen=True)
+  class OpenTelemetryServiceFieldsConfig(BaseConfig):
+      opentelemetry_service_name: str | None = None
+      opentelemetry_namespace: str | None = None
+  ```
+
+- Change `OpentelemetryConfig` to inherit from
+  `OpenTelemetryServiceFieldsConfig` instead of `BaseConfig`. Remove the two
+  duplicate field declarations.
+
+- In `lite_bootstrap/instruments/pyroscope_instrument.py`, import
+  `OpenTelemetryServiceFieldsConfig` and have `PyroscopeConfig` inherit from
+  it instead of `BaseConfig`. Remove the two duplicate field declarations.
+
+- Verify MRO still works in `FreeBootstrapperConfig`,
+  `FastAPIConfig`, `LitestarConfig`, `FastStreamConfig` — all of which inherit
+  from both `OpentelemetryConfig` and `PyroscopeConfig`. With the shared mixin,
+  the diamond resolves cleanly: the mixin is the single source of truth for
+  these fields.
+
+- Run `just test` to confirm no behavior change. The
+  `test_pyroscope_standalone_config_accepts_otel_fields` test in
+  `tests/instruments/test_pyroscope_instrument.py` will exercise the new
+  inheritance path.
+
+**Decision (locked):** Inline the mixin in `opentelemetry_instrument.py` rather
+than create a new file. The mixin is small; pyroscope already imports
+otel-adjacent symbols.
+
+**Files:** `lite_bootstrap/instruments/opentelemetry_instrument.py`,
+`lite_bootstrap/instruments/pyroscope_instrument.py`.
+
+**Risk:** Dataclass MRO is finicky. Full test run after the change is
+non-negotiable.
+
+---
+
+## PR7: DES-1 — Generic `BaseInstrument[ConfigT]`
+
+**Branch:** `refactor/des-1-generic-instruments`
+
+**Scope:**
+
+- In `lite_bootstrap/instruments/base.py`, introduce a type variable and make
+  `BaseInstrument` generic:
+
+  ```python
+  ConfigT = typing.TypeVar("ConfigT", bound=BaseConfig)
+
+  @dataclasses.dataclass(kw_only=True, slots=True, frozen=True)
+  class BaseInstrument(typing.Generic[ConfigT]):
+      bootstrap_config: ConfigT
+      ...
+  ```
+
+- Update each instrument's class declaration to parameterize the base:
+  - `LoggingInstrument(BaseInstrument[LoggingConfig])`
+  - `SentryInstrument(BaseInstrument[SentryConfig])`
+  - `OpenTelemetryInstrument(BaseInstrument[OpentelemetryConfig])`
+  - `PyroscopeInstrument(BaseInstrument[PyroscopeConfig])`
+  - `PrometheusInstrument(BaseInstrument[PrometheusConfig])`
+  - `HealthChecksInstrument(BaseInstrument[HealthChecksConfig])`
+  - `CorsInstrument(BaseInstrument[CorsConfig])`
+  - `SwaggerInstrument(BaseInstrument[SwaggerConfig])`
+
+- Delete pure-annotation framework subclasses. Concrete deletion list (from a
+  read of the three bootstrappers):
+
+  - `FastAPILoggingInstrument` — pure annotation, delete.
+  - `FastAPISentryInstrument` — pure annotation, delete.
+  - `LitestarSentryInstrument` — pure annotation, delete.
+  - `FastStreamSentryInstrument` — pure annotation, delete.
+
+- Keep framework subclasses that **do** real work — but remove their now-redundant
+  `bootstrap_config: FastAPIConfig` field annotations (the generic parameter
+  handles it):
+
+  - `FastAPICorsInstrument` — overrides `bootstrap()`. Keep, drop annotation.
+  - `FastAPIHealthChecksInstrument` — overrides `bootstrap()`. Keep, drop annotation.
+  - `FastAPIOpenTelemetryInstrument` — overrides `bootstrap()` and `teardown()`,
+    has `_build_excluded_urls`. Keep, drop annotation.
+  - `FastAPIPrometheusInstrument` — overrides `check_dependencies()` and
+    `bootstrap()`. Keep, drop annotation.
+  - `FastAPISwaggerInstrument` — overrides `bootstrap()`. Keep, drop annotation.
+  - `LitestarCorsInstrument` — overrides `bootstrap()`. Keep, drop annotation.
+  - `LitestarHealthChecksInstrument` — overrides `bootstrap()`. Keep, drop annotation.
+  - `LitestarLoggingInstrument` — overrides `bootstrap()`. Keep, drop annotation.
+  - `LitestarOpenTelemetryInstrument` — overrides `bootstrap()`. Keep, drop annotation.
+  - `LitestarPrometheusInstrument` — overrides `check_dependencies()` and
+    `bootstrap()`. Keep, drop annotation.
+  - `LitestarSwaggerInstrument` — overrides `is_ready()` and `bootstrap()`. Keep, drop annotation.
+  - `FastStreamHealthChecksInstrument` — overrides `bootstrap()`. Keep, drop annotation.
+  - `FastStreamLoggingInstrument` — overrides `bootstrap()`. Keep, drop annotation.
+  - `FastStreamOpenTelemetryInstrument` — overrides `is_ready()` and `bootstrap()`. Keep, drop annotation.
+  - `FastStreamPrometheusInstrument` — overrides `is_ready()`, `check_dependencies()`,
+    has its own collector_registry field, `bootstrap()`. Keep, drop annotation.
+
+- In `instruments_types` ClassVar lists per bootstrapper, replace deleted
+  pure-annotation subclasses with the base generic forms:
+  `LoggingInstrument`, `SentryInstrument`, etc. Where the kept subclass exists
+  (e.g. `FastAPICorsInstrument`), keep it.
+
+- Run `just lint` (includes `ty check`) and `just test`. The type checker is
+  the primary safety net for this refactor.
+
+**Decision (locked):** Per-framework subclasses that do real work lose their
+redundant `bootstrap_config: <FrameworkConfig>` annotation. The generic
+parameter provides the type.
+
+**Linked findings deferred:** REF-1 (`_build_excluded_urls` duplication) and
+REF-2 (dead defensive check in `LitestarLoggingInstrument`) become trivial
+follow-ups after this lands. REF-5 (collapse of empty base instruments) is
+also adjacent. Whether to fold them into this PR depends on diff size:
+
+- If after PR7's core changes the diff is under ~300 lines, fold REF-1 and
+  REF-2 in (REF-5 is cosmetic and can wait).
+- Otherwise, land REF-1/2 as a quick PR8 immediately after.
+
+Decide once PR7's actual diff exists, not now.
+
+**Files:** every file in `lite_bootstrap/instruments/`, every bootstrapper.
+Largest diff in the sequence.
+
+---
+
+## Branch hygiene & CI
+
+- One branch per PR off `main`, name `fix/<finding-id>-<slug>` for bug PRs and
+  `refactor/<finding-id>-<slug>` for PR7.
+- Each PR runs `just lint-ci` and `just test` via existing CI.
+- PR7 additionally requires manual confirmation that `ty check` passes locally
+  (the strict-mode type check is the only enforcement that the generic
+  parameterization is wired correctly).
+- For PR2 and PR3, the new tests must fail on `main` and pass on the branch.
+  This is regression-proof.
+- Squash-merge each PR before the next branches off. This keeps the linear
+  history clean and matches the project's recent commit pattern (`fix:`,
+  `feat:`, `chore:` prefixes).
+
+---
+
+## Locked decisions
+
+| Decision | Choice |
+|----------|--------|
+| PR2 storage pattern | `object.__setattr__` (matches existing pattern) |
+| PR5 `from_object` semantics | Keep "skip None", document, pin with tests |
+| PR6 mixin location | Inline in `opentelemetry_instrument.py` |
+| PR3 test placement | Extend `test_free_bootstrap.py` |
+| PR7 annotation cleanup | Drop redundant `bootstrap_config:` annotations on kept subclasses |
+
+---
+
+## Deferred (out of scope for this plan)
+
+- REF-1, REF-2, REF-3, REF-4, REF-5, REF-6, REF-7 — refactor opportunities.
+  REF-1/2/5 are likely follow-ups to PR7. REF-3/4/6/7 are independent and can
+  be planned later.
+- TEST-4 (standalone instrument tests), TEST-7 (`is_valid_path` negatives),
+  TEST-8 (logging lifecycle replay) — non-critical test gaps.
+- LOW-1 through LOW-9 — cosmetic / minor.
+
+These are intentionally left for a future planning pass once the criticals and
+high-priority design issues have shipped.

lite_bootstrap/helpers/fastapi_helpers.py

@@ -50,9 +50,10 @@ async def swagger_ui_redirect() -> HTMLResponse:
         return get_swagger_ui_oauth2_redirect_html()
 
     @app.get(redoc_url, include_in_schema=False)
-    async def redoc_html() -> HTMLResponse:
+    async def redoc_html(request: Request) -> HTMLResponse:
+        root_path = request.scope.get("root_path", "").rstrip("/")
         return get_redoc_html(
-            openapi_url=app_openapi_url,
+            openapi_url=f"{root_path}{app_openapi_url}",
             title=f"{app.title} - ReDoc",
-            redoc_js_url=f"{static_path}/redoc.standalone.js",
+            redoc_js_url=f"{root_path}{static_path}/redoc.standalone.js",
         )

tests/test_fastapi_offline_docs.py

@@ -42,6 +42,11 @@ def test_fastapi_offline_docs_root_path() -> None:
         response = client.get("/some-root-path/static/swagger-ui.css")
         assert response.status_code == HTTPStatus.OK
 
+        response = client.get("/redoc")
+        assert response.status_code == HTTPStatus.OK
+        assert "/some-root-path/static/redoc.standalone.js" in response.text
+        assert "/some-root-path/openapi.json" in response.text
+
 
 def test_fastapi_offline_docs_raises_without_openapi_url() -> None:
     app = FastAPI(openapi_url=None)