diff --git a/docs/superpowers/specs/2026-05-31-audit-implementation-sequencing.md b/docs/superpowers/specs/2026-05-31-audit-implementation-sequencing.md new file mode 100644 index 0000000..8ef8200 --- /dev/null +++ b/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/-` +(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: ` 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/-` for bug PRs and + `refactor/-` 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. diff --git a/lite_bootstrap/helpers/fastapi_helpers.py b/lite_bootstrap/helpers/fastapi_helpers.py index 2fe2fd4..2972819 100644 --- a/lite_bootstrap/helpers/fastapi_helpers.py +++ b/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", ) diff --git a/tests/test_fastapi_offline_docs.py b/tests/test_fastapi_offline_docs.py index 9b449e2..cf28015 100644 --- a/tests/test_fastapi_offline_docs.py +++ b/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)