# Sprint 15.4: Code Quality & Architectural Debt

> **Size**: L | **Owner**: Full Stack | **Priority**: MEDIUM
> **Phase**: 4.5 - Stabilization
> **Prerequisites**: Sprint 15.1 complete
> **Status**: ✅ COMPLETE

---

## Objective

Address code quality and architectural concerns identified during Phase 4 closeout:
- Oversized mixins exceeding 500 LoC limit
- In-memory state scalability concerns
- CLI print statements instead of proper logging
- Unsafe Rust `.unwrap()` calls in production code
- Domain entity coupling with infrastructure details

---

## Issue Summary

| Issue | Count/Size | Severity | Sprint Priority |
|-------|------------|----------|-----------------|
| Oversized mixins | 2 files | MEDIUM | P0 |
| Print statements in CLI | 13 locations | LOW | P1 |
| In-memory state dicts | 13 dictionaries | HIGH | P2 (design only) |
| Domain entity coupling | 3 fields | MEDIUM | P2 |
| Settings sprawl | 150+ env vars | LOW | P3 |

---

## Task 1: Refactor Oversized Mixins

### Current State

| Mixin | Lines | Limit | Excess |
|-------|-------|-------|--------|
| `streaming.py` | 578 | 500 | +78 |
| `diarization.py` | 562 | 500 | +62 |

### Target: `streaming.py` → Package

**Current Structure** (`streaming.py:578 lines`):

```
StreamingMixin
├── StreamTranscription (main RPC)
├── _init_stream_for_meeting
├── _init_stream_session
├── _process_stream_chunk
├── _process_audio_data
├── _handle_vad_result
├── _flush_segmenter
├── _create_segment_from_partial
├── _persist_segment
├── _cleanup_streaming_state
├── _close_audio_writer
└── Various helper methods
```

**Proposed Structure** (`grpc/_mixins/streaming/`):

```
streaming/
├── __init__.py              # Re-export StreamingMixin
├── _mixin.py                # Main StreamingMixin class (RPC entry points only)
├── _session.py              # Session initialization and management
├── _processing.py           # Audio chunk processing, VAD handling
├── _persistence.py          # Segment persistence logic
├── _cleanup.py              # Resource cleanup utilities
└── _types.py                # Internal types (_StreamSessionInit, etc.)
```

**File: `streaming/_mixin.py`** (~150 lines)

```python
"""StreamingMixin - gRPC streaming entry points."""

from __future__ import annotations

from typing import TYPE_CHECKING, AsyncIterator

import grpc

from noteflow.grpc._mixins.streaming._session import StreamSessionManager
from noteflow.grpc._mixins.streaming._processing import AudioProcessor
from noteflow.grpc._mixins.streaming._cleanup import cleanup_streaming_state
from noteflow.grpc._mixins.protocols import ServicerHost
import noteflow.grpc.proto.noteflow_pb2 as noteflow_pb2

if TYPE_CHECKING:
    pass


class StreamingMixin:
    """Handle bidirectional audio streaming with persistence."""

    async def StreamTranscription(
        self: ServicerHost,
        request_iterator: AsyncIterator[noteflow_pb2.AudioChunk],
        context: grpc.aio.ServicerContext,
    ) -> AsyncIterator[noteflow_pb2.TranscriptUpdate]:
        """Handle bidirectional audio streaming."""
        session_manager = StreamSessionManager(self)
        processor = AudioProcessor(self)

        current_meeting_id: str | None = None

        try:
            async for chunk in request_iterator:
                # Delegate to session manager and processor
                if current_meeting_id is None:
                    current_meeting_id = await session_manager.initialize(
                        chunk.meeting_id, context
                    )
                    if current_meeting_id is None:
                        return

                if self._should_stop(current_meeting_id):
                    break

                async for update in processor.process_chunk(current_meeting_id, chunk):
                    yield update

            # Flush remaining audio
            if current_meeting_id:
                async for update in processor.flush(current_meeting_id):
                    yield update
        finally:
            if current_meeting_id:
                await cleanup_streaming_state(self, current_meeting_id)
```

**File: `streaming/_session.py`** (~120 lines)

```python
"""Stream session initialization and management."""

from __future__ import annotations

import asyncio
from typing import TYPE_CHECKING

import grpc

from noteflow.grpc._mixins.errors import abort_failed_precondition
from noteflow.grpc._mixins.streaming._types import StreamSessionInit

if TYPE_CHECKING:
    from noteflow.grpc._mixins.protocols import ServicerHost


class StreamSessionManager:
    """Manage stream session lifecycle."""

    def __init__(self, host: ServicerHost) -> None:
        self._host = host
        self._init_lock = asyncio.Lock()

    async def initialize(
        self,
        meeting_id: str,
        context: grpc.aio.ServicerContext,
    ) -> str | None:
        """Initialize stream session with race condition protection."""
        async with self._init_lock:
            if meeting_id in self._host._active_streams:
                await abort_failed_precondition(
                    context, f"Meeting {meeting_id} already streaming"
                )
                return None

            self._host._active_streams.add(meeting_id)

        init_result = await self._init_session(meeting_id)
        if not init_result.success:
            self._host._active_streams.discard(meeting_id)
            await context.abort(init_result.error_code, init_result.error_message or "")
            return None

        return meeting_id

    async def _init_session(self, meeting_id: str) -> StreamSessionInit:
        """Initialize audio writer, VAD, segmenter for meeting."""
        # ... existing _init_stream_session logic ...
```

### Target: `diarization.py` → Package

**Proposed Structure** (`grpc/_mixins/diarization/`):

```
diarization/
├── __init__.py              # Re-export DiarizationMixin
├── _mixin.py                # Main DiarizationMixin class (RPCs only)
├── _streaming.py            # Streaming diarization processing
├── _refinement.py           # Offline refinement job logic
├── _jobs.py                 # Job management (start, status, cancel)
├── _speaker.py              # Speaker assignment and renaming
└── _types.py                # Internal types (job status, etc.)
```

**File: `diarization/_mixin.py`** (~100 lines)

```python
"""DiarizationMixin - gRPC diarization entry points."""

from __future__ import annotations

from typing import TYPE_CHECKING

import grpc

from noteflow.grpc._mixins.diarization._jobs import JobManager
from noteflow.grpc._mixins.diarization._speaker import SpeakerManager
from noteflow.grpc._mixins.protocols import ServicerHost
import noteflow.grpc.proto.noteflow_pb2 as noteflow_pb2

if TYPE_CHECKING:
    pass


class DiarizationMixin:
    """Handle speaker diarization operations."""

    async def RefineSpeakerDiarization(
        self: ServicerHost,
        request: noteflow_pb2.RefineSpeakerDiarizationRequest,
        context: grpc.aio.ServicerContext,
    ) -> noteflow_pb2.RefineSpeakerDiarizationResponse:
        """Start background diarization refinement job."""
        job_manager = JobManager(self)
        return await job_manager.start_refinement(request, context)

    async def GetDiarizationJobStatus(
        self: ServicerHost,
        request: noteflow_pb2.GetDiarizationJobStatusRequest,
        context: grpc.aio.ServicerContext,
    ) -> noteflow_pb2.DiarizationJobStatus:
        """Get status of a diarization job."""
        job_manager = JobManager(self)
        return await job_manager.get_status(request, context)

    async def RenameSpeaker(
        self: ServicerHost,
        request: noteflow_pb2.RenameSpeakerRequest,
        context: grpc.aio.ServicerContext,
    ) -> noteflow_pb2.RenameSpeakerResponse:
        """Rename a speaker across all segments."""
        speaker_manager = SpeakerManager(self)
        return await speaker_manager.rename(request, context)
```

---

## Task 2: Replace CLI Print Statements with Logging

### Locations

| File | Line Count | Purpose |
|------|------------|---------|
| `cli/retention.py` | 10 | Status output |
| `cli/models.py` | 15 | Download progress |
| `cli/__main__.py` | 14 | Help text |
| `grpc/_startup.py` | 8 | Server banner |

### Fix Pattern

```python
# BEFORE (cli/retention.py:57-64)
print("\nRetention Cleanup Report:")
print(f"  Meetings checked: {report.meetings_checked}")
print(f"  Meetings deleted: {report.meetings_deleted}")
if report.errors:
    print(f"  Errors: {len(report.errors)}")
    for err in report.errors:
        print(f"    - {err}")

# AFTER
import logging
from rich.console import Console

logger = logging.getLogger(__name__)
console = Console()

def print_cleanup_report(report: CleanupReport) -> None:
    """Print retention cleanup report to console."""
    console.print("\n[bold]Retention Cleanup Report:[/bold]")
    console.print(f"  Meetings checked: {report.meetings_checked}")
    console.print(f"  Meetings deleted: {report.meetings_deleted}")
    if report.errors:
        console.print(f"  [yellow]Errors: {len(report.errors)}[/yellow]")
        for err in report.errors:
            console.print(f"    - {err}")

    # Also log for non-interactive contexts
    logger.info(
        "Retention cleanup: checked=%d deleted=%d errors=%d",
        report.meetings_checked,
        report.meetings_deleted,
        len(report.errors),
    )
```

### Server Banner Pattern

```python
# BEFORE (grpc/_startup.py:342-355)
print(f"\nNoteFlow server running on port {config.port}")
print("Database: Connected" if config.database_url else "Database: Not configured")
# ...

# AFTER
def log_startup_banner(config: ServerConfig, services: ServiceStatus) -> None:
    """Log server startup information."""
    logger.info("NoteFlow server starting on port %d", config.port)
    logger.info("Database: %s", "Connected" if config.database_url else "In-memory")
    logger.info("Diarization: %s", services.diarization_status)
    logger.info("Summarization: %s", services.summarization_status)

    # Rich console for interactive terminals
    if sys.stdout.isatty():
        console = Console()
        console.print(f"\n[bold green]NoteFlow server running on port {config.port}[/bold green]")
        # ...
```

---

## Task 3: Document Scalability Design (For Phase 5)

### Current In-Memory State

**Location**: `src/noteflow/grpc/service.py:124-160`

```python
# 13 dictionaries tracking per-meeting state:
self._audio_writers: dict[str, MeetingAudioWriter] = {}
self._vad_instances: dict[str, StreamingVad] = {}
self._segmenters: dict[str, Segmenter] = {}
self._was_speaking: dict[str, bool] = {}
self._segment_counters: dict[str, int] = {}
self._stream_formats: dict[str, tuple[int, int]] = {}
self._partial_buffers: dict[str, PartialAudioBuffer] = {}
self._diarization_sessions: dict[str, StreamingDiarizationSession] = {}
self._active_streams: set[str] = set()
self._stop_requested: set[str] = set()
self._diarization_jobs: dict[str, DiarizationJob] = {}
self._diarization_tasks: dict[str, asyncio.Task] = {}
self._streaming_diarization_turns: dict[str, list[SpeakerTurn]] = {}
```

### Impact Analysis

| Impact | Description |
|--------|-------------|
| Horizontal scaling | Cannot scale across instances |
| Restart resilience | Loses all in-flight state |
| Memory pressure | Grows with concurrent meetings |
| Upgrade path | No graceful state transfer |

### Proposed Design (Phase 5)

**Option A: Redis-based State**

```python
# grpc/_state/redis_state.py
class RedisStreamingState:
    """Distributed streaming state via Redis."""

    def __init__(self, redis_url: str) -> None:
        self._redis = aioredis.from_url(redis_url)

    async def register_stream(self, meeting_id: str, instance_id: str) -> bool:
        """Atomically register stream ownership."""
        return await self._redis.set(
            f"stream:{meeting_id}:owner",
            instance_id,
            nx=True,  # Only set if not exists
            ex=300,   # 5 minute TTL
        )

    async def heartbeat(self, meeting_id: str, instance_id: str) -> None:
        """Refresh stream ownership TTL."""
        await self._redis.expire(f"stream:{meeting_id}:owner", 300)

    async def get_segment_counter(self, meeting_id: str) -> int:
        """Atomic segment counter."""
        return await self._redis.incr(f"stream:{meeting_id}:segment_counter")
```

**Option B: Database-backed State**

```python
# Use existing database with optimistic locking
# Pros: No additional infrastructure
# Cons: Higher latency for hot-path operations
```

---

## Task 4: Address Domain Entity Coupling (Design Only)

### Current State

**Location**: `src/noteflow/domain/entities/meeting.py:34-36`

```python
@dataclass
class Meeting:
    # ... pure domain fields ...

    # Infrastructure concerns leaked into domain:
    wrapped_dek: bytes | None = None    # Encryption detail
    asset_path: str | None = None       # Storage detail
    version: int = 0                    # ORM optimistic locking
```

### Proposed Separation (Phase 5)

```python
# domain/entities/meeting.py - Pure domain
@dataclass
class Meeting:
    id: MeetingId
    title: str | None
    state: MeetingState
    created_at: datetime
    started_at: datetime | None
    ended_at: datetime | None
    segments: list[Segment]
    summary: Summary | None
    metadata: dict[str, str]

# infrastructure/persistence/models/meeting_storage.py - Storage concerns
@dataclass
class MeetingStorageMetadata:
    """Storage-layer metadata for a meeting."""
    meeting_id: MeetingId
    wrapped_dek: bytes | None
    asset_path: str | None
    version: int
```

---

## Task 5: Rust `.unwrap()` Audit

### Locations Found (in test files - acceptable)

| File | Line | Context |
|------|------|---------|
| `grpc/proto_compliance_tests.rs` | 81 | Test assertions |
| `grpc/client_tests.rs` | 60, 64, 68, 72 | Test assertions |
| `commands/audio_testing.rs` | 235, 247 | Test cleanup |
| `commands/playback_tests.rs` | 18, 22, 26 | Test assertions |

### Analysis

All `.unwrap()` calls found are in **test files**, which is acceptable. No production code contains unsafe unwraps.

### Recommendation

Add clippy lint to CI to prevent future unwrap additions:

```toml
# client/src-tauri/Cargo.toml
[lints.clippy]
unwrap_used = "warn"  # Or "deny" for strict enforcement
```

---

## Tasks Checklist

### Phase A: Mixin Refactoring (Day 1-4)
- [ ] Create `grpc/_mixins/streaming/` package structure
- [ ] Extract session management to `_session.py`
- [ ] Extract audio processing to `_processing.py`
- [ ] Extract cleanup logic to `_cleanup.py`
- [ ] Update imports and tests
- [ ] Verify streaming still works end-to-end
- [ ] Create `grpc/_mixins/diarization/` package structure
- [ ] Extract job management to `_jobs.py`
- [ ] Extract speaker logic to `_speaker.py`
- [ ] Update imports and tests

### Phase B: CLI Logging (Day 5-6)
- [ ] Replace prints in `cli/retention.py`
- [ ] Replace prints in `cli/models.py`
- [ ] Replace prints in `cli/__main__.py`
- [ ] Replace prints in `grpc/_startup.py`
- [ ] Add `rich` for formatted console output
- [ ] Verify CLI commands still work

### Phase C: Documentation (Day 7)
- [ ] Document scalability design for Phase 5
- [ ] Document domain entity separation plan
- [ ] Add Rust clippy lint for unwrap prevention
- [ ] Update CLAUDE.md with new package structure

---

## Acceptance Criteria

### Functional

- [ ] `streaming.py` refactored to package, all files <200 lines
- [ ] `diarization.py` refactored to package, all files <200 lines
- [ ] No `print()` statements in production code (CLI uses logger + rich)
- [ ] All streaming and diarization tests pass

### Quality Gates

- [ ] `ruff check src/noteflow` passes
- [ ] `basedpyright` passes
- [ ] `pytest tests/grpc/` passes
- [ ] `cargo clippy` passes in client/src-tauri

---

## Files Created/Modified

| File | Change Type |
|------|-------------|
| `grpc/_mixins/streaming/__init__.py` | Create |
| `grpc/_mixins/streaming/_mixin.py` | Create |
| `grpc/_mixins/streaming/_session.py` | Create |
| `grpc/_mixins/streaming/_processing.py` | Create |
| `grpc/_mixins/streaming/_persistence.py` | Create |
| `grpc/_mixins/streaming/_cleanup.py` | Create |
| `grpc/_mixins/streaming/_types.py` | Create |
| `grpc/_mixins/diarization/__init__.py` | Create |
| `grpc/_mixins/diarization/_mixin.py` | Create |
| `grpc/_mixins/diarization/_streaming.py` | Create |
| `grpc/_mixins/diarization/_jobs.py` | Create |
| `grpc/_mixins/diarization/_speaker.py` | Create |
| `grpc/_mixins/streaming.py` | Delete |
| `grpc/_mixins/diarization.py` | Delete |
| `cli/retention.py` | Modify |
| `cli/models.py` | Modify |
| `cli/__main__.py` | Modify |
| `grpc/_startup.py` | Modify |
| `docs/architecture/scalability-design.md` | Create |

---

## Rollback Plan

1. **Mixin refactoring**: Keep old files, add deprecation warnings
2. **CLI changes**: Rich is additive, can revert to prints
3. **Documentation**: No runtime impact

---

## Post-Sprint

- Implement Redis-based state for horizontal scaling (Sprint 16+)
- Complete domain entity separation (Sprint 17+)
- Add metrics for mixin method durations
