|
| 1 | +# Clean Code and SRP Audit |
| 2 | + |
| 3 | +## Summary |
| 4 | + |
| 5 | +- **Highest-leverage split:** remove HTTP transport/session/error handling from |
| 6 | + the 500-line `Admin` class while keeping its public methods and Kafka admin |
| 7 | + lifecycle intact. |
| 8 | +- `Admin` changes for Kafka topic/group APIs, REST transport, cluster |
| 9 | + observability, message inspection, metrics history, and Moonshot branches. |
| 10 | +- `Consumer` mixes Kafka group consumption with a second semantic-search HTTP |
| 11 | + client and duplicates the dedicated `search.py` model/client. |
| 12 | +- Duplicate topic/group dataclasses in `admin.py` and `types.py` have already |
| 13 | + diverged; consolidating exported types requires a compatibility decision and |
| 14 | + is deferred. |
| 15 | +- Telemetry, serializers, retry, and circuit-breaker modules are long but each |
| 16 | + has one actor and should remain independent. |
| 17 | + |
| 18 | +## Findings |
| 19 | + |
| 20 | +| ID | Location | Category | Severity | Actors in conflict | Cost | Size | Behavior risk | |
| 21 | +|---|---|---|---|---|---|---|---| |
| 22 | +| PY-SRP-1 | `streamline_sdk/admin.py:229-753` | SRP, mixed class | P1 | Kafka administrators; HTTP transport; SRE/inspection; branch product | A transport/auth/error change and a topic/group behavior change edit one public lifecycle class. | L | Medium | |
| 23 | +| PY-SRP-2 | `streamline_sdk/consumer.py:55-554` | SRP, mixed class | P2 | Kafka consumer groups; semantic-search HTTP API | Search dependencies/routes/error parsing live in the stateful Kafka consumer and duplicate `search.py`. | M | Medium | |
| 24 | +| PY-CC-1 | `streamline_sdk/admin.py:24-226`; `streamline_sdk/types.py` | Duplication with drift | P2 | Admin public API; shared model consumers | Topic/group/query model copies differ in fields/defaults, so replacing one copy can break imports or state shape. | M | High | |
| 25 | +| PY-CC-2 | `streamline_sdk/admin.py:748-753` | Dead comments | P2 | maintainers | Stale implementation notes imply features that are already implemented elsewhere and obscure the real end of the class. | S | None | |
| 26 | + |
| 27 | +## Actor and State Partition |
| 28 | + |
| 29 | +### `Admin` |
| 30 | + |
| 31 | +| Partition | Methods/state | Actor/axis | |
| 32 | +|---|---|---| |
| 33 | +| Kafka lifecycle | `_admin`, `start`, `close`, topic/group Kafka calls | Kafka administrators | |
| 34 | +| HTTP transport | `_http_get`, `_http_post`, `_http_delete`, aiohttp/urllib fallback | transport/platform | |
| 35 | +| Cluster/lag/inspection | cluster, lag, message inspection, metrics mapping | SRE/tooling | |
| 36 | +| Branches | create/list/discard branch | Moonshot product | |
| 37 | + |
| 38 | +Resulting internal unit: `_AdminHttpTransport`, owning URL construction, |
| 39 | +aiohttp/urllib fallback, timeout, status mapping, and JSON decoding. `Admin` |
| 40 | +retains public methods, Kafka state, and response mapping decisions. |
| 41 | + |
| 42 | +### `Consumer` |
| 43 | + |
| 44 | +Kafka state (`_consumer`, subscription, offsets, polling, iteration) is |
| 45 | +independent of semantic-search HTTP request construction. The search behavior |
| 46 | +should use the existing `SearchClient`/`SearchHit` implementation where its |
| 47 | +route and response contract are equivalent; otherwise the difference must be |
| 48 | +reported rather than silently normalized. |
| 49 | + |
| 50 | +## Ordered Refactor Sequence |
| 51 | + |
| 52 | +1. Characterize Admin HTTP paths, methods, status errors, aiohttp and urllib |
| 53 | + fallback behavior. |
| 54 | +2. Move HTTP transport unchanged into `_AdminHttpTransport`. |
| 55 | +3. Remove stale end-of-file comments and keep response mapping in `Admin`. |
| 56 | +4. Characterize Consumer semantic search against the dedicated search client. |
| 57 | +5. Reuse one search response decoder/model only where tests prove behavior |
| 58 | + equivalence. |
| 59 | +6. Run the Python 3.9/3.12 test, Ruff, mypy, and package-build matrix after |
| 60 | + every commit. |
| 61 | + |
| 62 | +## Deferred |
| 63 | + |
| 64 | +- Canonical `/v1` versus `/api/v1` routes require the org HTTP contract |
| 65 | + decision; this refactor preserves existing Python paths. |
| 66 | +- Public model consolidation between `admin.py`, `types.py`, `query.py`, and |
| 67 | + `search.py` requires a deprecation/version plan. |
| 68 | +- Live integration and conformance tests require the external server image. |
| 69 | + |
| 70 | +## Out of Scope |
| 71 | + |
| 72 | +- `telemetry.py`: one observability actor. |
| 73 | +- `producer.py`: one producer/transaction lifecycle and shared producer state. |
| 74 | +- `serializers.py`: schema serialization actor; format classes are intentionally |
| 75 | + separate. |
| 76 | +- `retry.py` and `circuit_breaker.py`: separate reliability policies with |
| 77 | + distinct state machines. |
0 commit comments