diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index 06661fe..7710512 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -1,6 +1,6 @@ # DocuVault — v1 Roadmap -_Last updated: 2026-05-31_ +_Last updated: 2026-06-02_ ## Mandatory Cross-Cutting Gates (every phase) @@ -385,13 +385,62 @@ Before any phase is marked complete, all three gates must pass: ### Phase 7: Redo and optimize LLM integration -**Goal:** [To be planned] -**Requirements**: TBD -**Depends on:** Phase 6 -**Plans:** 0 plans +**Goal:** A fully refactored, production-reliable AI provider layer: AI provider settings live in a new `system_settings` DB table (replacing env-only config), API keys encrypted at rest via HKDF/AES-GCM (same pattern as cloud credentials), all OpenAI-compatible providers (Groq, xAI, DeepSeek, OpenRouter, Gemini-compat, Mistral-compat, Ollama, LMStudio) handled by a single `GenericOpenAIProvider` class, Anthropic uses native `output_config.format.type="json_schema"` structured output, JSON-mode enforced everywhere with `parse_classification()` as fallback, Celery exponential-backoff retry (30s/90s/270s) replaces silent failure on classification errors, per-provider context window size with smart 60/40 truncation replaces the global `MAX_AI_CHARS`, the singleton `_client` pattern restores httpx connection-pool reuse, an admin AI Providers panel allows interactive configuration plus test-connection, and a "Re-analyze" button on the document card re-queues failed classifications. +**Mode:** standard +**Depends on:** Phase 6.2 +**Requirements**: D-01..D-18 (CONTEXT.md decisions; no REQ-IDs yet mapped — phase introduces new infrastructure) -Plans: -- [ ] TBD (run /gsd-plan-phase 7 to break down) +**Success Criteria** (what must be TRUE): + +1. AI provider settings live in `system_settings`; admin can view, edit, and atomically activate any provider via `PUT /api/admin/ai-config`; `GET /api/admin/ai-config` never returns `api_key_enc` or any decrypted key value +2. A document whose classification raises an exception is automatically retried by Celery at 30 s, 90 s, and 270 s; after the third failure the document's status remains `classification_failed` and no further retries occur +3. All OpenAI-compatible providers route through `GenericOpenAIProvider`; classify/suggest pass `response_format={"type":"json_object"}` when `supports_json_mode` is True (Gemini preset is False and falls back to `parse_classification()`); Anthropic uses `output_config.format.type="json_schema"` with a constrained schema +4. `MAX_AI_CHARS` no longer exists in `openai_provider.py`, `anthropic_provider.py`, or `classifier.py`; each provider truncates input text using its own `context_chars` value via a 60% head + 40% tail strategy +5. A failed document shows a red "Classification failed" badge and a "Re-analyze" button on the document card; clicking the button calls `POST /api/documents/{id}/classify`, which sets the document to `processing` and re-queues the Celery task + +**Plans**: 5 plans (5 waves) + +**Wave 1** — Foundation: migration, ORM model, encryption helpers, Wave 0 test stubs + +- [ ] 07-01-PLAN.md — Alembic migration 0005 (system_settings table) + SystemSettings ORM model + services/ai_config.py (HKDF helpers + load_provider_config + env seed on startup) + Wave 0 xfail stubs for D-01..D-16 (test_ai_providers.py, test_ai_config.py, test_admin_ai_config.py, test_document_tasks.py) + +**Wave 2** *(blocked on Wave 1)* — Provider layer: ProviderConfig, GenericOpenAIProvider, singleton client fix, registry + +- [ ] 07-02-PLAN.md — ProviderConfig Pydantic model + PROVIDER_DEFAULTS + SUPPORTS_JSON_MODE + GenericOpenAIProvider(OpenAIProvider) + OpenAIProvider singleton refactor + ollama/lmstudio context_chars + MAX_AI_CHARS removal from openai_provider.py + classifier.py + registry-based ai/__init__.py get_provider(config: ProviderConfig) + anthropic>=0.95.0 pin + +**Wave 3** *(blocked on Wave 2)* — Anthropic + Classifier wiring + +- [ ] 07-03-PLAN.md — AnthropicProvider singleton + output_config json_schema + smart truncation + MAX_AI_CHARS removal + classifier.classify_document driven by load_provider_config (no inline _settings dict) + ai_config stub removed in favor of real ProviderConfig + load_provider_config_by_id helper + +**Wave 4** *(blocked on Wave 3)* — Celery retry harness + Re-queue endpoint + +- [ ] 07-04-PLAN.md — Celery extract_and_classify decorator gains bind=True + max_retries=3 + _ClassificationError sentinel + 30/90/270 backoff + _mark_classification_failed on exhaustion + POST /api/documents/{id}/classify changes to re-queue Celery (sets status=processing, calls .delay()) — promotes test_retry_backoff, test_exhaustion_sets_failed_status, test_reclassify_requeues_celery + +**Wave 5** *(blocked on Wave 4)* — Admin UI + Frontend badge + +- [ ] 07-05-PLAN.md — Admin AI-config backend (GET/PUT/test-connection with whitelist + audit logging + atomic is_active flip) + frontend API client helpers (getAiConfig/saveAiConfig/testAiConnection) + AdminAiConfigTab.vue global System AI Providers section ABOVE existing per-user table + DocumentCard.vue classification_failed badge + Re-analyze button + human checkpoint UAT + +**Cross-cutting constraints:** + +- `api_key_enc` is never returned by any admin endpoint — enforced by `_ai_config_to_dict()` whitelist (Plan 05) +- HKDF domain separation: AI settings use `info=b"ai-provider-settings"`, cloud credentials use `info=b"cloud-credentials"` (Plan 01) +- `is_active` flip is atomic: single `UPDATE SET is_active = (provider_id = $target)` — never read-then-write (Plan 05) +- Provider clients are stored as `self._client` in `__init__` — never recreated per call (Plans 02, 03) +- `MAX_AI_CHARS` removal must cover all three locations: openai_provider.py L5, anthropic_provider.py L5, classifier.py L28 (Plans 02 + 03) +- Celery `self.retry()` must be raised from the outer sync task body, never inside `asyncio.run()` (Plan 04 — Pitfall 3) +- `extra_hosts: ["host.docker.internal:host-gateway"]` already present in docker-compose.yml — D-14 is already done; no docker-compose changes required (RESEARCH.md) +- AdminAiConfigTab.vue per-user assignment table is preserved untouched; the new global system section is added ABOVE it (Plan 05 — Pitfall 6) + +**Phase gates (must pass before Phase 7 is complete):** + +- [ ] `pytest -v` — zero failures; D-01/D-03/D-05/D-06/D-07/D-09/D-10/D-11/D-12/D-13/D-16 covered by promoted tests +- [ ] Security agent: bandit -r backend/ (zero HIGH), pip audit (zero critical/high), npm audit --audit-level=high (zero high/critical) +- [ ] `_ai_config_to_dict()` confirmed to never include `api_key_enc` (integration test asserts response.text does not contain the key) +- [ ] HKDF domain separation verified: same master key + same plaintext produces different ciphertexts for `provider_id` vs `user_id` derivations +- [ ] Atomic `is_active` flip verified: after two PUTs with `is_active=true` on different providers, exactly one row has `is_active=true` +- [ ] Human checkpoint UAT approves admin panel + DocumentCard badge end-to-end + +**UI hint**: yes --- @@ -407,3 +456,4 @@ Plans: | 6. Performance & Production Hardening | 0/TBD | Not started | — | | 6.1. Close v1.0 audit gaps | 2/2 | Complete | 2026-05-30 | | 6.2. Close v1 sharing + cloud-delete + CSV export gaps | 5/5 | Complete | 2026-05-31 | +| 7. Redo and optimize LLM integration | 0/5 | Planned | — | diff --git a/.planning/STATE.md b/.planning/STATE.md index be293a8..ed599c8 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -1,24 +1,24 @@ --- gsd_state_version: 1.0 milestone: v1.0 -milestone_name: "Performance & Production Hardening" -current_phase: "06" -status: planned -last_updated: "2026-06-02T00:00:00.000Z" +milestone_name: "audit gaps: SHARE-02/STORE-06/ADMIN-06" +current_phase: 7 +status: executing +last_updated: "2026-06-03T16:24:52.248Z" progress: - total_phases: 1 - completed_phases: 0 - total_plans: 6 - completed_plans: 0 - percent: 0 + total_phases: 9 + completed_phases: 7 + total_plans: 55 + completed_plans: 44 + percent: 78 --- # Project State **Project:** DocuVault -**Status:** Executing Phase 06.2 -**Current Phase:** 06.2 -**Last Updated:** 2026-05-28 +**Status:** Phase 7 Planned — Ready to Execute +**Current Phase:** 7 +**Last Updated:** 2026-06-03 ## Phase Status @@ -32,14 +32,15 @@ progress: | 6 | Performance & Production Hardening | Not started | | 6.1 | Close v1.0 audit gaps: SHARE-02/STORE-06/ADMIN-06 | ✓ Complete (2/2 plans) | | 6.2 | Close v1 sharing + cloud-delete + CSV export gaps | ✓ Complete (5/5 plans, UAT passed, security gate passed) | +| 7 | Redo and Optimize LLM Integration | Planned (5/5 plans, verification passed) | ## Current Position -Phase: 06.2 (close-v1-sharing-cloud-delete-csv-export-gaps) — EXECUTING -Plan: 1 of 5 -**Phase:** 05-cloud-storage-backends — Complete (12/12 plans, all UAT gaps resolved) -**Plan:** 05-12 — complete -**Progress:** [██████████] 100% +Phase: 7 (redo-and-optimize-llm-integration) — PLANNED +Plan: 0 of 5 +**Phase:** 07-redo-and-optimize-llm-integration — Ready to execute (5 plans, verification passed 2026-06-03) +**Plan:** 07-01 — next +**Progress:** [__________] 0% ## Performance Metrics @@ -201,6 +202,7 @@ _Updated at each phase transition._ | Last session | 2026-05-30 — Plan 05-12 executed: OAuth 400 preflight (unconfigured creds), 502 cloud fallback, celery-worker volume mount, upload hint in CloudStorageView; 293 passed / 24 xfailed / 1 pre-existing failure | | Last session | 2026-05-30 — Phase 6.1 executed: 7 share tests + 4 audit tests promoted from xfail stubs; second_auth_user fixture added; 309 passed / 0 failed | | Last session | 2026-05-31 — Phase 6.2 planned: 4 plans (3 waves); SHARE-03/SHARE-05 (Plan 02), cloud-delete (Plan 03), ADMIN-06 audit enrichment + CSV + daily exports (Plan 04); verification passed (0 blockers, 2 cosmetic warnings fixed) | -| Next action | Milestone v1.0 complete — run /gsd:complete-milestone or start Phase 6 (Performance & Production Hardening) | +| Last session | 2026-06-03 — Phase 7 planned: 5 plans (5 waves); system_settings DB + HKDF (Plan 01), ProviderConfig + GenericOpenAIProvider + singleton fix (Plan 02), Anthropic output_config + classifier wiring (Plan 03), Celery retry harness + re-queue endpoint (Plan 04), admin AI panel + DocumentCard badge (Plan 05); verification passed (4 blockers fixed in revision round, 0 remaining) | +| Next action | Execute Phase 7 — run /gsd:execute-phase 7 | | Pending decisions | None | | Resume file | None | diff --git a/.planning/phases/07-redo-and-optimize-llm-integration/.gitkeep b/.planning/phases/07-redo-and-optimize-llm-integration/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/.planning/phases/07-redo-and-optimize-llm-integration/07-01-PLAN.md b/.planning/phases/07-redo-and-optimize-llm-integration/07-01-PLAN.md new file mode 100644 index 0000000..4b6440e --- /dev/null +++ b/.planning/phases/07-redo-and-optimize-llm-integration/07-01-PLAN.md @@ -0,0 +1,295 @@ +--- +phase: 07-redo-and-optimize-llm-integration +plan: 01 +type: execute +wave: 1 +depends_on: [] +files_modified: + - backend/migrations/versions/0005_system_settings.py + - backend/db/models.py + - backend/services/ai_config.py + - backend/main.py + - backend/tests/test_ai_providers.py + - backend/tests/test_ai_config.py + - backend/tests/test_admin_ai_config.py + - backend/tests/test_document_tasks.py +autonomous: true +requirements: + - D-04 + - D-05 + - D-07 + - D-09 + - D-10 + - D-11 + - D-12 + - D-13 + - D-14 # pre-satisfied — extra_hosts already present in docker-compose.yml; this plan adds a regression-guard grep in + - D-16 + +must_haves: + truths: + - "Wave 0 test scaffold files exist with xfail stubs for every D-XX behavior listed in 07-VALIDATION.md" + - "Running pytest backend/tests/ -v emits xfail markers for the new stubs and zero new failures" + - "alembic upgrade head creates the system_settings table with provider_id UNIQUE constraint" + - "load_provider_config(session) returns a ProviderConfig when an active row exists in system_settings" + - "encrypt_api_key/decrypt_api_key round-trip a plaintext key using HKDF salt=provider_id, info=b\"ai-provider-settings\"" + - "Backend startup seeds system_settings from env vars on first boot when no row exists for the env-configured default provider" + - "docker-compose.yml continues to declare extra_hosts: host.docker.internal:host-gateway on both backend and celery-worker services (D-14 regression guard)" + artifacts: + - path: "backend/migrations/versions/0005_system_settings.py" + provides: "system_settings table creation" + contains: "system_settings" + - path: "backend/db/models.py" + provides: "SystemSettings ORM model" + contains: "class SystemSettings" + - path: "backend/services/ai_config.py" + provides: "HKDF encryption helpers, ProviderConfig loader, startup seed" + contains: "_derive_ai_settings_key" + - path: "backend/tests/test_ai_providers.py" + provides: "Wave 0 xfail stubs for D-01, D-03, D-06, D-07, D-12, D-13, D-16" + contains: "pytest.xfail" + - path: "backend/tests/test_ai_config.py" + provides: "Wave 0 xfail stubs for D-04, D-05" + contains: "pytest.xfail" + - path: "backend/tests/test_admin_ai_config.py" + provides: "Wave 0 xfail stubs for admin endpoint key-never-returned invariant" + contains: "pytest.xfail" + key_links: + - from: "backend/services/ai_config.py::_derive_ai_settings_key" + to: "backend/storage/cloud_utils.py::_derive_fernet_key" + via: "HKDF pattern reuse with info=b\"ai-provider-settings\"" + pattern: "info=b\"ai-provider-settings\"" + - from: "backend/main.py lifespan" + to: "backend/services/ai_config.py::seed_system_settings_from_env" + via: "startup callback" + pattern: "seed_system_settings_from_env" + - from: "backend/migrations/versions/0005_system_settings.py" + to: "backend/db/models.py::SystemSettings" + via: "Schema/ORM alignment" + pattern: "UniqueConstraint.*provider_id" + - from: "docker-compose.yml backend service" + to: "host.docker.internal:host-gateway extra_hosts entry" + via: "Linux Docker host networking (D-14, pre-satisfied)" + pattern: "host.docker.internal:host-gateway" +--- + + +Lay the database, encryption, and test scaffolding foundation that every other Phase 7 plan depends on. + +Purpose: Establish the single source of truth for AI provider configuration (`system_settings` table) with HKDF/Fernet encryption for API keys, mirror the proven cloud_utils.py pattern with domain-separated info bytes, and put Wave 0 xfail stubs in place so every later wave can promote red tests to green incrementally. +Output: Alembic migration 0005, `SystemSettings` ORM model, `backend/services/ai_config.py` (encryption helpers + `load_provider_config()` + `seed_system_settings_from_env`), startup wiring, and four Wave 0 test files containing the xfail stubs enumerated in 07-VALIDATION.md. + +D-14 is pre-satisfied (RESEARCH.md confirms extra_hosts already present in docker-compose.yml). This plan formally acknowledges D-14 by adding a regression-guard grep in so any future refactor that drops the entry fails the gate. + + + +@$HOME/.claude/get-shit-done/workflows/execute-plan.md +@$HOME/.claude/get-shit-done/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/ROADMAP.md +@.planning/STATE.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-CONTEXT.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-RESEARCH.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-PATTERNS.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-VALIDATION.md +@backend/storage/cloud_utils.py +@backend/db/models.py +@backend/migrations/versions/0004_phase4_pdf_open_mode_tsvector.py +@backend/main.py +@backend/config.py +@docker-compose.yml + + + + +From backend/storage/cloud_utils.py (HKDF pattern to mirror): +- `_derive_fernet_key(master_key: bytes, user_id: str) -> Fernet` — uses salt=user_id.encode(), info=b"cloud-credentials"; creates FRESH HKDF instance every call (cryptography raises AlreadyFinalized on second .derive()). +- `encrypt_credentials(master_key: bytes, user_id: str, credentials: dict) -> str` — JSON-encodes then Fernet-encrypts. +- `decrypt_credentials(master_key: bytes, user_id: str, credentials_enc: str) -> dict` — Fernet-decrypts then JSON-decodes. + +From backend/db/models.py (lines 299-318, CloudConnection): +- `Mapped[uuid.UUID]` columns with `mapped_column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4)` +- `TIMESTAMP(timezone=True)` columns with `server_default=func.now()` +- `__table_args__` carries Index/UniqueConstraint definitions + +From backend/config.py (env var sources for seed): +- `settings.default_ai_provider` (env: DEFAULT_AI_PROVIDER, default "ollama") +- `settings.default_ai_model` (env: DEFAULT_AI_MODEL, default "llama3.2") +- `settings.cloud_creds_key` (env: CLOUD_CREDS_KEY) — reused as master key for AI settings per D-05 + +From backend/main.py (lifespan): +- Existing async lifespan context manager calls startup hooks before yield. + +From docker-compose.yml (D-14 pre-satisfied): +- `backend` service has `extra_hosts: ["host.docker.internal:host-gateway"]`. +- `celery-worker` service has `extra_hosts: ["host.docker.internal:host-gateway"]`. +- This plan adds a regression-guard grep — DO NOT remove either entry. + +PROVIDER_DEFAULTS table (from 07-RESEARCH.md, also reused by Plan 02): +- "openai" -> {"base_url": None, "model": "gpt-4o", "context_chars": 120000} +- "anthropic" -> {"base_url": None, "model": "claude-sonnet-4-6", "context_chars": 180000} +- "gemini" -> {"base_url": "https://generativelanguage.googleapis.com/v1beta/openai/", "model": "gemini-2.0-flash", "context_chars": 800000} +- "groq" -> {"base_url": "https://api.groq.com/openai/v1", "model": "llama-3.3-70b-versatile", "context_chars": 128000} +- "xai" -> {"base_url": "https://api.x.ai/v1", "model": "grok-3-mini", "context_chars": 128000} +- "deepseek" -> {"base_url": "https://api.deepseek.com", "model": "deepseek-chat", "context_chars": 60000} +- "openrouter" -> {"base_url": "https://openrouter.ai/api/v1", "model": "anthropic/claude-3.5-sonnet", "context_chars": 180000} +- "mistral" -> {"base_url": "https://api.mistral.ai/v1", "model": "mistral-large-latest", "context_chars": 128000} +- "ollama" -> {"base_url": "http://host.docker.internal:11434/v1", "model": "llama3.2", "context_chars": 8000} +- "lmstudio" -> {"base_url": "http://host.docker.internal:1234/v1", "model": "gemma-4-e4b-it", "context_chars": 8000} + + + + + + + Task 1: Wave 0 test scaffolds for D-01..D-16 + + backend/tests/conftest.py + backend/tests/test_classifier.py + backend/tests/test_documents.py + backend/tests/test_cloud_utils.py + .planning/phases/07-redo-and-optimize-llm-integration/07-VALIDATION.md + + + - test_ai_providers.py contains pytest.xfail stubs named: test_generic_openai_json_mode, test_anthropic_structured_output, test_get_provider_typed, test_client_singleton, test_context_chars_truncation, test_smart_truncation, test_gemini_fallback_to_parse_classification (covers D-02 — supports_json_mode=False path returns ClassificationResult via parse_classification) + - test_ai_config.py contains pytest.xfail stubs named: test_load_provider_config, test_api_key_encrypt_decrypt + - test_admin_ai_config.py contains pytest.xfail stubs named: test_get_never_returns_key, test_put_writes_active_provider + - test_document_tasks.py contains pytest.xfail stubs named: test_retry_backoff, test_exhaustion_sets_failed_status; if file does not exist it must be created + - test_documents.py gains a single pytest.xfail stub named: test_reclassify_requeues_celery + - Running pytest backend/tests/ -v reports the new xfailed tests and zero new failures + + + Create backend/tests/test_ai_providers.py, backend/tests/test_ai_config.py, backend/tests/test_admin_ai_config.py, and backend/tests/test_document_tasks.py (create new if missing — verify with ls first). Each test file imports pytest only and contains the test function names listed under behavior; each function body is a single line: pytest.xfail("not implemented yet — Plan 0X-Y") where the plan reference matches the wave that will promote it per 07-VALIDATION.md per-task-verification map. Mark every xfail stub with @pytest.mark.xfail(strict=False, reason="Wave 0 stub") above the function definition. Append the single test_reclassify_requeues_celery xfail stub to the bottom of the existing backend/tests/test_documents.py (do not rewrite the file). Pattern follows the "single-line body only" decision recorded in STATE.md for Wave 0 stubs. No assertion code in any stub. test_gemini_fallback_to_parse_classification is the D-02 coverage stub — Plan 02 Task 4 promotes it. + + + cd backend && pytest tests/test_ai_providers.py tests/test_ai_config.py tests/test_admin_ai_config.py tests/test_document_tasks.py tests/test_documents.py -v 2>&1 | grep -E "xfailed|passed|failed" | tail -3 + + + - Source assertion: backend/tests/test_ai_providers.py contains exactly seven stub functions named test_generic_openai_json_mode, test_anthropic_structured_output, test_get_provider_typed, test_client_singleton, test_context_chars_truncation, test_smart_truncation, test_gemini_fallback_to_parse_classification + - Source assertion: backend/tests/test_ai_config.py contains test_load_provider_config and test_api_key_encrypt_decrypt + - Source assertion: backend/tests/test_admin_ai_config.py contains test_get_never_returns_key and test_put_writes_active_provider + - Source assertion: backend/tests/test_document_tasks.py contains test_retry_backoff and test_exhaustion_sets_failed_status + - Source assertion: backend/tests/test_documents.py contains test_reclassify_requeues_celery + - Source assertion: grep -c 'pytest.mark.xfail(strict=False' backend/tests/test_ai_providers.py returns 7 + - Source assertion (D-14 regression guard): `grep -c 'host.docker.internal:host-gateway' docker-compose.yml` returns 2 + - Behavior: pytest backend/tests/ -v reports 0 new failures attributable to these files + + All four scaffold files exist with the correct stub names; existing test_documents.py gained one stub; full suite xfail count grows by 13, failure count is unchanged. + + + + Task 2: Alembic migration 0005 + SystemSettings ORM model + + backend/migrations/versions/0004_phase4_pdf_open_mode_tsvector.py + backend/migrations/versions/0001_initial_schema.py + backend/db/models.py + + + - alembic upgrade head succeeds and creates the system_settings table + - The table has columns: id (UUID PK, server_default gen_random_uuid()), provider_id (String NOT NULL UNIQUE), api_key_enc (Text NULL), base_url (Text NULL), model_name (Text NOT NULL default ''), context_chars (Integer NOT NULL default 8000), is_active (Boolean NOT NULL default false), created_at (TIMESTAMPTZ NOT NULL default now()), updated_at (TIMESTAMPTZ NOT NULL default now()) + - UniqueConstraint on provider_id named uq_system_settings_provider_id is created + - SystemSettings ORM model exposes the same columns with Mapped[] typed declarations + - alembic downgrade -1 drops the table cleanly + + + Create backend/migrations/versions/0005_system_settings.py with revision = "0005", down_revision = "0004", branch_labels = None, depends_on = None. Use the analog from 0004_phase4_pdf_open_mode_tsvector.py (imports, structure) but the body uses op.create_table("system_settings", ...) with sa.dialects.postgresql.UUID(as_uuid=True) for id, server_default=sa.text("gen_random_uuid()") for id, sa.Text for api_key_enc/base_url/model_name (model_name server_default=""), sa.Integer for context_chars (server_default="8000"), sa.Boolean for is_active (server_default="false"), sa.TIMESTAMP(timezone=True) for created_at/updated_at (server_default=sa.text("now()")), and sa.UniqueConstraint("provider_id", name="uq_system_settings_provider_id"). downgrade() calls op.drop_table("system_settings"). + + In backend/db/models.py append a `class SystemSettings(Base)` ORM model with __tablename__ = "system_settings". Mirror the CloudConnection column style (Mapped[uuid.UUID] id with mapped_column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4)). provider_id: Mapped[str] = mapped_column(String, nullable=False, unique=True). api_key_enc: Mapped[str | None] = mapped_column(Text, nullable=True). base_url: Mapped[str | None] = mapped_column(Text, nullable=True). model_name: Mapped[str] = mapped_column(Text, nullable=False, default=""). context_chars: Mapped[int] = mapped_column(Integer, nullable=False, default=8000). is_active: Mapped[bool] = mapped_column(Boolean, nullable=False, default=False). created_at / updated_at: Mapped[datetime] = mapped_column(TIMESTAMP(timezone=True), nullable=False, server_default=func.now()). Add __table_args__ = (UniqueConstraint("provider_id", name="uq_system_settings_provider_id"),). Confirm all imports (Boolean, Integer, Text, UniqueConstraint, TIMESTAMP, func) are already present at the top of models.py — they are per 07-PATTERNS.md. + + + cd backend && python -c "from db.models import SystemSettings; print(SystemSettings.__tablename__, SystemSettings.__table_args__)" && ls migrations/versions/0005_system_settings.py + + + - Source assertion: backend/migrations/versions/0005_system_settings.py exists and contains the literal strings `revision = "0005"`, `down_revision = "0004"`, `op.create_table("system_settings"`, `name="uq_system_settings_provider_id"`, `op.drop_table("system_settings")` + - Source assertion: backend/db/models.py contains the string `class SystemSettings(Base)` and the string `__tablename__ = "system_settings"` + - Source assertion: grep -c 'mapped_column' backend/db/models.py increases by at least 9 (one per SystemSettings column) + - Behavior: `python -c "from db.models import SystemSettings"` exits 0 + - Behavior: SystemSettings.__table_args__ contains a UniqueConstraint whose name equals "uq_system_settings_provider_id" + + Migration file and ORM model are both committed; SystemSettings imports cleanly; migration revision chain is 0001→0002→0003→0004→0005. + + + + Task 3: services/ai_config.py — HKDF helpers, ProviderConfig loader, env seed + + backend/storage/cloud_utils.py + backend/config.py + backend/db/models.py + backend/main.py + backend/services/storage.py + + + - _derive_ai_settings_key(master_key, provider_id) creates a fresh HKDF(salt=provider_id.encode(), info=b"ai-provider-settings") on every call and returns a Fernet instance + - encrypt_api_key(master_key, provider_id, api_key)/decrypt_api_key(master_key, provider_id, ciphertext) round-trip a plain string (no JSON wrap) + - load_provider_config(session) returns a ProviderConfig built from the row where is_active=True, decrypting api_key_enc when present; returns None when no active row exists + - seed_system_settings_from_env(session) inserts default rows for the env-configured default provider only when no row exists for that provider_id; never overwrites an existing row + - Backend startup calls seed_system_settings_from_env inside the existing lifespan + - HKDF info bytes differ from cloud_utils (`b"ai-provider-settings"` vs `b"cloud-credentials"`) — domain separation invariant verified by test + + + Create backend/services/ai_config.py mirroring backend/storage/cloud_utils.py structure (module docstring explaining HKDF domain separation, AlreadyFinalized warning comment, imports of Fernet/HKDF/hashes from cryptography). Implement: `_derive_ai_settings_key(master_key: bytes, provider_id: str) -> Fernet` (fresh HKDF instance, salt=provider_id.encode("utf-8"), info=b"ai-provider-settings", length=32, sha256); `encrypt_api_key(master_key: bytes, provider_id: str, api_key: str) -> str` (Fernet.encrypt(api_key.encode()).decode()); `decrypt_api_key(master_key: bytes, provider_id: str, api_key_enc: str) -> str` (Fernet.decrypt(...).decode()); `async def load_provider_config(session: AsyncSession) -> Optional[ProviderConfig]` (select SystemSettings where is_active is True, decrypt api_key_enc using settings.cloud_creds_key bytes when not None, build ProviderConfig — note this depends on Plan 02 providing ProviderConfig; for now import lazily inside the function with a TYPE_CHECKING guard and a deferred import comment); `async def seed_system_settings_from_env(session: AsyncSession) -> None` (read settings.default_ai_provider/default_ai_model from config; SELECT one row WHERE provider_id = default; if missing INSERT a SystemSettings row with provider_id=default_ai_provider, model_name=default_ai_model, context_chars=8000, is_active=True, api_key_enc=None, base_url=None). + + Because Plan 02 introduces ProviderConfig, define a minimal `class _ProviderConfigStub(BaseModel): provider_id: str; api_key: str = ""; base_url: str | None = None; model: str = ""; context_chars: int = 8000` inside ai_config.py for now, and add a module-level note `# ProviderConfig redefined in ai/provider_config.py during Plan 02 — load_provider_config will re-import and return that class once Plan 02 lands`. load_provider_config tries `from ai.provider_config import ProviderConfig` inside the function body and falls back to the stub if the import fails. This stub is removed in Plan 03 once classifier consumes the real ProviderConfig. + + In backend/main.py register seed_system_settings_from_env inside the existing async lifespan: after the existing startup steps (and after the async session factory is available) acquire an AsyncSession via the existing session factory, await seed_system_settings_from_env(session), and await session.commit(). Wrap with try/except logging the error and continuing startup (do not crash boot if the table is missing during fresh container startup before migrations run — log a warning and skip). Import the function at the top of main.py. + + + cd backend && python -c "from services.ai_config import _derive_ai_settings_key, encrypt_api_key, decrypt_api_key, load_provider_config, seed_system_settings_from_env; mk=b'0'*32; ct=encrypt_api_key(mk,'openai','sk-test'); assert decrypt_api_key(mk,'openai',ct)=='sk-test'; print('round-trip OK')" + + + - Source assertion: backend/services/ai_config.py contains the literal strings `info=b"ai-provider-settings"`, `salt=provider_id.encode("utf-8")`, `async def load_provider_config`, `async def seed_system_settings_from_env` + - Source assertion: backend/services/ai_config.py does NOT contain the string `info=b"cloud-credentials"` (domain separation) + - Source assertion: backend/main.py imports seed_system_settings_from_env and calls it inside the lifespan + - Behavior: `python -c` round-trip above prints "round-trip OK" exactly + - Behavior: encrypting the same plaintext with provider_id="openai" vs provider_id="anthropic" produces different ciphertexts (domain salt isolation) + - Behavior: HKDF derivation never raises AlreadyFinalized when _derive_ai_settings_key is called twice consecutively (fresh instance per call) + + ai_config.py module exposes encryption + loader + seed; main.py invokes the seed on startup; round-trip test passes from CLI. + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| admin browser → /api/admin/ai-config (planned Plan 05) | admin-supplied API key crosses here; api_key_enc must never travel back | +| application → PostgreSQL system_settings | ciphertext at rest; master key derived per-provider; never logged | +| HKDF master key (env CLOUD_CREDS_KEY) → derived Fernet keys | shared master across cloud + AI; domain separated by info bytes | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Disposition | Mitigation Plan | +|-----------|----------|-----------|-------------|-----------------| +| T-07-01 | Information Disclosure | services/ai_config.py decrypt path | mitigate | api_key returned only inside ProviderConfig consumed server-side; never serialized through API — Plan 05 enforces whitelist `_ai_config_to_dict()` that excludes api_key_enc | +| T-07-02 | Elevation of Privilege | HKDF key derivation | mitigate | info=b"ai-provider-settings" enforces domain separation from b"cloud-credentials"; same master key cannot derive the same Fernet for both domains — test enforces inequality | +| T-07-03 | Tampering | system_settings.is_active dual-write | mitigate | Plan 05 enforces single UPDATE SET is_active = (provider_id = $target) — atomic flip, no read-then-write | +| T-07-SC | Tampering | npm/pip installs | accept | No new packages added in Phase 7 — RESEARCH.md Package Legitimacy Audit confirms zero new deps; bandit + pip audit re-runs at phase gate | + + + +- alembic upgrade head succeeds against a fresh PostgreSQL: `cd backend && alembic upgrade head` exits 0 and creates the system_settings table. +- python -c smoke test in Task 3 prints "round-trip OK" — encryption round-trip verified. +- pytest backend/tests/ -v adds 13 xfail tests with zero new failures. +- grep -F 'info=b"ai-provider-settings"' backend/services/ai_config.py returns the helper line. +- grep -F 'info=b"cloud-credentials"' backend/services/ai_config.py returns no matches (domain separation). +- D-14 regression guard: `grep "host.docker.internal:host-gateway" docker-compose.yml | wc -l` outputs 2 (entries for both backend and celery-worker services; D-14 is pre-satisfied per RESEARCH.md — this guard prevents accidental removal). + + + +- system_settings table created and indexed (alembic head = 0005). +- SystemSettings ORM model importable from backend.db.models. +- backend/services/ai_config.py encrypt/decrypt round-trips; HKDF salt + info match RESEARCH.md spec. +- Startup lifespan seeds default provider from env when row absent; idempotent across restarts. +- Wave 0 test stubs exist in four files and reflect every D-XX listed in 07-VALIDATION.md. +- pytest backend/tests/ -v shows new xfail count of 13 with zero new failures. +- D-14 regression guard passes: docker-compose.yml retains both host-gateway entries. + + + +Create `.planning/phases/07-redo-and-optimize-llm-integration/07-01-SUMMARY.md` when done. + diff --git a/.planning/phases/07-redo-and-optimize-llm-integration/07-02-PLAN.md b/.planning/phases/07-redo-and-optimize-llm-integration/07-02-PLAN.md new file mode 100644 index 0000000..6f181b9 --- /dev/null +++ b/.planning/phases/07-redo-and-optimize-llm-integration/07-02-PLAN.md @@ -0,0 +1,332 @@ +--- +phase: 07-redo-and-optimize-llm-integration +plan: 02 +type: execute +wave: 2 +depends_on: + - 07-01 +files_modified: + - backend/ai/provider_config.py + - backend/ai/generic_openai_provider.py + - backend/ai/openai_provider.py + - backend/ai/ollama_provider.py + - backend/ai/lmstudio_provider.py + - backend/ai/__init__.py + - backend/services/classifier.py + - requirements.txt +autonomous: true +requirements: + - D-01 + - D-02 # parse_classification/parse_suggestions remain as last-resort fallback for non-json_mode paths (Gemini preset path covered by test_gemini_fallback_to_parse_classification) + - D-06 + - D-07 + - D-12 + - D-13 + - D-15 + - D-16 + - D-17 + - D-18 + +must_haves: + truths: + - "ProviderConfig is a Pydantic BaseModel with provider_id, api_key, base_url, model, context_chars" + - "GenericOpenAIProvider subclasses OpenAIProvider and passes response_format={\"type\":\"json_object\"} when supports_json_mode is True" + - "GenericOpenAIProvider imports parse_classification + parse_suggestions from ai.utils and uses them on every classify()/suggest_topics() raw response (D-02 last-resort fallback)" + - "OpenAIProvider stores self._client = AsyncOpenAI(...) in __init__ and never recreates it" + - "get_provider(config: ProviderConfig) is a typed registry lookup (no if/elif chain)" + - "MAX_AI_CHARS is absent from backend/ai/openai_provider.py and backend/services/classifier.py" + - "PROVIDER_DEFAULTS dict covers all 10 providers listed in 07-RESEARCH.md Pattern 2" + - "requirements.txt pins anthropic>=0.95.0" + - "backend/ai/utils.py (parse_classification / parse_suggestions) remains intact — no edits, no deletions — and is imported by generic_openai_provider.py" + artifacts: + - path: "backend/ai/provider_config.py" + provides: "ProviderConfig Pydantic model + PROVIDER_DEFAULTS dict" + contains: "class ProviderConfig(BaseModel)" + - path: "backend/ai/generic_openai_provider.py" + provides: "Unified OpenAI-compat provider with JSON-mode + smart truncation + parse_classification fallback" + contains: "class GenericOpenAIProvider(OpenAIProvider)" + - path: "backend/ai/openai_provider.py" + provides: "Singleton client + context_chars + _truncate" + contains: "self._client = AsyncOpenAI" + - path: "backend/ai/__init__.py" + provides: "Registry-based get_provider(config)" + contains: "_REGISTRY" + - path: "requirements.txt" + provides: "anthropic SDK floor for output_config support" + contains: "anthropic>=0.95" + key_links: + - from: "backend/ai/__init__.py::get_provider" + to: "backend/ai/provider_config.py::ProviderConfig" + via: "typed function signature" + pattern: "get_provider\\(config: ProviderConfig\\)" + - from: "backend/ai/generic_openai_provider.py::classify" + to: "backend/ai/provider_config.py::supports_json_mode flag" + via: "conditional response_format kwarg" + pattern: "response_format" + - from: "backend/ai/generic_openai_provider.py" + to: "backend/ai/utils.py::parse_classification" + via: "D-02 last-resort fallback import" + pattern: "from ai.utils import parse_classification" + - from: "backend/ai/openai_provider.py::__init__" + to: "AsyncOpenAI httpx connection pool" + via: "singleton storage on self._client" + pattern: "self\\._client = AsyncOpenAI" +--- + + +Replace the legacy AI provider plumbing with a typed Pydantic config model, a unified GenericOpenAIProvider that covers all 8 OpenAI-compatible vendors, a singleton client lifecycle, and a registry-based factory. Pin the anthropic SDK floor so Plan 03 can use output_config. + +Purpose: Eliminate the per-request `_client()` anti-pattern (D-07) that prevents httpx connection pool reuse; consolidate Groq/xAI/DeepSeek/OpenRouter/Gemini/Mistral/Ollama/LMStudio into one class with named presets (D-16/D-17/D-18); make adding a new provider an O(1) registry edit; remove MAX_AI_CHARS in favour of per-provider context_chars (D-12) with 60/40 smart truncation (D-13); preserve parse_classification/parse_suggestions in ai/utils.py as the last-resort fallback path for providers that do not honour response_format (D-02 — Gemini preset path). +Output: provider_config.py, generic_openai_provider.py, refactored openai_provider.py, ollama_provider.py + lmstudio_provider.py with context_chars passthrough, registry-based ai/__init__.py, MAX_AI_CHARS removed from openai_provider.py and classifier.py, requirements.txt anthropic floor bumped. + + + +@$HOME/.claude/get-shit-done/workflows/execute-plan.md +@$HOME/.claude/get-shit-done/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-CONTEXT.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-RESEARCH.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-PATTERNS.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-01-SUMMARY.md +@backend/ai/openai_provider.py +@backend/ai/ollama_provider.py +@backend/ai/lmstudio_provider.py +@backend/ai/__init__.py +@backend/ai/base.py +@backend/ai/utils.py +@backend/services/classifier.py +@requirements.txt + + + + +From backend/ai/base.py: +- `class AIProvider(ABC)` with abstract async methods classify(document_text, existing_topics, system_prompt) -> ClassificationResult and suggest_topics(document_text) -> list[str] and health_check() -> bool +- `ClassificationResult` dataclass with `assigned_topics: list[str]`, `new_topic_suggestions: list[str]`, optional `reasoning` + +From backend/ai/utils.py (D-02 — last-resort fallback; DO NOT modify or delete): +- `parse_classification(raw: str) -> ClassificationResult` — handles well-formed JSON and degraded prose +- `parse_suggestions(raw: str) -> list[str]` +- `strip_code_fences(raw: str) -> str` + +From backend/ai/openai_provider.py (current, lines 8-69): +- `class OpenAIProvider(AIProvider)` +- `def __init__(self, api_key: str, model: str = "gpt-4o", base_url=None)` — to be replaced +- `def _client(self) -> AsyncOpenAI` — to be removed +- async methods classify/suggest_topics/health_check call self._client().chat.completions.create(...) + +OpenAI SDK signal (07-RESEARCH.md): +- `AsyncOpenAI(api_key=..., base_url=...)` — must pass non-empty api_key in SDK 2.34+ (use "not-needed" placeholder) +- `chat.completions.create(model=..., max_tokens=..., response_format={"type":"json_object"}, messages=[...])` + +PROVIDER_DEFAULTS dict (verbatim from 07-RESEARCH.md Pattern 2): +- 10 keys: openai, anthropic, gemini, groq, xai, deepseek, openrouter, mistral, ollama, lmstudio +- Each value: {"base_url": str|None, "model": str, "context_chars": int} +- Add a parallel SUPPORTS_JSON_MODE dict (defaults True; "gemini" -> False per 07-RESEARCH.md JSON Compatibility Matrix; "ollama" / "lmstudio" remain True but classify() falls back to parse_classification() on any output anyway) + + + + + + + Task 1: ProviderConfig model + PROVIDER_DEFAULTS + + backend/ai/__init__.py + backend/api/admin.py + .planning/phases/07-redo-and-optimize-llm-integration/07-RESEARCH.md + + + - ProviderConfig(BaseModel) accepts provider_id (str, required), api_key (str, default ""), base_url (str | None, default None), model (str, default ""), context_chars (int, default 8000) + - PROVIDER_DEFAULTS dict at module top contains exactly the 10 provider_ids listed in 07-RESEARCH.md Pattern 2 with matching base_url/model/context_chars values + - SUPPORTS_JSON_MODE dict has gemini=False and openai/anthropic/groq/xai/deepseek/openrouter/mistral/ollama/lmstudio=True + - Module imports without side effects; importing ProviderConfig does not import any provider class + - pytest backend/tests/test_ai_providers.py::test_get_provider_typed promotes from xfail to pass after Task 4 + + + Create backend/ai/provider_config.py with: `from __future__ import annotations`; `from pydantic import BaseModel`; `class ProviderConfig(BaseModel)` with the five fields above and a class-level model_config setting extra="forbid" so unknown keys raise validation errors; `PROVIDER_DEFAULTS: dict[str, dict] = {...}` with the 10 entries from 07-RESEARCH.md Pattern 2 verbatim (do not change any base_url, model name, or context_chars value); `SUPPORTS_JSON_MODE: dict[str, bool] = {...}` with the 10 entries described above. No provider class imports — keep this file a pure data module. Add a brief docstring noting "Loaded by get_provider() in ai/__init__.py; populated by load_provider_config() in services/ai_config.py." + + + cd backend && python -c "from ai.provider_config import ProviderConfig, PROVIDER_DEFAULTS, SUPPORTS_JSON_MODE; c=ProviderConfig(provider_id='openai'); assert c.context_chars==8000; assert set(PROVIDER_DEFAULTS)=={'openai','anthropic','gemini','groq','xai','deepseek','openrouter','mistral','ollama','lmstudio'}; assert SUPPORTS_JSON_MODE['gemini'] is False; print('OK')" + + + - Source assertion: backend/ai/provider_config.py contains `class ProviderConfig(BaseModel)` and `PROVIDER_DEFAULTS` and `SUPPORTS_JSON_MODE` + - Source assertion: backend/ai/provider_config.py contains "gemini" once each in PROVIDER_DEFAULTS and SUPPORTS_JSON_MODE + - Behavior: Smoke test prints "OK" + - Behavior: ProviderConfig(provider_id="x", garbage="y") raises ValidationError (extra="forbid") + - Behavior: ProviderConfig does not import any provider class (no circular imports at module load) + + provider_config.py exists, smoke test passes, ProviderConfig and the two defaults dicts are importable. + + + + Task 2: GenericOpenAIProvider + OpenAIProvider singleton + MAX_AI_CHARS removal + ollama/lmstudio context_chars + + backend/ai/openai_provider.py + backend/ai/ollama_provider.py + backend/ai/lmstudio_provider.py + backend/services/classifier.py + backend/ai/utils.py + backend/ai/base.py + + + - OpenAIProvider.__init__(api_key, model, base_url, context_chars) stores self._client = AsyncOpenAI(api_key=api_key or "not-needed", base_url=base_url) exactly once + - OpenAIProvider has no `_client(self)` method and no `MAX_AI_CHARS` constant + - OpenAIProvider has a _truncate(text) method that returns text unchanged when len(text) <= self._context_chars; otherwise returns text[:int(self._context_chars*0.6)] + "\n[...truncated...]\n" + text[-(self._context_chars - int(self._context_chars*0.6)):] + - OpenAIProvider.classify() and suggest_topics() use self._truncate(document_text) and call self._client.chat.completions.create(...) directly (no parentheses) + - GenericOpenAIProvider(OpenAIProvider) overrides classify() and suggest_topics() to pass response_format={"type":"json_object"} when class flag supports_json_mode is True; falls back to omitting the kwarg when False; parses with parse_classification/parse_suggestions in both branches (D-02 — last-resort fallback always wraps the raw response) + - GenericOpenAIProvider.classify() and .suggest_topics() import parse_classification and parse_suggestions from ai.utils (D-02 contract — these helpers in ai/utils.py remain the canonical fallback; no provider re-implements them) + - GenericOpenAIProvider accepts supports_json_mode as a constructor kwarg (default True) so the factory can set False for Gemini + - OllamaProvider.__init__ accepts an optional context_chars kwarg (default 8000) and passes it through super().__init__ + - LMStudioProvider mirrors OllamaProvider's context_chars passthrough + - MAX_AI_CHARS is removed from backend/services/classifier.py (Plan 03 will fix the call-sites that used it; this task removes the constant) + - backend/ai/utils.py is NOT modified — parse_classification/parse_suggestions remain intact per D-02 + - All existing test_classifier.py and test_lmstudio.py tests remain green + + + Refactor backend/ai/openai_provider.py: change __init__ signature to `def __init__(self, api_key: str, model: str, base_url: str | None, context_chars: int)`. Inside __init__ set self._api_key=api_key or "not-needed", self._model=model, self._base_url=base_url, self._context_chars=context_chars, self._client = AsyncOpenAI(api_key=self._api_key, base_url=self._base_url). Delete the existing `def _client(self):` method entirely. Delete the module-level `MAX_AI_CHARS = 8_000` line. Add `def _truncate(self, text: str) -> str:` returning the 60/40 split as described under behavior. Update classify(): replace `document_text[:MAX_AI_CHARS]` with `self._truncate(document_text)`; replace `await self._client().chat.completions.create(...)` with `await self._client.chat.completions.create(...)` (no parens). Update suggest_topics() and health_check() with the same self._client (no parens) change. Do not add response_format kwarg to OpenAIProvider — that lives in GenericOpenAIProvider (D-16). Existing OpenAIProvider semantics for OpenAI proper remain unchanged otherwise. + + Create backend/ai/generic_openai_provider.py: import AsyncOpenAI from openai, OpenAIProvider from ai.openai_provider, and EXACTLY `from ai.utils import parse_classification, parse_suggestions` (D-02 contract — these are the last-resort fallback helpers; do not redefine locally, do not import-rename). Define `class GenericOpenAIProvider(OpenAIProvider)` with class attribute `supports_json_mode: bool = True` and override __init__ to accept supports_json_mode as a kwarg (after context_chars), set self.supports_json_mode = supports_json_mode, then call super().__init__(api_key=api_key, model=model, base_url=base_url, context_chars=context_chars). Override classify(self, document_text, existing_topics, system_prompt): assemble topics_str and user_msg the same way as OpenAIProvider.classify(); call self._client.chat.completions.create with model=self._model, max_tokens=1024, messages=[system, user], and conditionally add response_format={"type":"json_object"} when self.supports_json_mode is True; raw = response.choices[0].message.content or ""; return parse_classification(raw). Override suggest_topics() with the same conditional response_format pattern (omit response_format when supports_json_mode is False) and call parse_suggestions on the raw text. Do NOT override health_check — inherit from OpenAIProvider. + + Update backend/ai/ollama_provider.py: change __init__ signature to `def __init__(self, base_url: str = "http://host.docker.internal:11434", model: str = "llama3.2", context_chars: int = 8000)`. Pass context_chars=context_chars to super().__init__. Same for lmstudio_provider.py (default model and base_url unchanged, add context_chars=8000 default and pass through). These files remain functional shims even though the registry (Task 3) routes "ollama" and "lmstudio" to GenericOpenAIProvider — keep them green for the existing test_lmstudio.py / test_classifier.py callers. + + Update backend/services/classifier.py: delete the module-level `MAX_AI_CHARS = 8_000` constant. Replace any remaining `text[:MAX_AI_CHARS]` usage with `text` (truncation now happens inside the provider via _truncate; Plan 03 finalizes the load_provider_config wiring). Do NOT change the function signatures or remove the inline _settings dict yet — that is Plan 03's job. The only change to classifier.py in this plan is removing the MAX_AI_CHARS constant and the slice that uses it. + + DO NOT modify backend/ai/utils.py — parse_classification, parse_suggestions, and strip_code_fences must remain intact (D-02 contract). They are imported by generic_openai_provider.py as the last-resort JSON fallback for non-json_mode paths. + + Bump requirements.txt: locate the line beginning with `anthropic` and change the constraint to `anthropic>=0.95.0` (preserving any other version markers). If the line is `anthropic>=0.26` change to `anthropic>=0.95.0`; if it pins an exact version that is < 0.95, raise it to `>=0.95.0`. This unblocks Plan 03 output_config usage. + + + cd backend && python -c "from ai.openai_provider import OpenAIProvider; from ai.generic_openai_provider import GenericOpenAIProvider; p=OpenAIProvider(api_key='', model='gpt-4o', base_url=None, context_chars=100); assert hasattr(p,'_client') and not callable(p._client.chat); print('singleton:', type(p._client).__name__); g=GenericOpenAIProvider(api_key='', model='m', base_url='http://x/v1', context_chars=50, supports_json_mode=False); assert g.supports_json_mode is False; print('OK')" && grep -c 'MAX_AI_CHARS' backend/ai/openai_provider.py backend/services/classifier.py 2>/dev/null | grep -v ':0' || echo "MAX_AI_CHARS removed" + + + - Source assertion: grep -v '^#' backend/ai/openai_provider.py | grep -c 'MAX_AI_CHARS' returns 0 + - Source assertion: grep -v '^#' backend/services/classifier.py | grep -c 'MAX_AI_CHARS' returns 0 + - Source assertion: backend/ai/openai_provider.py contains `self._client = AsyncOpenAI(` + - Source assertion: backend/ai/openai_provider.py does NOT contain `def _client(self)` (regex: `def _client\(self\)`) + - Source assertion: backend/ai/generic_openai_provider.py contains `class GenericOpenAIProvider(OpenAIProvider)` and `response_format={"type": "json_object"}` + - Source assertion (D-02 enforcement): `grep "from ai.utils import parse_classification" backend/ai/generic_openai_provider.py` returns a match + - Source assertion (D-02 enforcement): `grep -c "parse_suggestions" backend/ai/generic_openai_provider.py` returns >= 1 + - Source assertion (D-02 invariant): backend/ai/utils.py contains `def parse_classification` and `def parse_suggestions` (file untouched — these helpers remain the single canonical fallback) + - Source assertion: backend/ai/ollama_provider.py and backend/ai/lmstudio_provider.py both contain `context_chars` + - Source assertion: requirements.txt contains a line matching `^anthropic>=0\.9[5-9]` or `^anthropic>=0\.1` followed by `[0-9][0-9]` + - Behavior: python -c smoke prints "OK" + - Behavior: pytest backend/tests/test_classifier.py backend/tests/test_lmstudio.py -x exits 0 + + OpenAIProvider singleton fix landed; GenericOpenAIProvider created and imports parse_classification/parse_suggestions from ai.utils (D-02); ai/utils.py unchanged; ollama/lmstudio carry context_chars; MAX_AI_CHARS removed from openai_provider.py and classifier.py (anthropic_provider.py removal is Plan 03 because that file is refactored there); requirements.txt anthropic pin raised to >=0.95.0; existing tests still green. + + + + Task 3: Registry-based get_provider(config: ProviderConfig) + + backend/ai/__init__.py + backend/ai/provider_config.py + backend/ai/openai_provider.py + backend/ai/generic_openai_provider.py + backend/ai/anthropic_provider.py + + + - backend/ai/__init__.py exposes `get_provider(config: ProviderConfig) -> AIProvider` with a typed signature (not `settings: dict`) + - The flat if/elif chain is replaced by a _REGISTRY dict mapping provider_id strings to provider classes + - "openai" -> OpenAIProvider, "anthropic" -> AnthropicProvider, "gemini"/"groq"/"xai"/"deepseek"/"openrouter"/"mistral"/"ollama"/"lmstudio" -> GenericOpenAIProvider + - Unknown provider_id raises ValueError("Unknown AI provider: ...") + - For "anthropic" the factory passes api_key+model+context_chars (no base_url — AnthropicProvider has no base_url ctor arg until Plan 03 refactors it; until then continue passing only the args its current __init__ accepts) + - For GenericOpenAIProvider entries the factory reads PROVIDER_DEFAULTS to resolve base_url when config.base_url is None, and reads SUPPORTS_JSON_MODE to pass supports_json_mode kwarg + - For api_key="" the factory passes "not-needed" placeholder (OpenAI SDK 2.34+ rejects empty string) + - test_ai_providers.py::test_get_provider_typed promotes from xfail to passing + + + Rewrite backend/ai/__init__.py to: import AIProvider from ai.base, OpenAIProvider from ai.openai_provider, AnthropicProvider from ai.anthropic_provider, GenericOpenAIProvider from ai.generic_openai_provider, ProviderConfig + PROVIDER_DEFAULTS + SUPPORTS_JSON_MODE from ai.provider_config. Define `_REGISTRY: dict[str, type[AIProvider]]` with the 10 mappings described above. Define `def get_provider(config: ProviderConfig) -> AIProvider`: lookup cls in _REGISTRY, raise ValueError on miss; resolve effective_base_url = config.base_url or PROVIDER_DEFAULTS[config.provider_id]["base_url"]; effective_model = config.model or PROVIDER_DEFAULTS[config.provider_id]["model"]; effective_context_chars = config.context_chars or PROVIDER_DEFAULTS[config.provider_id]["context_chars"]; effective_api_key = config.api_key or "not-needed"; branch on provider_id: if "anthropic" return cls(api_key=effective_api_key, model=effective_model) — passing only the args its current ctor accepts (Plan 03 will widen this), elif cls is GenericOpenAIProvider return cls(api_key=effective_api_key, model=effective_model, base_url=effective_base_url, context_chars=effective_context_chars, supports_json_mode=SUPPORTS_JSON_MODE[config.provider_id]), else (OpenAIProvider) return cls(api_key=effective_api_key, model=effective_model, base_url=effective_base_url, context_chars=effective_context_chars). Promote the test_get_provider_typed xfail in backend/tests/test_ai_providers.py to a real test: build a ProviderConfig(provider_id="groq"), call get_provider(config), assert isinstance(result, GenericOpenAIProvider), assert result.supports_json_mode is True, assert result._context_chars == PROVIDER_DEFAULTS["groq"]["context_chars"]. Build a second ProviderConfig(provider_id="gemini"), assert result.supports_json_mode is False. Build a ProviderConfig(provider_id="bogus"), assert pytest.raises(ValueError). + + + cd backend && pytest tests/test_ai_providers.py::test_get_provider_typed -x -v + + + - Source assertion: backend/ai/__init__.py contains `_REGISTRY` and `def get_provider(config: ProviderConfig)` + - Source assertion: backend/ai/__init__.py contains all 10 provider_id keys in _REGISTRY ("openai","anthropic","gemini","groq","xai","deepseek","openrouter","mistral","ollama","lmstudio") + - Source assertion: backend/ai/__init__.py does NOT contain a sequence of `elif active ==` (no if/elif chain) + - Behavior: pytest backend/tests/test_ai_providers.py::test_get_provider_typed exits 0 + - Behavior: ValueError raised when provider_id is unknown (asserted by test_get_provider_typed) + + Registry get_provider function in place; test_get_provider_typed promoted and green. + + + + Task 4: Promote singleton + JSON-mode + truncation + Gemini fallback tests + + backend/tests/test_ai_providers.py + backend/ai/openai_provider.py + backend/ai/generic_openai_provider.py + backend/ai/provider_config.py + backend/ai/utils.py + + + - test_client_singleton: building two GenericOpenAIProvider instances and calling classify on the same instance twice exercises self._client only — the test asserts the AsyncOpenAI class is called exactly once per instance (use unittest.mock.patch on openai.AsyncOpenAI) + - test_generic_openai_json_mode: mock self._client.chat.completions.create and assert response_format={"type":"json_object"} appears in the call kwargs when supports_json_mode=True; assert response_format kwarg is absent when supports_json_mode=False (Gemini preset path) + - test_context_chars_truncation: build a provider with context_chars=100, feed an input of length 500, assert _truncate returns a string of length < 500 that contains "[...truncated...]" + - test_smart_truncation: build context_chars=1000, input of length 5000; assert the returned string starts with input[:600] and ends with input[-400:] + - test_gemini_fallback_to_parse_classification (D-02 coverage): build GenericOpenAIProvider(supports_json_mode=False) (Gemini preset path); mock self._client.chat.completions.create to return a synthetic response whose .choices[0].message.content is a valid JSON classification string; assert classify() returns a valid ClassificationResult; assert response_format kwarg is absent from mock_create.call_args.kwargs; assert ai.utils.parse_classification was the helper that produced the ClassificationResult (use unittest.mock.patch to wrap parse_classification and assert it was called once with the raw content) + - All five tests run green; xfail markers removed + + + Edit backend/tests/test_ai_providers.py: replace the xfail stub bodies for test_client_singleton, test_generic_openai_json_mode, test_context_chars_truncation, test_smart_truncation, and test_gemini_fallback_to_parse_classification with real implementations. Remove the @pytest.mark.xfail decorator on these five functions only (leave anthropic and others as xfail until their respective plans). Use unittest.mock.patch("ai.openai_provider.AsyncOpenAI") and AsyncMock to mock the async chat.completions.create method. For test_generic_openai_json_mode, assert response_format kwarg is in mock_create.call_args.kwargs when supports_json_mode=True and absent when supports_json_mode=False. For test_client_singleton, instantiate one OpenAIProvider, await its classify() twice (mock create returns a synthetic OpenAI response), assert AsyncOpenAI class mock was called exactly once. For truncation tests, instantiate any provider with context_chars=100 (or 1000) and call provider._truncate(text) directly — no async needed. For test_gemini_fallback_to_parse_classification (D-02): use unittest.mock.patch("ai.generic_openai_provider.parse_classification", wraps=parse_classification) so the real function still runs but the call is observable; instantiate GenericOpenAIProvider(api_key="", model="gemini-2.0-flash", base_url="https://generativelanguage.googleapis.com/v1beta/openai/", context_chars=8000, supports_json_mode=False); mock create() to return `MagicMock(choices=[MagicMock(message=MagicMock(content='{"assigned_topics":["x"],"new_topic_suggestions":[],"reasoning":"r"}'))])`; await classify("doc text", [], "sys"); assert the returned ClassificationResult.assigned_topics == ["x"]; assert mock_parse.called; assert "response_format" not in mock_create.call_args.kwargs. Imports needed: pytest, unittest.mock (AsyncMock, MagicMock, patch), ai.generic_openai_provider.GenericOpenAIProvider, ai.openai_provider.OpenAIProvider, ai.provider_config.ProviderConfig, ai.utils.parse_classification. + + + cd backend && pytest tests/test_ai_providers.py::test_client_singleton tests/test_ai_providers.py::test_generic_openai_json_mode tests/test_ai_providers.py::test_context_chars_truncation tests/test_ai_providers.py::test_smart_truncation tests/test_ai_providers.py::test_gemini_fallback_to_parse_classification -v + + + - Source assertion: backend/tests/test_ai_providers.py defines test_client_singleton/test_generic_openai_json_mode/test_context_chars_truncation/test_smart_truncation/test_gemini_fallback_to_parse_classification without @pytest.mark.xfail decorators on those five functions + - Behavior: pytest -v reports 5 passed for those five test ids + - Behavior: test_gemini_fallback_to_parse_classification asserts parse_classification was invoked AND response_format is absent (D-02 contract enforcement) + - Behavior: pytest backend/tests/ -v shows no new failures + + Five Wave-2 tests green (including D-02 fallback coverage); total xfail count down by 5; full suite still passes. + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| Provider class → external LLM API | api_key transits over TLS; client owns the only outbound auth header | +| Untrusted document text → provider.classify() | content is the user's own; truncation prevents context blowout but is not a security boundary | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Disposition | Mitigation Plan | +|-----------|----------|-----------|-------------|-----------------| +| T-07-04 | Tampering | OpenAI SDK 2.34+ empty api_key rejection | mitigate | Factory normalizes empty api_key to "not-needed" placeholder; never passes "" to AsyncOpenAI ctor (Pitfall 1 in RESEARCH.md) | +| T-07-05 | Information Disclosure | Singleton _client retained across calls | accept | Each Celery task creates its own asyncio.run() event loop and a fresh ProviderConfig → fresh provider instance → fresh client; no cross-task sharing per RESEARCH.md D-07 | +| T-07-SC | Tampering | No new packages | accept | RESEARCH.md Package Legitimacy Audit: zero new packages; only anthropic floor raised; pip audit re-runs at phase gate | + + + +- grep -v '^#' backend/ai/openai_provider.py | grep -c 'MAX_AI_CHARS' returns 0. +- grep -v '^#' backend/services/classifier.py | grep -c 'MAX_AI_CHARS' returns 0. +- D-02 enforcement: `grep "from ai.utils import parse_classification" backend/ai/generic_openai_provider.py` returns a match. +- D-02 invariant: `grep -c "def parse_classification" backend/ai/utils.py` returns 1 (file untouched). +- pytest backend/tests/test_ai_providers.py::test_get_provider_typed test_client_singleton test_generic_openai_json_mode test_context_chars_truncation test_smart_truncation test_gemini_fallback_to_parse_classification exits 0. +- pytest backend/tests/ -v shows no new failures. +- requirements.txt contains an anthropic>=0.95.0 (or higher) pin. + + + +- ProviderConfig + PROVIDER_DEFAULTS + SUPPORTS_JSON_MODE present in backend/ai/provider_config.py. +- GenericOpenAIProvider class exists and subclasses OpenAIProvider with JSON-mode conditional on supports_json_mode. +- GenericOpenAIProvider imports parse_classification + parse_suggestions from ai.utils (D-02). +- backend/ai/utils.py unchanged — parse_classification and parse_suggestions remain the single canonical fallback (D-02). +- OpenAIProvider singleton client lifecycle implemented. +- Ollama/LMStudio shims carry context_chars. +- ai/__init__.py registry-based get_provider() with typed signature. +- MAX_AI_CHARS removed from openai_provider.py and classifier.py. +- anthropic SDK floor bumped to >=0.95.0 in requirements.txt. +- 6 previously-xfailed tests now pass (test_get_provider_typed + 5 from Task 4 including the D-02 Gemini fallback test). + + + +Create `.planning/phases/07-redo-and-optimize-llm-integration/07-02-SUMMARY.md` when done. + diff --git a/.planning/phases/07-redo-and-optimize-llm-integration/07-03-PLAN.md b/.planning/phases/07-redo-and-optimize-llm-integration/07-03-PLAN.md new file mode 100644 index 0000000..e4018ce --- /dev/null +++ b/.planning/phases/07-redo-and-optimize-llm-integration/07-03-PLAN.md @@ -0,0 +1,253 @@ +--- +phase: 07-redo-and-optimize-llm-integration +plan: 03 +type: execute +wave: 3 +depends_on: + - 07-02 +files_modified: + - backend/ai/anthropic_provider.py + - backend/ai/__init__.py + - backend/services/classifier.py + - backend/services/ai_config.py + - backend/tests/test_ai_providers.py + - backend/tests/test_ai_config.py +autonomous: true +requirements: + - D-03 + - D-04 + - D-06 + - D-07 + - D-12 + - D-13 + +must_haves: + truths: + - "AnthropicProvider stores self._client = AsyncAnthropic(...) in __init__ and never recreates it" + - "AnthropicProvider.classify() passes output_config={\"format\": {\"type\": \"json_schema\", \"schema\": _CLASSIFICATION_SCHEMA}} to messages.create()" + - "AnthropicProvider.suggest_topics() passes output_config={\"format\": {\"type\": \"json_schema\", \"schema\": _SUGGESTIONS_SCHEMA}} to messages.create()" + - "AnthropicProvider has no MAX_AI_CHARS constant and uses self._truncate(document_text)" + - "classifier.classify_document calls load_provider_config(session) and passes a ProviderConfig to get_provider" + - "load_provider_config returns the real ProviderConfig from ai/provider_config.py (stub removed)" + - "Registry get_provider now passes base_url+context_chars to AnthropicProvider as well" + artifacts: + - path: "backend/ai/anthropic_provider.py" + provides: "Singleton AsyncAnthropic + output_config structured output + smart truncation" + contains: "output_config" + - path: "backend/services/classifier.py" + provides: "ProviderConfig-driven classifier (no inline dict construction)" + contains: "load_provider_config" + - path: "backend/services/ai_config.py" + provides: "ProviderConfig stub replaced by import from ai.provider_config" + contains: "from ai.provider_config import ProviderConfig" + key_links: + - from: "backend/services/classifier.py::classify_document" + to: "backend/services/ai_config.py::load_provider_config" + via: "async session.execute path" + pattern: "await load_provider_config" + - from: "backend/ai/anthropic_provider.py::classify" + to: "anthropic SDK messages.create" + via: "output_config kwarg" + pattern: "output_config" + - from: "backend/ai/__init__.py::get_provider anthropic branch" + to: "AnthropicProvider.__init__" + via: "widened ctor signature (context_chars)" + pattern: "context_chars" +--- + + +Complete the provider refactor by giving AnthropicProvider the same singleton+context_chars+output_config treatment as the OpenAI side, and replace the classifier's inline `_settings` dict with a real ProviderConfig sourced from the database. + +Purpose: Resolve D-03 (Anthropic structured output via `output_config.format.type="json_schema"`) using the constrained-decoding API that GA'd in anthropic SDK 0.95+, finish the D-07 singleton client cleanup, and close out D-06 by routing the classifier through `load_provider_config()` instead of the legacy env-var dict shim. +Output: Refactored anthropic_provider.py (output_config + singleton + truncation, MAX_AI_CHARS removed), classifier.py talking to ai_config.load_provider_config, ai_config.py stub removed in favour of the real ProviderConfig, registry updated to pass base_url/context_chars to AnthropicProvider, and the matching Anthropic + classifier integration tests promoted from xfail. + + + +@$HOME/.claude/get-shit-done/workflows/execute-plan.md +@$HOME/.claude/get-shit-done/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-CONTEXT.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-RESEARCH.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-PATTERNS.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-01-SUMMARY.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-02-SUMMARY.md +@backend/ai/anthropic_provider.py +@backend/ai/openai_provider.py +@backend/ai/provider_config.py +@backend/ai/__init__.py +@backend/services/classifier.py +@backend/services/ai_config.py + + + + +From backend/ai/anthropic_provider.py (current state before this plan): +- `MAX_AI_CHARS = 8_000` module constant +- `class AnthropicProvider(AIProvider)` +- `def __init__(self, api_key: str, model: str = "claude-sonnet-4-6")` — narrow ctor +- `def _client(self)` — to be removed +- `async def classify(...)` / `async def suggest_topics(...)` / `async def health_check(...)` calling `await client.messages.create(...)` + +Anthropic SDK output_config schema (07-RESEARCH.md D-03): +- `messages.create(..., output_config={"format": {"type": "json_schema", "schema": }})` +- Response read from `response.content[0].text` only when `response.stop_reason == "end_turn"`; otherwise call parse_classification("") for graceful degradation +- Required schema fields all listed in `required` array; `additionalProperties: False` + +From backend/ai/provider_config.py (introduced in Plan 02): +- `ProviderConfig(BaseModel)` with provider_id/api_key/base_url/model/context_chars +- `PROVIDER_DEFAULTS["anthropic"] = {"base_url": None, "model": "claude-sonnet-4-6", "context_chars": 180000}` + +From backend/services/ai_config.py (introduced in Plan 01): +- `async def load_provider_config(session) -> ProviderConfig | None` — currently imports the _ProviderConfigStub; this plan switches it to import from ai.provider_config + +From backend/services/classifier.py (current — lines 57-64): +- Inline `_settings = {"active_provider": _ai_provider, "providers": {_ai_provider: {"model": _ai_model}}}` then `provider = get_provider(_settings)` +- Old signature `async def classify_document(document_id, session, ai_provider: str | None = None, ai_model: str | None = None)` + +Module-level _DEFAULT_SYSTEM_PROMPT remains in classifier.py. + + + + + + + Task 1: AnthropicProvider — singleton, output_config, truncation + + backend/ai/anthropic_provider.py + backend/ai/openai_provider.py + backend/ai/utils.py + backend/ai/provider_config.py + backend/ai/__init__.py + + + - AnthropicProvider.__init__(api_key, model, context_chars, base_url=None) stores self._client = AsyncAnthropic(api_key=api_key) once; base_url is accepted but ignored (anthropic SDK uses its default; signature widened so the factory can pass uniformly) + - AnthropicProvider has no MAX_AI_CHARS constant, no `_client(self)` method + - AnthropicProvider has _truncate(text) implementing the 60/40 split + - Module-level _CLASSIFICATION_SCHEMA and _SUGGESTIONS_SCHEMA dicts match the structures defined in 07-RESEARCH.md (assigned_topics + new_topic_suggestions + reasoning required for classification; suggested_topics required for suggestions; additionalProperties False) + - classify() and suggest_topics() pass output_config={"format": {"type": "json_schema", "schema": _CLASSIFICATION_SCHEMA / _SUGGESTIONS_SCHEMA}} to messages.create + - When response.stop_reason != "end_turn" the raw text is replaced by "" so parse_classification/parse_suggestions degrade gracefully + - get_provider in ai/__init__.py now invokes AnthropicProvider with api_key+model+context_chars+base_url (uniform call signature) + - test_ai_providers.py::test_anthropic_structured_output promoted to passing (mocks anthropic.AsyncAnthropic.messages.create and asserts output_config kwarg) + + + Refactor backend/ai/anthropic_provider.py: delete `MAX_AI_CHARS = 8_000`. Change __init__ signature to `def __init__(self, api_key: str, model: str, context_chars: int, base_url: str | None = None)`. Set self._api_key, self._model, self._context_chars, then self._client = anthropic.AsyncAnthropic(api_key=self._api_key). Delete the `def _client(self)` method. Add `def _truncate(self, text: str) -> str` body identical to OpenAIProvider._truncate (60/40 split using self._context_chars). + + Add module-level constants: + `_CLASSIFICATION_SCHEMA = {"type": "object", "properties": {"assigned_topics": {"type": "array", "items": {"type": "string"}}, "new_topic_suggestions": {"type": "array", "items": {"type": "string"}}, "reasoning": {"type": "string"}}, "required": ["assigned_topics", "new_topic_suggestions"], "additionalProperties": False}` + `_SUGGESTIONS_SCHEMA = {"type": "object", "properties": {"suggested_topics": {"type": "array", "items": {"type": "string"}}}, "required": ["suggested_topics"], "additionalProperties": False}` + + Rewrite classify(): truncated = self._truncate(document_text); topics_str composed as before; user_msg as before; await self._client.messages.create(model=self._model, max_tokens=1024, system=system_prompt, messages=[{"role":"user","content":user_msg}], output_config={"format": {"type": "json_schema", "schema": _CLASSIFICATION_SCHEMA}}); raw = response.content[0].text if response.content and getattr(response, "stop_reason", "end_turn") == "end_turn" else ""; return parse_classification(raw). + + Rewrite suggest_topics(): same shape but output_config uses _SUGGESTIONS_SCHEMA and returns parse_suggestions(raw). + + Rewrite health_check(): await self._client.messages.create(model=self._model, max_tokens=8, messages=[{"role":"user","content":"ping"}]); return True on success, False on Exception. Do not pass output_config in health_check (the response shape doesn't matter; this just verifies credentials/connectivity). + + Update backend/ai/__init__.py get_provider(): in the registry branch for "anthropic", call `cls(api_key=effective_api_key, model=effective_model, context_chars=effective_context_chars, base_url=effective_base_url)`. Remove the special-case branch from Plan 02 (the comment "AnthropicProvider does not take base_url" is no longer true after this task). + + Promote test_ai_providers.py::test_anthropic_structured_output: build an AnthropicProvider(api_key="k", model="claude-sonnet-4-6", context_chars=100); patch ai.anthropic_provider.anthropic.AsyncAnthropic with a MagicMock whose messages.create is an AsyncMock returning a stub response (object with .content=[MagicMock(text='{"assigned_topics":[],"new_topic_suggestions":[]}')] and .stop_reason="end_turn"); call await provider.classify("doc", [], "sys"); assert mock_create.await_args.kwargs["output_config"] == {"format": {"type": "json_schema", "schema": _CLASSIFICATION_SCHEMA}}. Remove the @pytest.mark.xfail decorator. + + + cd backend && pytest tests/test_ai_providers.py::test_anthropic_structured_output -x -v && grep -c 'MAX_AI_CHARS' backend/ai/anthropic_provider.py + + + - Source assertion: grep -v '^#' backend/ai/anthropic_provider.py | grep -c 'MAX_AI_CHARS' returns 0 + - Source assertion: backend/ai/anthropic_provider.py contains `self._client = anthropic.AsyncAnthropic(`, `output_config=`, `_CLASSIFICATION_SCHEMA`, `_SUGGESTIONS_SCHEMA` + - Source assertion: backend/ai/anthropic_provider.py does NOT contain `def _client(self)` + - Source assertion: backend/ai/__init__.py passes `context_chars=` and `base_url=` to AnthropicProvider + - Behavior: pytest backend/tests/test_ai_providers.py::test_anthropic_structured_output exits 0 + - Behavior: response.stop_reason "max_tokens" or "refusal" → raw == "" → parse_classification("") returns an empty ClassificationResult (no crash) + + AnthropicProvider mirrors the OpenAI-side refactor (singleton, truncation, no MAX_AI_CHARS); classify/suggest call output_config with the two schemas; registry passes uniform kwargs; test_anthropic_structured_output is green. + + + + Task 2: Classifier wired to load_provider_config + ai_config stub removal + + backend/services/classifier.py + backend/services/ai_config.py + backend/ai/provider_config.py + backend/ai/__init__.py + backend/tasks/document_tasks.py + backend/tests/test_classifier.py + backend/tests/test_ai_config.py + + + - backend/services/ai_config.py imports `from ai.provider_config import ProviderConfig` at module top (lazy import inside load_provider_config removed) + - The _ProviderConfigStub class defined in ai_config.py is deleted + - backend/services/classifier.py calls `config = await load_provider_config(session)` and `provider = get_provider(config)` — no inline `_settings = {...}` dict construction remains + - classify_document keeps optional ai_provider/ai_model kwargs (per-user override from ADMIN-05 still honored): when either kwarg is non-None, build a ProviderConfig override (start from PROVIDER_DEFAULTS[user provider], merge user-provided model) and skip the DB load + - When load_provider_config returns None (no active row), fall back to a ProviderConfig built from settings.default_ai_provider/default_ai_model + PROVIDER_DEFAULTS — preserves existing default-AI behaviour from STATE.md ("Default AI provider is ollama/llama3.2") + - The classifier no longer slices document_text — truncation happens inside provider._truncate() + - tests/test_classifier.py still passes (existing behavior preserved) + - tests/test_ai_config.py::test_load_provider_config and tests/test_ai_config.py::test_api_key_encrypt_decrypt promoted to passing + + + Edit backend/services/ai_config.py: remove the `_ProviderConfigStub` class entirely; replace the lazy import inside load_provider_config with a module-top `from ai.provider_config import ProviderConfig, PROVIDER_DEFAULTS`. load_provider_config(session) reads the row WHERE is_active = True via select(SystemSettings).where(SystemSettings.is_active.is_(True)) (use `.is_(True)` for SQLAlchemy comparison, not `==`); if no row found return None; otherwise decrypt api_key_enc using settings.cloud_creds_key when not None (else api_key=""); build and return ProviderConfig(provider_id=row.provider_id, api_key=decrypted_api_key, base_url=row.base_url, model=row.model_name, context_chars=row.context_chars). Import AsyncSession from sqlalchemy.ext.asyncio. Import select from sqlalchemy. Use base64 + bytes(settings.cloud_creds_key, "utf-8") if cloud_creds_key is a str — match the existing pattern in cloud_utils. + + Edit backend/services/classifier.py: at the top of `async def classify_document`, replace the inline dict construction (lines ~57-64 per 07-PATTERNS.md) with: + 1. `config = await load_provider_config(session)` (import `from services.ai_config import load_provider_config` at module top) + 2. If ai_provider is not None (per-user override path): build `config = ProviderConfig(provider_id=ai_provider, model=ai_model or PROVIDER_DEFAULTS.get(ai_provider, {}).get("model", ""), api_key="", base_url=None, context_chars=PROVIDER_DEFAULTS.get(ai_provider, {}).get("context_chars", 8000))` (per-user overrides do not carry an API key in this codebase — the system_settings api_key is still authoritative; when the per-user override changes the provider away from the active system provider, api_key remains empty and get_provider falls back to "not-needed". Document this in a comment: "per-user override does not carry an api_key — admin must configure each provider's key in system_settings".) + 3. If config is None: fallback ProviderConfig(provider_id=app_settings.default_ai_provider, model=app_settings.default_ai_model, base_url=None, context_chars=PROVIDER_DEFAULTS.get(app_settings.default_ai_provider, {}).get("context_chars", 8000), api_key=""). + 4. `provider = get_provider(config)` (unchanged call). + Apply the same load/override/fallback logic in `suggest_topics_for_document`. Remove any remaining `document_text[:MAX_AI_CHARS]` slices; pass the full document_text to provider.classify/provider.suggest_topics (the provider truncates internally). + + Promote backend/tests/test_ai_config.py::test_load_provider_config and ::test_api_key_encrypt_decrypt. For test_api_key_encrypt_decrypt: a simple unit test using a 32-byte master key, encrypt_api_key + decrypt_api_key round-trip with provider_id="openai", assert plaintext recovered; assert encrypting with provider_id="anthropic" yields different ciphertext (domain salt isolation). For test_load_provider_config: requires DB — gate with `pytest.importorskip("psycopg")` and `@pytest.mark.skipif(not os.getenv("INTEGRATION"), reason="needs PostgreSQL")`; insert a SystemSettings row with is_active=True via the async session fixture; encrypt a test api_key; await load_provider_config(session); assert returned ProviderConfig has the expected fields and decrypted api_key. Remove xfail decorators from both. + + + cd backend && pytest tests/test_ai_config.py::test_api_key_encrypt_decrypt tests/test_classifier.py -x -v + + + - Source assertion: backend/services/ai_config.py contains `from ai.provider_config import ProviderConfig` at module level (grep on the top 30 lines) + - Source assertion: backend/services/ai_config.py does NOT contain `_ProviderConfigStub` + - Source assertion: backend/services/classifier.py contains `await load_provider_config(` and `get_provider(config)` + - Source assertion: backend/services/classifier.py does NOT contain `"active_provider"` or `"providers": {` + - Source assertion: grep -v '^#' backend/services/classifier.py | grep -c 'MAX_AI_CHARS' returns 0 + - Behavior: pytest backend/tests/test_classifier.py exits 0 + - Behavior: pytest backend/tests/test_ai_config.py::test_api_key_encrypt_decrypt exits 0 + - Behavior: INTEGRATION=1 pytest backend/tests/test_ai_config.py::test_load_provider_config exits 0 when DB is available (test is skipped otherwise) + + ai_config.py imports ProviderConfig directly; stub class gone; classifier uses load_provider_config + per-user/system fallback; truncation lives only in providers; two test_ai_config.py tests promoted; existing test_classifier.py still green. + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| services/ai_config.load_provider_config → providers | decrypted api_key crosses here in-memory only; never serialized | +| AnthropicProvider → anthropic.com API | api_key transits over TLS; SDK manages outbound auth | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Disposition | Mitigation Plan | +|-----------|----------|-----------|-------------|-----------------| +| T-07-06 | Information Disclosure | classifier per-user override path | mitigate | per-user override does not carry api_key; system_settings is the only key store; api_key never leaves backend services tier | +| T-07-07 | Tampering | Anthropic output_config grammar limit | accept | Pitfall 5 RESEARCH.md — schemas are simple (≤4 required fields, no unions); will not exceed Anthropic's 24-optional / 16-union limits | +| T-07-08 | Denial of Service | Anthropic stop_reason "refusal"/"max_tokens" | mitigate | classify() falls back to parse_classification("") which returns empty ClassificationResult; document status set by caller (Plan 04 retry logic) | +| T-07-SC | Tampering | anthropic SDK pin raised to >=0.95.0 | accept | RESEARCH.md confirms package legitimacy (official Anthropic SDK, ~3 yrs old); pip audit re-runs at phase gate | + + + +- grep -v '^#' backend/ai/anthropic_provider.py | grep -c 'MAX_AI_CHARS' returns 0. +- grep -F 'output_config' backend/ai/anthropic_provider.py returns 2+ matches (one per classify/suggest call). +- grep -F '_ProviderConfigStub' backend/services/ai_config.py returns 0. +- pytest backend/tests/test_ai_providers.py::test_anthropic_structured_output backend/tests/test_classifier.py backend/tests/test_ai_config.py::test_api_key_encrypt_decrypt exits 0. +- pytest backend/tests/ -v shows no new failures. + + + +- AnthropicProvider singleton + output_config + truncation + no MAX_AI_CHARS. +- classifier.classify_document driven by load_provider_config with per-user override + env fallback. +- ai_config.py stub removed; real ProviderConfig used everywhere. +- 3 previously-xfailed tests now pass (test_anthropic_structured_output, test_api_key_encrypt_decrypt, test_load_provider_config under INTEGRATION=1). +- Existing test_classifier.py still green. + + + +Create `.planning/phases/07-redo-and-optimize-llm-integration/07-03-SUMMARY.md` when done. + diff --git a/.planning/phases/07-redo-and-optimize-llm-integration/07-04-PLAN.md b/.planning/phases/07-redo-and-optimize-llm-integration/07-04-PLAN.md new file mode 100644 index 0000000..b764292 --- /dev/null +++ b/.planning/phases/07-redo-and-optimize-llm-integration/07-04-PLAN.md @@ -0,0 +1,224 @@ +--- +phase: 07-redo-and-optimize-llm-integration +plan: 04 +type: execute +wave: 4 +depends_on: + - 07-03 +files_modified: + - backend/tasks/document_tasks.py + - backend/api/documents.py + - backend/tests/test_document_tasks.py + - backend/tests/test_documents.py +autonomous: true +requirements: + - D-09 + - D-10 + - D-11 + +must_haves: + truths: + - "extract_and_classify task decorator carries bind=True and max_retries=3" + - "Classification failures raise _ClassificationError from _run; the outer sync task catches it and calls self.retry(countdown=[30,90,270][min(retries,2)])" + - "MaxRetriesExceededError handler writes doc.status = 'classification_failed' via _mark_classification_failed" + - "POST /api/documents/{id}/classify sets doc.status='processing', commits, calls extract_and_classify.delay(doc_id), returns {'document_id': str, 'status': 'processing'}" + - "Non-classification failures (extract_failed, invalid_id) still return a dict and are not retried" + artifacts: + - path: "backend/tasks/document_tasks.py" + provides: "Celery retry harness + _ClassificationError + _mark_classification_failed" + contains: "class _ClassificationError" + - path: "backend/api/documents.py" + provides: "Re-queue behaviour on POST /{id}/classify" + contains: "extract_and_classify.delay" + key_links: + - from: "backend/tasks/document_tasks.py::extract_and_classify" + to: "Celery self.retry" + via: "exponential backoff countdowns" + pattern: "self\\.retry\\(.*countdown" + - from: "backend/api/documents.py POST /{id}/classify" + to: "extract_and_classify.delay" + via: "Celery enqueue" + pattern: "extract_and_classify\\.delay" + - from: "extract_and_classify MaxRetriesExceededError handler" + to: "_mark_classification_failed" + via: "final status writeback" + pattern: "classification_failed" +--- + + +Make the classification pipeline self-healing by adding Celery exponential-backoff retry around `extract_and_classify`, and convert the synchronous `POST /api/documents/{id}/classify` endpoint into a re-queue trigger so the new "Re-analyze" UI button (Plan 05) drives the same retry loop. + +Purpose: Deliver D-09 (30s/90s/270s exponential backoff with max_retries=3), D-10 (final state remains `classification_failed`), and D-11 backend half (re-classify endpoint re-queues Celery instead of running synchronously). Apply Pitfall 3 from RESEARCH.md — `self.retry()` must escape the sync task layer, not the inner asyncio.run() — by introducing a `_ClassificationError` sentinel. +Output: Refactored extract_and_classify task with bind=True, _ClassificationError sentinel, _mark_classification_failed helper, retry-with-countdown logic, and a refactored POST /classify endpoint that calls .delay(); matching tests promoted from xfail. + + + +@$HOME/.claude/get-shit-done/workflows/execute-plan.md +@$HOME/.claude/get-shit-done/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-CONTEXT.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-RESEARCH.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-PATTERNS.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-03-SUMMARY.md +@backend/tasks/document_tasks.py +@backend/api/documents.py +@backend/services/classifier.py +@backend/tests/test_documents.py +@backend/tests/test_document_tasks.py + + + + +From backend/tasks/document_tasks.py (current): +- `@celery_app.task(name="tasks.document_tasks.extract_and_classify") def extract_and_classify(document_id: str) -> dict:` calls `return asyncio.run(_run(document_id))` +- `async def _run(document_id: str) -> dict` performs UUID parse, DB load, MinIO fetch, extract text, then await classify_document(...) +- Current behaviour: on classification exception, sets `doc.status = "classification_failed"` and returns `{"document_id": ..., "status": "classification_failed"}` directly (no retry) +- A second task `cleanup_abandoned_uploads` already follows the asyncio.run bridge pattern + +From backend/api/documents.py (current POST /{doc_id}/classify): +- Authenticated route via Depends(get_regular_user); ownership check returns 404 on mismatch (per STATE.md "Cross-user doc access returns 404 not 403") +- Currently `await classifier.classify_document(doc_id=..., session=..., ai_provider=..., ai_model=...)` synchronously and returns a dict containing assigned topics +- Receives only doc_id and current_user from auth context + +Celery retry API (RESEARCH.md Pattern 4): +- `@celery_app.task(bind=True, max_retries=3, name=...)` signature requires `self` as first positional arg +- `self.request.retries` reports the current retry attempt (0 on first try) +- `raise self.retry(exc=exc, countdown=N)` schedules a retry; raises Retry exception +- `self.MaxRetriesExceededError` is the exception type raised by Celery when retries are exhausted + +Important — asyncio.run + AsyncMock interaction (test contract): +- Production code calls `asyncio.run(_mark_classification_failed(document_id))`. asyncio.run() invokes the coroutine object returned by calling _mark_classification_failed(...); this is a `__call__()` on the patched function, NOT a `__await__()`. +- An AsyncMock substitute for _mark_classification_failed therefore records the invocation via `mock.call_args` / `mock.call_count`, NOT via `await_count`. Tests must assert via `assert_called_once_with(document_id)`, not `assert_awaited_once_with(...)`. + + + + + + + Task 1: Celery retry harness + _ClassificationError + classification_failed writeback + + backend/tasks/document_tasks.py + backend/services/classifier.py + backend/db/models.py + backend/db/session.py + backend/tests/test_document_tasks.py + + + - Module-level `class _ClassificationError(Exception)` exists + - `async def _mark_classification_failed(document_id: str) -> None` exists, opens an AsyncSession, loads the Document, sets status="classification_failed", commits + - `_run()` raises `_ClassificationError(str(exc))` from the existing classification try/except (lines ~109-124 per 07-PATTERNS.md) — for classification exceptions only + - Extract/storage/validation failures still `return {... "status": "extract_failed" ...}` or `return {... "status": "invalid_id" ...}` and are not retried + - The task decorator becomes `@celery_app.task(name="tasks.document_tasks.extract_and_classify", bind=True, max_retries=3)` + - Task signature is `def extract_and_classify(self, document_id: str) -> dict` + - try/except outside asyncio.run handles `_ClassificationError`: countdown = [30, 90, 270][min(self.request.retries, 2)]; raise self.retry(exc=exc, countdown=countdown) + - Separate handler catches `self.MaxRetriesExceededError` (or `MaxRetriesExceededError` imported from celery.exceptions) and calls `asyncio.run(_mark_classification_failed(document_id))`; returns {"document_id": document_id, "status": "classification_failed"} + - test_document_tasks.py::test_retry_backoff and ::test_exhaustion_sets_failed_status promoted to passing + + + Refactor backend/tasks/document_tasks.py: + 1. Add `from celery.exceptions import MaxRetriesExceededError` near the existing celery imports. + 2. Add `class _ClassificationError(Exception): pass` at module level above the task. + 3. Add `async def _mark_classification_failed(document_id: str) -> None:` that opens an AsyncSession via the existing session factory, calls `await session.get(Document, _uuid.UUID(document_id))`, sets doc.status = "classification_failed", commits. Use the same imports the existing _run uses for session + Document. + 4. Modify _run(): in the existing try/except around the classification call (the block that currently sets `doc.status = "classification_failed"` and returns a dict on classification exception), replace the except body with `raise _ClassificationError(str(exc)) from exc`. The non-classification failure returns (invalid_id, extract_failed, storage failures) are preserved unchanged. + 5. Change the task decorator from `@celery_app.task(name="tasks.document_tasks.extract_and_classify")` to `@celery_app.task(name="tasks.document_tasks.extract_and_classify", bind=True, max_retries=3)`. + 6. Change the task signature from `def extract_and_classify(document_id: str)` to `def extract_and_classify(self, document_id: str)`. + 7. Wrap the body as: try: return asyncio.run(_run(document_id)) except _ClassificationError as exc: countdowns = [30, 90, 270]; countdown = countdowns[min(self.request.retries, 2)]; raise self.retry(exc=exc, countdown=countdown) except MaxRetriesExceededError: asyncio.run(_mark_classification_failed(document_id)); return {"document_id": document_id, "status": "classification_failed"}. + + Promote backend/tests/test_document_tasks.py::test_retry_backoff: use unittest.mock.patch on `tasks.document_tasks._run` so it raises `_ClassificationError("fake")`; construct a MagicMock `self` with `request.retries` settable; assert that calling `extract_and_classify.run(self, "")` raises celery.exceptions.Retry; iterate retries=0,1,2 and assert the Retry's `countdown` attribute equals 30, 90, 270 respectively. (Use celery's `extract_and_classify.run` to bypass the Celery messaging layer and invoke the wrapped function directly.) + + Promote test_exhaustion_sets_failed_status: patch `tasks.document_tasks._run` to raise `_ClassificationError`; patch `tasks.document_tasks._mark_classification_failed` with an AsyncMock; configure `self.retry` to raise `MaxRetriesExceededError`; call extract_and_classify.run(self, ""); assert `mock_mark_failed.assert_called_once_with(document_id)` — NOT `assert_awaited_once_with`. Rationale: production code is `asyncio.run(_mark_classification_failed(document_id))`. The coroutine factory call (`_mark_classification_failed(document_id)`) is recorded as `call`, not `await`, on the AsyncMock; asyncio.run() then runs the resulting coroutine to completion (which on an AsyncMock returns immediately). Therefore the assertion must inspect `call_args`/`call_count`, not `await_count`. Also assert the return dict has status="classification_failed". Remove the xfail decorators from both tests. + + + cd backend && pytest tests/test_document_tasks.py::test_retry_backoff tests/test_document_tasks.py::test_exhaustion_sets_failed_status -x -v + + + - Source assertion: backend/tasks/document_tasks.py contains `class _ClassificationError(Exception)`, `bind=True`, `max_retries=3`, `def extract_and_classify(self, document_id`, `countdowns = [30, 90, 270]`, `MaxRetriesExceededError`, `async def _mark_classification_failed` + - Source assertion: backend/tasks/document_tasks.py contains `raise self.retry(exc=` once + - Source assertion (test contract): backend/tests/test_document_tasks.py test_exhaustion_sets_failed_status uses `assert_called_once_with` (not `assert_awaited_once_with`) on the _mark_classification_failed mock — `grep "assert_called_once_with" backend/tests/test_document_tasks.py` returns a match and `grep "assert_awaited_once_with" backend/tests/test_document_tasks.py` returns no match for the _mark_classification_failed assertion line + - Behavior: pytest backend/tests/test_document_tasks.py::test_retry_backoff exits 0 and asserts countdown values [30, 90, 270] + - Behavior: pytest backend/tests/test_document_tasks.py::test_exhaustion_sets_failed_status exits 0 + - Behavior: pytest backend/tests/ -v shows no new failures + + Celery retry harness in place; classification failures retry with 30/90/270 backoff; exhaustion writes classification_failed; two test stubs promoted (test_exhaustion_sets_failed_status uses assert_called_once_with to match the asyncio.run(coro) calling convention); rest of suite green. + + + + Task 2: POST /api/documents/{id}/classify → re-queue Celery + + backend/api/documents.py + backend/tasks/document_tasks.py + backend/services/classifier.py + backend/tests/test_documents.py + backend/db/models.py + + + - POST /api/documents/{doc_id}/classify is `async def`, depends on get_db + get_regular_user (unchanged) + - Loads the document via existing _get_owned_doc helper (raises 404 on wrong owner per STATE.md "Cross-user doc access returns 404 not 403") — keep the helper, do not duplicate ownership logic + - Sets doc.status = "processing", awaits session.commit() + - Calls extract_and_classify.delay(str(doc.id)) + - Returns {"document_id": str(doc.id), "status": "processing"} with HTTP 200 + - Existing synchronous behaviour (await classifier.classify_document(...) + return assigned topics) is removed + - test_documents.py::test_reclassify_requeues_celery promoted to passing with extract_and_classify.delay monkeypatched + + + Edit backend/api/documents.py: locate the existing POST /{doc_id}/classify route. Import `from tasks.document_tasks import extract_and_classify` (lazy import inside the function body to avoid circular import at module load — match the existing deferred-import pattern used elsewhere in the codebase per STATE.md decision "Deferred Celery import in /password-reset"). Replace the function body with: doc = await _get_owned_doc(doc_id, current_user, session) (use whatever the existing helper name is in documents.py — verify via grep before editing; if no helper exists, replicate the same select+ownership check the existing POST /{doc_id}/classify already performs). Then `doc.status = "processing"`. Then `await session.commit()`. Then `from tasks.document_tasks import extract_and_classify` followed by `extract_and_classify.delay(str(doc.id))`. Then `return {"document_id": str(doc.id), "status": "processing"}`. Remove any call to `await classifier.classify_document(...)` from this endpoint body. Remove any code that returns assigned topics from this endpoint — the topics will be written by the Celery task and clients should re-fetch the document after polling. + + Promote backend/tests/test_documents.py::test_reclassify_requeues_celery: as an authenticated regular user, POST to /api/documents/{doc_id}/classify with monkeypatch on `tasks.document_tasks.extract_and_classify.delay` (a MagicMock). Confirm: response status is 200; response JSON contains status="processing" and document_id matching the doc; mock_delay was called exactly once with str(doc.id); after the request the Document row in DB has status="processing". Use the existing auth_user/admin_user fixtures from conftest.py per STATE.md "Celery mock required in /confirm tests". Remove the xfail decorator. + + + cd backend && pytest tests/test_documents.py::test_reclassify_requeues_celery -x -v + + + - Source assertion: backend/api/documents.py POST /{doc_id}/classify body contains `extract_and_classify.delay(` + - Source assertion: backend/api/documents.py POST /{doc_id}/classify body contains `doc.status = "processing"` and `await session.commit()` + - Source assertion: backend/api/documents.py POST /{doc_id}/classify body does NOT contain `await classifier.classify_document(` + - Behavior: pytest backend/tests/test_documents.py::test_reclassify_requeues_celery exits 0 + - Behavior: full suite pytest backend/tests/ -v has no new failures + - Behavior: cross-user POST /api/documents/{other_users_doc_id}/classify still returns 404 (regression of existing IDOR test) + + POST /classify re-queues instead of running synchronously; doc moves to processing; Celery task takes over; test_reclassify_requeues_celery passes; existing IDOR/auth coverage intact. + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| Authenticated user → POST /classify | doc_id crosses untrusted boundary; ownership re-checked via _get_owned_doc → 404 on mismatch | +| Celery worker process → DB | retry exponential backoff bounded (max_retries=3) — prevents runaway loops | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Disposition | Mitigation Plan | +|-----------|----------|-----------|-------------|-----------------| +| T-07-09 | Denial of Service | Re-classify endpoint enqueue without rate limit | accept | per-user rate-limit infrastructure lives on auth endpoints (Phase 2) and is being expanded by Phase 6; reclassify endpoint is gated by ownership check and inherits Phase 6 per-account limiter once that ships; documenting accepted risk for v1 | +| T-07-10 | Tampering | Cross-user reclassify | mitigate | _get_owned_doc returns 404 on cross-user (matches STATE.md cross-user 404 policy); existing IDOR test in test_documents.py validates this | +| T-07-11 | Information Disclosure | Retry exception payload | accept | _ClassificationError carries only the str(exc); no document content; Celery serializes exc message into the broker but RabbitMQ/Redis is internal-only | +| T-07-12 | Tampering | self.retry mid-asyncio.run | mitigate | Pitfall 3 — retry raised from the outer sync layer, never inside asyncio.run; _ClassificationError sentinel enforces this boundary | +| T-07-SC | Tampering | No new packages | accept | celery already pinned; pip audit re-runs at phase gate | + + + +- grep -F 'bind=True' backend/tasks/document_tasks.py returns the task decorator line. +- grep -F 'extract_and_classify.delay' backend/api/documents.py returns the endpoint enqueue line. +- pytest backend/tests/test_document_tasks.py backend/tests/test_documents.py::test_reclassify_requeues_celery exits 0. +- pytest backend/tests/ -v shows no new failures (test_documents.py IDOR + auth tests still green). +- grep -F 'await classifier.classify_document' backend/api/documents.py POST /{doc_id}/classify body returns 0. +- test_exhaustion_sets_failed_status uses assert_called_once_with (asyncio.run(coro_factory(...)) calls the factory, not awaits it — see note). + + + +- Celery retry harness operational with 30/90/270 backoff and final classification_failed status. +- POST /{id}/classify is a re-queue endpoint; clients receive status=processing. +- 3 previously-xfailed tests now pass (test_retry_backoff, test_exhaustion_sets_failed_status, test_reclassify_requeues_celery). +- No regressions in existing test_documents.py IDOR/auth coverage. + + + +Create `.planning/phases/07-redo-and-optimize-llm-integration/07-04-SUMMARY.md` when done. + diff --git a/.planning/phases/07-redo-and-optimize-llm-integration/07-05-PLAN.md b/.planning/phases/07-redo-and-optimize-llm-integration/07-05-PLAN.md new file mode 100644 index 0000000..90f09b7 --- /dev/null +++ b/.planning/phases/07-redo-and-optimize-llm-integration/07-05-PLAN.md @@ -0,0 +1,380 @@ +--- +phase: 07-redo-and-optimize-llm-integration +plan: 05 +type: execute +wave: 5 +depends_on: + - 07-04 +files_modified: + - backend/api/admin.py + - backend/api/__init__.py + - backend/services/ai_config.py # Task 1 adds load_provider_config_by_id + - frontend/src/api/client.js + - frontend/src/components/admin/AdminAiConfigTab.vue + - frontend/src/components/documents/DocumentCard.vue + - backend/tests/test_admin_ai_config.py + - frontend/tests/api.spec.js # NEW Vitest tests for getAiConfig/saveAiConfig/testAiConnection (D-11 frontend coverage) + - frontend/tests/DocumentCard.spec.js # NEW Vitest test for reanalyze() (D-11 frontend coverage) +autonomous: false +requirements: + - D-05 + - D-08 + - D-11 + - D-15 + +must_haves: + truths: + - "GET /api/admin/ai-config returns a list of provider configs with provider_id, base_url, model_name, context_chars, is_active, updated_at — never api_key_enc" + - "PUT /api/admin/ai-config accepts {provider_id, api_key?, base_url?, model?, context_chars?, is_active?}; when api_key is provided it is HKDF-encrypted via encrypt_api_key and stored; when omitted the existing api_key_enc is left untouched" + - "PUT /api/admin/ai-config atomically flips is_active via a single UPDATE: SET is_active = (provider_id = $target) when is_active=true is requested" + - "GET /api/admin/ai-config/test-connection?provider_id=X loads the row, builds ProviderConfig, get_provider(config).health_check(), returns {\"ok\": bool}" + - "load_provider_config_by_id(session, provider_id) is added to backend/services/ai_config.py — filters by provider_id, ignores is_active so test-connection works on inactive rows" + - "AdminAiConfigTab.vue adds a Global AI Providers section ABOVE the existing per-user assignment table; per-user table remains untouched" + - "DocumentCard.vue shows a red 'Classification failed' badge with a 'Re-analyze' button when doc.status === 'classification_failed'" + - "Re-analyze button posts to /api/documents/{id}/classify via the existing classifyDocument client function; spinner displayed while in-flight" + - "All admin AI-config endpoints require get_current_admin and write audit_log entries with metadata containing only provider_id (never api_key)" + - "frontend/tests/api.spec.js Vitest tests verify getAiConfig/saveAiConfig/testAiConnection call request() with the expected method, path, and JSON body shape (D-11 frontend coverage; CLAUDE.md mandate that every new function has at least one test)" + - "frontend/tests/DocumentCard.spec.js Vitest test verifies reanalyze() calls classifyDocument with props.doc.id and emits 'reclassified' (D-11 frontend coverage)" + artifacts: + - path: "backend/api/admin.py" + provides: "GET/PUT /api/admin/ai-config + test-connection endpoint + _ai_config_to_dict whitelist" + contains: "_ai_config_to_dict" + - path: "backend/services/ai_config.py" + provides: "load_provider_config_by_id (added in Task 1)" + contains: "load_provider_config_by_id" + - path: "frontend/src/api/client.js" + provides: "getAiConfig/saveAiConfig/testAiConnection helpers" + contains: "getAiConfig" + - path: "frontend/src/components/admin/AdminAiConfigTab.vue" + provides: "Global system provider config section + per-provider accordion + test-connection button" + contains: "System AI Providers" + - path: "frontend/src/components/documents/DocumentCard.vue" + provides: "Classification failed badge + Re-analyze button" + contains: "classification_failed" + - path: "frontend/tests/api.spec.js" + provides: "Vitest unit tests for getAiConfig/saveAiConfig/testAiConnection" + contains: "getAiConfig" + - path: "frontend/tests/DocumentCard.spec.js" + provides: "Vitest unit test for DocumentCard.reanalyze() (D-11 frontend half)" + contains: "reanalyze" + key_links: + - from: "backend/api/admin.py PUT /api/admin/ai-config" + to: "backend/services/ai_config.py::encrypt_api_key" + via: "HKDF encryption before DB write" + pattern: "encrypt_api_key" + - from: "backend/api/admin.py GET /api/admin/ai-config/test-connection" + to: "backend/services/ai_config.py::load_provider_config_by_id" + via: "fetch row by provider_id (ignoring is_active)" + pattern: "load_provider_config_by_id" + - from: "backend/api/admin.py PUT /api/admin/ai-config" + to: "PostgreSQL atomic is_active flip" + via: "single UPDATE SET is_active = (provider_id = $target)" + pattern: "is_active = \\(provider_id =" + - from: "frontend AdminAiConfigTab.vue saveSystemConfig" + to: "PUT /api/admin/ai-config" + via: "fetch wrapper request()" + pattern: "saveAiConfig" + - from: "frontend DocumentCard.vue reanalyze()" + to: "POST /api/documents/{id}/classify" + via: "existing classifyDocument client function" + pattern: "classifyDocument" +--- + + +Surface the new AI provider stack in the UI: an admin "AI Providers" configuration panel that drives the `system_settings` table (read/write/test) and a Document card "Classification failed" badge with a re-analyze button that triggers the Plan 04 re-queue endpoint. + +Purpose: Close D-08 (admin AI Providers panel with test-connection), D-11 frontend half (failed badge + re-analyze button), and D-15 (LMStudio base URL configurable through the panel). Enforce the admin-endpoint invariant that `api_key_enc` is never serialised in any response (Threat T-07-01 mitigation). Add Vitest coverage for the three new frontend API helpers and the reanalyze() handler so the D-11 frontend half satisfies CLAUDE.md's "every new function / component must have at least one test" rule. +Output: Backend admin endpoints (GET / PUT / test-connection), `load_provider_config_by_id` added to backend/services/ai_config.py, frontend API client helpers, AdminAiConfigTab.vue extended with a global System AI Providers section above the existing per-user table, DocumentCard.vue badge + button, Vitest test files for the new client helpers and DocumentCard.reanalyze, and the matching admin endpoint security tests promoted from xfail. Plan ends on a human checkpoint that exercises the panel and badge end-to-end. + + + +@$HOME/.claude/get-shit-done/workflows/execute-plan.md +@$HOME/.claude/get-shit-done/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-CONTEXT.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-RESEARCH.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-PATTERNS.md +@.planning/phases/07-redo-and-optimize-llm-integration/07-04-SUMMARY.md +@backend/api/admin.py +@backend/services/ai_config.py +@backend/ai/__init__.py +@backend/ai/provider_config.py +@backend/services/storage.py +@backend/deps/utils.py +@frontend/src/api/client.js +@frontend/src/components/admin/AdminAiConfigTab.vue +@frontend/src/components/documents/DocumentCard.vue + + + + +From backend/api/admin.py (existing patterns): +- Router prefix is "/api/admin" (or similar — verify); endpoints take Depends(get_db), Depends(get_current_admin) +- `_user_to_dict(user) -> dict` is the whitelist helper pattern (see 07-PATTERNS.md "Admin Endpoint Whitelist Helper") +- `await write_audit_log(session, event_type=..., user_id=..., actor_id=..., resource_id=..., ip_address=get_client_ip(request), metadata_={...})` is used for admin actions +- get_client_ip lives in backend/deps/utils.py — import from there + +From backend/services/ai_config.py (introduced in Plan 01 + Plan 03): +- `encrypt_api_key(master_key: bytes, provider_id: str, api_key: str) -> str` +- `decrypt_api_key(master_key: bytes, provider_id: str, ciphertext: str) -> str` +- `load_provider_config(session) -> ProviderConfig | None` (per-row, active=True) +- For PUT we ALSO need to load by provider_id explicitly (not just is_active) — Task 1 adds `async def load_provider_config_by_id(session, provider_id) -> ProviderConfig | None` to this file + +From backend/ai/__init__.py and backend/ai/provider_config.py (Plan 02): +- `get_provider(config: ProviderConfig) -> AIProvider` +- `PROVIDER_DEFAULTS` dict — the 10 known provider_ids; PUT validates incoming provider_id against this list + +From frontend/src/api/client.js (existing pattern, line ~277): +- `request(path, options)` wrapper; `adminUpdateAiConfig(id, provider, model)` is the existing per-user PATCH pattern +- `classifyDocument(docId)` is the existing client function that POSTs /api/documents/{id}/classify (used by DocumentCard.reanalyze) + +From frontend/src/components/admin/AdminAiConfigTab.vue (existing — per Pitfall 6): +- Existing per-user table powered by `users` ref + `configs` reactive + `saveConfig(userId)` — KEEP UNTOUCHED +- This plan adds a SECOND top section before/above the per-user table + +From frontend/src/components/documents/DocumentCard.vue (existing): +- `doc` prop has status, is_shared fields; existing pattern at lines 31-34 shows the Shared badge + +Frontend Vitest framework (existing project setup): +- Vitest is the unit-test runner (matching the project's vue-test-utils / Vitest convention used elsewhere) +- Run command: `cd frontend && npm run test -- --run` (one-shot, no watch) +- If frontend/tests/ does not yet exist, create it. Configure vite.config.js / vitest.config.js to pick up `tests/**/*.spec.js` — if a config already exists, follow whatever globs are defined; otherwise add `test: { include: ['tests/**/*.spec.js'] }` to the existing vite config. + + + + + + + Task 1: Admin AI-config backend (GET, PUT, test-connection) + audit logging + load_provider_config_by_id + + backend/api/admin.py + backend/services/ai_config.py + backend/ai/__init__.py + backend/ai/provider_config.py + backend/deps/utils.py + backend/db/models.py + backend/tests/test_admin_ai_config.py + + + - `GET /api/admin/ai-config` returns 200 with body `{"providers": [...]}` where each entry is the whitelisted dict (provider_id, base_url, model_name, context_chars, is_active, has_api_key (bool), updated_at) + - `api_key_enc` and `api_key` are never present in any response body + - `PUT /api/admin/ai-config` accepts body `{provider_id: str, api_key?: str, base_url?: str|null, model?: str, context_chars?: int, is_active?: bool}` validated by a Pydantic model with extra="forbid" + - When body.api_key is a non-empty string → encrypt with encrypt_api_key(master_key=cloud_creds_key, provider_id, api_key) and store in api_key_enc + - When body.api_key is omitted (None) → leave existing api_key_enc untouched (do not overwrite to NULL) + - When body.api_key == "" → set api_key_enc to NULL (explicit clear) + - When body.is_active is True → single UPDATE: `UPDATE system_settings SET is_active = (provider_id = :target_id)` — atomically flips all rows + - If no row exists for provider_id, PUT inserts one (upsert behaviour) with the supplied fields, defaults from PROVIDER_DEFAULTS for omitted fields + - PUT writes an audit log entry: event_type="admin.ai_config_changed", actor_id=admin.id, user_id=None, resource_id=None, metadata_={"provider_id": body.provider_id, "fields_changed": [list of fields the body explicitly set, NEVER api_key value]} + - `GET /api/admin/ai-config/test-connection?provider_id=X` returns `{"ok": bool, "provider_id": str}`; loads the row, calls load_provider_config_by_id, builds via get_provider, awaits health_check(); returns ok=False if any exception or no row found (does not return 5xx for provider failures — surface as ok=False so the UI shows a clear status) + - All three endpoints depend on get_current_admin (admin-only) + - backend/services/ai_config.py gains `load_provider_config_by_id(session, provider_id)` — same shape as load_provider_config but filters by provider_id and does NOT require is_active=True + - Promote test_admin_ai_config.py::test_get_never_returns_key (asserts api_key_enc not in JSON body) and test_put_writes_active_provider (asserts atomic is_active flip) + + + Open backend/api/admin.py. Add imports: `from services.ai_config import encrypt_api_key, decrypt_api_key, load_provider_config, load_provider_config_by_id`, `from ai import get_provider`, `from ai.provider_config import ProviderConfig, PROVIDER_DEFAULTS`, `from db.models import SystemSettings`, `from sqlalchemy import select, update`. (Verify exact import paths against the existing admin.py style.) + + Add new Pydantic request model alongside existing ones: + `class AiConfigUpdate(BaseModel): model_config = ConfigDict(extra="forbid"); provider_id: str; api_key: Optional[str] = None; base_url: Optional[str] = None; model_name: Optional[str] = None; context_chars: Optional[int] = None; is_active: Optional[bool] = None` — and a `@field_validator("provider_id")` that asserts the value is in PROVIDER_DEFAULTS.keys() (rejects unknown providers with 422). + + Add `_ai_config_to_dict(row: SystemSettings) -> dict` whitelist helper (matches `_user_to_dict` pattern from lines 52-68): returns `{"provider_id": row.provider_id, "base_url": row.base_url, "model_name": row.model_name, "context_chars": row.context_chars, "is_active": row.is_active, "has_api_key": row.api_key_enc is not None, "updated_at": row.updated_at.isoformat() if row.updated_at else None}` — explicitly excludes api_key_enc. + + Add GET endpoint: `@router.get("/ai-config") async def get_ai_config(session: AsyncSession = Depends(get_db), _admin: User = Depends(get_current_admin)) -> dict:` — selects all SystemSettings rows, returns `{"providers": [_ai_config_to_dict(r) for r in rows]}`. Also include rows from PROVIDER_DEFAULTS that have no DB record (synthesized as `{"provider_id": key, "base_url": defaults["base_url"], "model_name": defaults["model"], "context_chars": defaults["context_chars"], "is_active": False, "has_api_key": False, "updated_at": None}`) so the admin UI can show all 10 providers even before any have been saved. + + Add PUT endpoint: `@router.put("/ai-config") async def update_ai_config(body: AiConfigUpdate, request: Request, session: AsyncSession = Depends(get_db), _admin: User = Depends(get_current_admin)) -> dict:` — load existing row via select(SystemSettings).where(provider_id == body.provider_id); if absent create new SystemSettings instance with defaults from PROVIDER_DEFAULTS[body.provider_id]; apply provided fields. For api_key: if body.api_key is None leave api_key_enc unchanged; if body.api_key == "" set api_key_enc = None; if body.api_key is a non-empty string set api_key_enc = encrypt_api_key(settings.cloud_creds_key.encode() if isinstance(settings.cloud_creds_key, str) else settings.cloud_creds_key, body.provider_id, body.api_key). session.add(row) if newly created. If body.is_active is True: execute `update(SystemSettings).values(is_active=(SystemSettings.provider_id == body.provider_id))` — atomic flip (no read-then-write). Then await write_audit_log with metadata_={"provider_id": body.provider_id, "fields_changed": [k for k in ("api_key","base_url","model_name","context_chars","is_active") if getattr(body, k, None) is not None]}. Commit. Return `_ai_config_to_dict(row)`. + + Add test-connection endpoint: `@router.get("/ai-config/test-connection") async def test_ai_connection(provider_id: str, session: AsyncSession = Depends(get_db), _admin: User = Depends(get_current_admin)) -> dict:` — load config via load_provider_config_by_id; if None return {"ok": False, "provider_id": provider_id, "reason": "not_configured"}; build provider via get_provider(config); try: await provider.health_check(); return {"ok": True, "provider_id": provider_id}. except Exception as exc: return {"ok": False, "provider_id": provider_id, "reason": "health_check_failed"}. + + Add `async def load_provider_config_by_id(session, provider_id: str) -> ProviderConfig | None` to backend/services/ai_config.py — mirrors load_provider_config but filters by provider_id and ignores is_active (so test-connection works on inactive rows too). (THIS FILE IS LISTED IN files_modified — Task 1 modifies it.) + + Promote backend/tests/test_admin_ai_config.py: test_get_never_returns_key — call GET as admin, assert response status 200, assert "api_key_enc" not in response.text and "api_key" not in response.json()["providers"][0]. test_put_writes_active_provider — PUT with provider_id="openai", api_key="sk-test", is_active=True; assert response 200 and has_api_key=True; PUT another provider with is_active=True; query DB and assert exactly one row has is_active=True. Add a third test (recommended) test_put_admin_only — call PUT as a regular (non-admin) user, assert 403. Remove xfail decorators. + + + cd backend && pytest tests/test_admin_ai_config.py -x -v + + + - Source assertion: backend/api/admin.py contains `_ai_config_to_dict`, `class AiConfigUpdate(BaseModel)`, `@router.get("/ai-config")`, `@router.put("/ai-config")`, `@router.get("/ai-config/test-connection")` + - Source assertion: backend/api/admin.py PUT body uses `update(SystemSettings).values(is_active=(SystemSettings.provider_id == body.provider_id))` (atomic flip) + - Source assertion: `_ai_config_to_dict` body contains no reference to `api_key_enc` (no leak) + - Source assertion: backend/services/ai_config.py contains `async def load_provider_config_by_id` + - Behavior: pytest backend/tests/test_admin_ai_config.py exits 0 (all promoted tests pass) + - Behavior: GET /api/admin/ai-config response body never contains the string "api_key_enc" or "api_key" value (assertion in test_get_never_returns_key) + - Behavior: After two PUTs with is_active=True on different provider_ids, SELECT COUNT(*) WHERE is_active = TRUE returns 1 (asserted in test_put_writes_active_provider) + - Behavior: PUT by non-admin user returns 403 (admin-only enforcement) + + Three admin AI-config endpoints land; atomic is_active flip enforced; api_key_enc never serialised; audit log records changes without leaking secrets; load_provider_config_by_id added to backend/services/ai_config.py; three Wave-5 admin tests promoted. + + + + Task 2: Frontend API client helpers + AdminAiConfigTab.vue global System AI Providers section + Vitest coverage + + frontend/src/api/client.js + frontend/src/components/admin/AdminAiConfigTab.vue + frontend/src/components/admin/AdminUsersTab.vue + frontend/src/components/admin/AdminQuotasTab.vue + frontend/vite.config.js + frontend/package.json + + + - frontend/src/api/client.js exports getAiConfig(), saveAiConfig(body), testAiConnection(providerId) + - getAiConfig calls request("/api/admin/ai-config", { method: "GET" }) + - saveAiConfig(body) calls request("/api/admin/ai-config", { method: "PUT", headers: {"Content-Type": "application/json"}, body: JSON.stringify(body) }) + - testAiConnection(providerId) calls request(`/api/admin/ai-config/test-connection?provider_id=${encodeURIComponent(providerId)}`, { method: "GET" }) + - AdminAiConfigTab.vue gains a new top-of-template section titled "System AI Providers" placed ABOVE the existing per-user table; existing template, script, and saveConfig logic for per-user assignments is unchanged + - New section: active-provider selector (radio or styled buttons over the 10 PROVIDER_DEFAULTS keys), per-provider accordion (open one at a time) with masked API key field (input type="password", placeholder shows "(unchanged)" when has_api_key is true, otherwise "(not set)"), base URL text input, model name text input, context_chars number input, Save button per provider, Test Connection button per provider + - On mount: call getAiConfig() and populate a reactive systemProviders array; if API call fails, show a non-blocking error banner above the new section only + - Saving a provider: build body containing only the fields the admin actually modified (api_key only if user typed something), call saveAiConfig, refresh systemProviders from response + - Test Connection: call testAiConnection(providerId), show inline "OK" badge (green) or "Failed" badge (red) for 3 seconds + - Per-user assignment block (existing) remains untouched (Pitfall 6 mitigation) + - frontend/tests/api.spec.js Vitest test asserts getAiConfig/saveAiConfig/testAiConnection construct the correct fetch/request payloads (path, method, body, JSON serialization, encodeURIComponent for provider_id) + - `npm run test -- --run` exits 0 with the new api.spec.js tests passing + + + Edit frontend/src/api/client.js: append at the bottom (or near other admin endpoints around line 277): `export function getAiConfig() { return request('/api/admin/ai-config', { method: 'GET' }) }`; `export function saveAiConfig(body) { return request('/api/admin/ai-config', { method: 'PUT', headers: {'Content-Type': 'application/json'}, body: JSON.stringify(body) }) }`; `export function testAiConnection(providerId) { return request('/api/admin/ai-config/test-connection?provider_id=' + encodeURIComponent(providerId), { method: 'GET' }) }`. + + Edit frontend/src/components/admin/AdminAiConfigTab.vue. Do NOT alter the existing per-user table block (Pitfall 6). Add a new
at the top of the