From a566a606633bfcd456955c68d32a1b5c87b9645c Mon Sep 17 00:00:00 2001 From: oabrivard Date: Sun, 29 Mar 2026 22:24:08 +0200 Subject: [PATCH] Code improvement after a code review with Codex --- audits/2026-03-27/README.md | 17 ++ audits/2026-03-27/backend-rust.md | 75 +++++++ .../2026-03-27/clarifications-2026-03-28.md | 30 +++ .../consolidated-refactoring-plan.md | 74 +++++++ .../2026-03-27/devils-advocate-priorities.md | 36 +++ audits/2026-03-27/frontend-solidjs.md | 59 +++++ audits/2026-03-27/implementation-plan.md | 137 ++++++++++++ audits/2026-03-27/qa-integration-e2e.md | 151 +++++++++++++ audits/2026-03-27/software-architect.md | 116 ++++++++++ audits/2026-03-27/technical-lead.md | 205 ++++++++++++++++++ 10 files changed, 900 insertions(+) create mode 100644 audits/2026-03-27/README.md create mode 100644 audits/2026-03-27/backend-rust.md create mode 100644 audits/2026-03-27/clarifications-2026-03-28.md create mode 100644 audits/2026-03-27/consolidated-refactoring-plan.md create mode 100644 audits/2026-03-27/devils-advocate-priorities.md create mode 100644 audits/2026-03-27/frontend-solidjs.md create mode 100644 audits/2026-03-27/implementation-plan.md create mode 100644 audits/2026-03-27/qa-integration-e2e.md create mode 100644 audits/2026-03-27/software-architect.md create mode 100644 audits/2026-03-27/technical-lead.md diff --git a/audits/2026-03-27/README.md b/audits/2026-03-27/README.md new file mode 100644 index 0000000..0747f4a --- /dev/null +++ b/audits/2026-03-27/README.md @@ -0,0 +1,17 @@ +# Audit Pack (2026-03-27) + +## Teammate Reports +- [software-architect.md](./software-architect.md) +- [frontend-solidjs.md](./frontend-solidjs.md) +- [backend-rust.md](./backend-rust.md) +- [technical-lead.md](./technical-lead.md) +- [qa-integration-e2e.md](./qa-integration-e2e.md) +- [clarifications-2026-03-28.md](./clarifications-2026-03-28.md) + +## Orchestration Artifacts +- [consolidated-refactoring-plan.md](./consolidated-refactoring-plan.md) +- [devils-advocate-priorities.md](./devils-advocate-priorities.md) +- [implementation-plan.md](./implementation-plan.md) + +## Note +Some source teammate reports were marked as partial by their authors, but all delivered outputs have been captured and saved here in Markdown form. diff --git a/audits/2026-03-27/backend-rust.md b/audits/2026-03-27/backend-rust.md new file mode 100644 index 0000000..e0075db --- /dev/null +++ b/audits/2026-03-27/backend-rust.md @@ -0,0 +1,75 @@ +# Backend Rust Audit Report (Partial) + +## Scope and limitations +- Audited backend docs + code paths in: + - `handlers/`, `services/`, `db/`, `models/`, migrations, representative tests +- Limitation: + - Full backend test execution could not be completed due persistent Cargo artifact lock after interruption. + +## Clarification Questions +1. Is "LLM vs HTML source-link extraction mode" still a product requirement, or intentionally removed? + - Docs still mention configurability (`functional_specs.md:140`) + - Related settings were removed in migrations (`20260326000026_remove_use_llm_for_source_links.sql`, `20260325000018_drop_deprecated_settings.sql`) +2. What are canonical theme defaults: docs/DB (`max_items=4`, `summary_length=3`) or handler (`5`, `2`)? + +## Assumptions +- Docs are source of truth unless explicitly superseded by migration intent. +- Multi-theme source scoping is mandatory. + +## Prioritized Findings + +### P0 +- Theme-scoped source import/export contract is broken. + - Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/sources.rs:111`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/sources.rs:154`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/sources.rs:234`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:120`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/sources.rs:249` + - Spec refs: `/Users/oabrivard/Projects/rust/ai_synth/docs/requirements.md:19`, `/Users/oabrivard/Projects/rust/ai_synth/docs/functional_specs.md:35` + - Direction: make import/export strictly theme-aware. + +### P1 +- Theme update endpoint lacks validation; invalid settings can persist. + - Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/models/theme.rs:77`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/themes.rs:72`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/db/themes.rs:72` + - Direction: add `UpdateThemeRequest::validate()` and enforce in handler. + +- Source create/import path does not verify theme ownership. + - Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/sources.rs:72`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/db/sources.rs:55` + - Direction: enforce `theme_id` ownership check before insert/update. + +- Theme creation contract drifts from documented behavior/defaults. + - Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/models/theme.rs:43`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/models/theme.rs:47`, `/Users/oabrivard/Projects/rust/ai_synth/docs/functional_specs.md:90`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/handlers/themes.rs:56`, `/Users/oabrivard/Projects/rust/ai_synth/docs/functional_specs.md:185` + - Direction: align create validation + defaults with agreed product contract. + +### P2 +- Core synthesis orchestration is monolithic and high-risk to change. + - Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:85`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:152`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:357`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:542` + - Direction: split into composable phase modules. + +- Scheduler reliability path is under-tested. + - Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/scheduler.rs:94` + +- Phase-2 filtering performs per-URL DB checks (N+1 pattern). + - Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:1031` + +### P3 +- Silent error suppression (`.ok()`) hides operational issues. + - Evidence: `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:351`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:457`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/synthesis.rs:604`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/scheduler.rs:67`, `/Users/oabrivard/Projects/rust/ai_synth/backend/src/services/scheduler.rs:85` + +## Idiomatic Rust Assessment +- Error handling: good (`AppError`, `Result`, minimal panics). +- Layering: mostly good, but orchestration service is overloaded. +- Async/concurrency: good use of `JoinSet`, `watch`, `AtomicBool`. +- Testability: strong in many modules; weaker around scheduler/autonomous workflows. + +## Refactoring Plan +1. Correctness first (1 sprint). + - Theme-aware import/export, theme ownership validation, update validation, defaults alignment. +2. Pipeline decomposition (1-2 sprints). + - Split `run_generation_inner` into phase modules and explicit state contexts. +3. Reliability and QA hardening (1 sprint). + - Scheduler integration tests + SSE progress integration tests. +4. Performance cleanup (incremental). + - Batch article-history checks and optimize bulk inserts. + +## Quick Wins +1. Add theme ownership guard in `POST /sources`. +2. Add update validation for themes. +3. Wire `theme_id` through bulk/csv import. +4. Make `export-csv` honor `theme_id`. diff --git a/audits/2026-03-27/clarifications-2026-03-28.md b/audits/2026-03-27/clarifications-2026-03-28.md new file mode 100644 index 0000000..e50d322 --- /dev/null +++ b/audits/2026-03-27/clarifications-2026-03-28.md @@ -0,0 +1,30 @@ +# Clarifications Resolved (2026-03-28) + +This memo records product/contract decisions provided after the initial audit. + +## Final Decisions +1. Theme creation: + - Creating a theme is allowed without sending categories. + - Default categories are `Divers` and `Sans date`. + - Category creation requires an existing theme. +2. Schedule API contract: + - `GET /themes/{id}/schedule` returns `200` with `null` when absent. +3. Canonical theme defaults: + - `max_items_per_category = 4` + - `summary_length = 3` +4. Preferred source scope: + - Preferred sources are managed per theme. +5. Import/export scope: + - Bulk import, CSV import, and CSV export are always bound to the selected theme. +6. Fallback category label: + - Canonical label is `Divers`. +7. Source-link extraction mode: + - Fully deprecated. +8. Frontend UI guideline: + - Using `Button` component instead of raw `