From f596464f3f3b21c4ccadca06fd0ad5b2e9963480 Mon Sep 17 00:00:00 2001 From: claude Date: Tue, 2 Jun 2026 05:08:59 +0000 Subject: [PATCH] =?UTF-8?q?docs(prd):=20add=20PRD=200031=20=E2=80=94=20spl?= =?UTF-8?q?it=20=5Fmerge=5Fprovider=5Froute=20into=20named=20case=20helper?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/prds/0031-split-merge-provider-route.md | 173 +++++++++++++++++++ 1 file changed, 173 insertions(+) create mode 100644 docs/prds/0031-split-merge-provider-route.md diff --git a/docs/prds/0031-split-merge-provider-route.md b/docs/prds/0031-split-merge-provider-route.md new file mode 100644 index 0000000..3d4c2b6 --- /dev/null +++ b/docs/prds/0031-split-merge-provider-route.md @@ -0,0 +1,173 @@ +# PRD 0031: Split `_merge_provider_route` into named case helpers + +- **Status:** Draft +- **Author:** didericis-claude +- **Created:** 2026-06-02 +- **Issue:** #120 + +## Summary + +Refactor `_merge_provider_route` in `bot_bottle/egress.py` to replace its +five-outcome nested conditional with a top-level host-lookup dispatch and +one named private function per outcome. No behaviour change. + +## Problem + +`_merge_provider_route` handles five distinct cases in a single function: + +1. **append-new** — host not in manifest; append a fresh route. +2. **upgrade-bare** — host found, no existing auth; adopt provider auth. +3. **no-op** — host found, existing auth matches provider auth exactly; + return unchanged. +4. **tls-passthrough upgrade** — same as no-op but provider sets + `tls_passthrough=True` and the existing route doesn't; flip the flag. +5. **conflict-die** — host found, existing auth differs from provider; + hard error. + +These cases are currently identified by interleaved `if`/`continue` +conditions rather than named dispatch. The control flow is: + +``` +for idx, route in enumerate(routes): # host lookup + if route.host != pr.host: continue + if route.auth_scheme or route.token_ref: # already-authed branch + if same auth: + if tls upgrade needed: replace # case 4 + return # case 3 + die(...) # case 5 + [token_env alloc] + routes[idx] = ... # case 2 + return routes +[token_env alloc] +routes.append(...) # case 1 +return routes +``` + +Specific problems: + +- Cases 3 and 4 share the same `if same auth:` block; case 3 is the + implicit fall-through after the inner `if`, making it invisible by + name. +- `_find_or_alloc_token_env` is called twice with identical arguments in + the upgrade-bare path (case 2) and the append-new path (case 1). +- Adding a sixth case requires reading the whole function to find the + right insertion point, with no structural hint about case boundaries. +- The in-place index write (`routes[idx] = EgressRoute(...)`) spells out + every field explicitly; a new field added to `EgressRoute` silently + drops its value on any in-place replacement that wasn't updated. + +## Goals / Success Criteria + +- Each of the five outcomes is implemented in its own named private + function with a docstring stating the precondition. +- `_merge_provider_route` reads as a dispatch table: find the host, then + call the right helper. +- In-place replacements use `dataclasses.replace` instead of full + constructor calls. +- All existing `TestProviderRouteMerge` tests pass without modification. +- No behaviour change. + +## Non-goals + +- Changing the public API of `egress_routes_for_bottle` or + `egress_manifest_routes`. +- Changing merge semantics (what counts as a conflict, what upgrade rules + apply). +- Consolidating any other complexity in `egress.py`. + +## Design + +### Dispatch structure + +```python +def _merge_provider_route( + routes: list[EgressRoute], pr: EgressRoute, +) -> list[EgressRoute]: + for idx, route in enumerate(routes): + if route.host.lower() == pr.host.lower(): + return _merge_at_index(routes, idx, route, pr) + return _append_provider_route(routes, pr) +``` + +### Per-case helpers + +**`_merge_at_index`** — dispatches on whether the existing route already +carries auth: + +```python +def _merge_at_index( + routes: list[EgressRoute], idx: int, route: EgressRoute, pr: EgressRoute, +) -> list[EgressRoute]: + if route.auth_scheme or route.token_ref: + return _merge_authed(routes, idx, route, pr) + return _upgrade_bare(routes, idx, route, pr) +``` + +**`_merge_authed`** — existing route has auth; either no-op/tls-upgrade +or conflict-die: + +```python +def _merge_authed( + routes: list[EgressRoute], idx: int, route: EgressRoute, pr: EgressRoute, +) -> list[EgressRoute]: + """Precondition: route.auth_scheme or route.token_ref is set.""" + if route.auth_scheme != pr.auth_scheme or route.token_ref != pr.token_ref: + die(...) # case 5 + if pr.tls_passthrough and not route.tls_passthrough: + routes[idx] = dataclasses.replace(route, tls_passthrough=True) # case 4 + return routes # case 3 (implicit no-op) +``` + +**`_upgrade_bare`** — existing route has no auth; adopt provider auth +and preserve `path_allowlist` + `roles`: + +```python +def _upgrade_bare( + routes: list[EgressRoute], idx: int, route: EgressRoute, pr: EgressRoute, +) -> list[EgressRoute]: + """Precondition: route has no auth_scheme or token_ref.""" + token_env = _find_or_alloc_token_env(routes, pr.token_ref) if pr.auth_scheme else "" + routes[idx] = dataclasses.replace( + route, + auth_scheme=pr.auth_scheme, + token_env=token_env, + token_ref=pr.token_ref, + tls_passthrough=pr.tls_passthrough, + ) + return routes +``` + +**`_append_provider_route`** — host not in manifest; append a new route +(no `path_allowlist` or `roles` to preserve): + +```python +def _append_provider_route( + routes: list[EgressRoute], pr: EgressRoute, +) -> list[EgressRoute]: + """Precondition: no existing route matches pr.host.""" + token_env = _find_or_alloc_token_env(routes, pr.token_ref) if pr.auth_scheme else "" + routes.append(dataclasses.replace(pr, token_env=token_env)) + return routes +``` + +Note: `dataclasses.replace(pr, token_env=token_env)` preserves any +future fields added to `EgressRoute` without requiring updates here. + +### Import + +Add `import dataclasses` to `egress.py` (currently uses the `dataclass` +decorator from `dataclasses` but not `dataclasses.replace`). + +## Implementation chunks + +1. **PRD (this commit).** Sets the design. +2. **Refactor.** Apply the dispatch + four helpers in `egress.py`; add + `dataclasses` import; delete the old `_merge_provider_route` body. +3. **Tests.** Existing `TestProviderRouteMerge` suite is the acceptance + gate — no new tests needed beyond confirming all pass. + +## References + +- Issue #120: Refactor `_merge_provider_route`. +- Issue #117: Complexity hotspots — source of the finding. +- PRD 0030: Deduplicate egress token resolution (prior egress cleanup).