diff --git a/CHANGELOG.md b/CHANGELOG.md index fa62d63..fc03267 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version ### Added +- docs: `docs/code-review.md` — full codebase review (architecture, code quality, + documentation, GUI, legacy, risks) with prioritized implementation plan and + model routing; cross-links in `implementation-plan.md` and `progress.md`. - docs (D6): Docker `HEALTHCHECK` probes `/healthz`; endpoint returns 503 unless opendkim, panel, and postfix are RUNNING (`internal/health.Liveness`). - docs (D6): README *Container health* — scope of `/healthz` vs authenticated Status. diff --git a/docs/code-review.md b/docs/code-review.md new file mode 100644 index 0000000..9f3e9b7 --- /dev/null +++ b/docs/code-review.md @@ -0,0 +1,415 @@ +# Рецензирование кодовой базы SelfPost + +**Дата:** 2026-08-05 +**Объём ревью:** ~172 файла, 64 Go-исходника (~10 770 строк в `internal/` + `cmd/`), 29 unit-тестов, e2e-модуль `test/e2e/`, 17 HTML-шаблонов, 15 doc-файлов. + +Связанные документы: [architecture.md](architecture.md), [security.md](security.md), [implementation-plan.md](implementation-plan.md), [progress.md](progress.md), [roadmap.md](roadmap.md). + +--- + +## Общая оценка + +| Критерий | Оценка | Комментарий | +|----------|--------|-------------| +| Архитектура | **Отлично** | Чёткое слоение, минимум связности | +| Сложность vs масштаб | **Отлично** | Без over-engineering | +| Качество кода | **Отлично** | Идиоматичный Go, продуманные rollback-пути | +| Документация | **Хорошо** | As-built docs точны; есть RU/EN split и stale comments | +| Поддерживаемость | **Хорошо** | Высокий порог входа из-за phase-комментариев | +| Логические ошибки | **Минимально** | Критичных багов не найдено; есть принятые операционные gap'ы | +| Legacy | **Низкий** | Только архив spec + phase-комментарии | +| GUI | **Хорошо** | Нет «костылей»; осознанные CSP/layout компромиссы | + +**Вывод:** проект готов к релизному тегу после закрытия § D ([implementation-plan.md](implementation-plan.md)) — предрелизного security review. Остальное — polish, не блокеры. + +--- + +## 1. Архитектура и структура проекта + +### Текущая структура + +```mermaid +flowchart TB + subgraph cmd [cmd] + panel["panel (HTTP + milter + logtail)"] + backup["selfpost-backup CLI"] + end + subgraph web [internal/web — 3030 LOC] + handlers["handlers_*.go"] + templates["templates/*.html"] + security["security.go, session.go"] + end + subgraph services [Services] + domainSvc["internal/domain"] + appSvc["internal/app"] + end + subgraph persistence [Persistence] + store["internal/store (SQLite)"] + end + subgraph adapters [Adapters] + postfix["internal/postfix"] + milterPkg["internal/milter"] + logtail["internal/logtail"] + dnscheck["internal/dnscheck"] + backupPkg["internal/backup"] + health["internal/health"] + end + panel --> web + panel --> milterPkg + panel --> logtail + web --> domainSvc + web --> appSvc + domainSvc --> store + appSvc --> store + milterPkg --> store + logtail --> store + domainSvc --> postfix + appSvc --> postfix +``` + +### Сильные стороны + +- **Layered / ports-and-adapters:** handlers → services (`domain`, `app`) → `store`; инфраструктура изолирована в адаптерах ([`internal/web/web.go`](../internal/web/web.go), [`internal/app/service.go`](../internal/app/service.go)). +- **Composition root** в [`cmd/panel/main.go`](../cmd/panel/main.go): три роли (HTTP, journal-milter, log-tailer) в одном процессе — оправдано для single-container deployment. +- **Interface seams** для тестов: `milter.Store`, `app.SenderMaps`, `logtail.StatusStore`. +- **Embedded migrations** ([`internal/store/store.go`](../internal/store/store.go)) — простой, надёжный подход для 2 миграций. +- **E2E как отдельный модуль** (`test/e2e/go.mod`) — не загрязняет основной модуль. + +### Замечания (не блокеры) + +- **`internal/web/` — 47 файлов, 3030 строк** — самый крупный пакет. При росте v2.x (роли, inbound relay) стоит выделить подпакеты (`web/handlers`, `web/auth`) или split по доменам. Сейчас — приемлемо. +- **Один SQLite connection** (`MaxOpenConns(1)`) — сознательный trade-off; при росте нагрузки milter + HTTP + logtail будут сериализованы. Документировано, для single-admin panel — норма. + +### Рекомендации + +| # | Действие | Приоритет | +|---|----------|-----------| +| A1 | Оставить текущую структуру; рефакторинг пакетов — только при старте 2.x | Низкий | +| A2 | Добавить в [architecture.md](architecture.md) диаграмму слоёв (как выше) | Низкий | + +**Модель:** Sonnet (документация) + +--- + +## 2. Соответствие сложности масштабу проекта + +### Факты + +- Outbound SMTP relay + admin panel для одного оператора. +- ~10.7K строк Go, 3 зависимости (`go-milter`, `x/crypto`, `modernc.org/sqlite`). +- Нет ORM, нет SPA, нет message queue — всё уместно. + +### Сильные стороны + +- Нет лишних абстракций (нет generic repository, нет DI-фреймворка). +- Сервисный слой тонкий, но достаточный: координация SQLite + sasldb2 + Postfix maps ([`internal/app/service.go`](../internal/app/service.go)). +- Fail-open/fail-closed решения явно задокументированы (OpenDKIM tempfail vs journal fail-open). + +### Замечания + +- **Rate limiting в двух местах** (Postfix anvil L1 + milter L2) — сложность оправдана спецификацией, но требует понимания оператором. +- **Backup с manifest version gate** ([`internal/backup/backup.go`](../internal/backup/backup.go)) — чуть тяжелее минимума, но оправдано для migration safety. + +**Вердикт:** сложность **адекватна** масштабу. Over-engineering не обнаружен. + +--- + +## 3. Качество написанного кода + +### Сильные стороны + +- **Комментарии объясняют «почему»**, не «что» — образцовый уровень ([`internal/web/security.go`](../internal/web/security.go), [`internal/milter/milter.go`](../internal/milter/milter.go)). +- **Rollback-паттерны** при partial failure ([`internal/app/service.go`](../internal/app/service.go) `rollbackCreate`). +- **Ordering guarantees:** SQLite row before SASL write — защита от race и password clobber. +- **Validation centralized:** [`internal/web/validate.go`](../internal/web/validate.go), [`internal/app/validate.go`](../internal/app/validate.go). +- **Atomic file writes** для конфигов ([`internal/postfix/write.go`](../internal/postfix/write.go), [`internal/domain/dkim.go`](../internal/domain/dkim.go)). +- **Test coverage ~45%** file ratio (29 test / 64 source files); ключевые пути покрыты (auth, sessions, milter, backup, dnscheck). + +### Замечания + +| Файл | Замечание | Severity | +|------|-----------|----------| +| [`internal/web/token.go`](../internal/web/token.go) | `panic` при сбое `crypto/rand` — осознанно, документировано | Info | +| [`internal/web/handlers_domains.go`](../internal/web/handlers_domains.go) | Stale comment: «Applications and send log arrive in later phases» — уже реализовано | Low | +| Phase/spec references | ~50+ файлов с «Phase N», «spec 7.x» — шум для новых контрибьюторов | Low | +| [`cmd/panel/main.go`](../cmd/panel/main.go) | Package comment всё ещё упоминает «Phase 1 stubs» | Low | + +### Потенциальные улучшения качества + +- Единый проход **gofmt + удаление stale phase-комментариев** (механическая работа). +- Добавить **table-driven test** для edge cases в `parseDelivery` (exotic Postfix status values) — опционально. + +**Модель:** Haiku (механическая чистка комментариев), Sonnet (точечные правки) + +--- + +## 4. Полнота документации и соответствие коду + +### Сильные стороны + +- **[architecture.md](architecture.md)** — as-built source of truth; маршруты, процессы, persistence совпадают с кодом. +- **[security.md](security.md)** — чеклист + принятые риски; каждый риск привязан к коду. +- **[product.md](product.md)** — границы v1.0/out-of-scope чёткие. +- **Regression guard:** [`cmd/panel/envdoc_test.go`](../cmd/panel/envdoc_test.go) — env vars в README = `loadConfig`. +- **CHANGELOG** в формате Keep a Changelog. + +### Расхождения docs ↔ code + +| Проблема | Где | Реальность | +|----------|-----|------------| +| Image tag `0.1.0` vs «v1.0» | [`deploy/docker-compose.yml`](../deploy/docker-compose.yml) vs README | Roadmap § v1.x — bump при теге | +| Quick start URLs | README | GitHub raw; основной repo — Codeberg | +| `docs/logo` | roadmap | Каталог отсутствует (не «пустой») | +| `docs/specification.md` | documentation-plan D9 | Архивирован в `docs/archive/`; ссылки в коде на «spec 7.x» устарели | +| Phase language в коде | 50+ файлов | Docs говорят «v1.0 done», код — «Phase 14» | +| RU/EN split | progress, roadmap, implementation-plan (RU) vs README/architecture (EN) | Намеренно, но барьер для EN-only contributors | + +### Комментирование кода + +- **Высокое качество** в security-critical paths. +- **Среднее** в CRUD handlers (делегируют в services — acceptable). +- **Рекомендация:** заменить «spec 7.x» на ссылки на [product.md](product.md) / [security.md](security.md) § или удалить. + +**Модель:** Sonnet (docs sync) + +--- + +## 5. Читаемость и поддерживаемость + +### Для кого код читаем + +- **Go-разработчик со знанием SMTP/Postfix** — да, без проблем. +- **Новичок без почтового бэкграунда** — потребуется [architecture.md](architecture.md) + README. + +### Факторы, помогающие поддержке + +- Предсказуемая структура handler → service → store. +- Embedded templates (`//go:embed`) — один binary, нет внешних assets. +- Makefile targets: `vet`, `test`, `build`, `e2e`. +- E2E suite покрывает happy path + negatives. + +### Факторы, затрудняющие поддержку + +- Phase-номера в комментариях без контекста. +- Dual cookie names (`__Host-` vs plain) — хорошо документировано, но неочевидно. +- HTMX fragment polling — нужно понимать SSR + partial updates. +- Dev loop: Windows local edit → SSH to Debian server ([progress.md](progress.md)) — не стандартный `go run`. + +### Рекомендации + +| # | Действие | Модель | +|---|----------|--------| +| M1 | Cleanup phase-комментариев → «as-built» language | Haiku | +| M2 | Добавить `CONTRIBUTING.md` (dev loop, model routing, commit protocol) — опционально v1.x | Sonnet | +| M3 | Consolidated doc index в README (ссылки на все docs/) | Sonnet | + +--- + +## 6. Логические ошибки и риски + +### Критичных багов не обнаружено + +E2E покрывает: bootstrap, SMTP AUTH, DKIM, send-log lifecycle, negatives (relay, sender mismatch, L1/L2 limits, milter fail-open, hostname gate, session survive restart). + +### Принятые операционные gap'ы (не баги, но важно знать) + +| Gap | Описание | Документировано | +|-----|----------|-----------------| +| Send-log `queued` forever | Log-tailer стартует с EOF; после restart пропущенный хвост не дочитывается | [security.md](security.md), [roadmap.md](roadmap.md) | +| Session resurrection from backup | Restore старого backup + valid cookie = old session alive | [security.md](security.md) | +| CSRF without tokens | POST без Origin/Sec-Fetch-Site пропускается | [security.md](security.md) | +| Fail-open L2 rate limit | DB error → mail проходит | [`internal/milter/ratelimit.go`](../internal/milter/ratelimit.go) | +| Shallow SPF check | Не следует `include:`/`redirect=` | README, `internal/dnscheck/spf.go` | + +### Потенциальные логические нюансы (низкий приоритет) + +1. **Rate limit race:** `CountMessages` + `InsertQueued` не в одной транзакции — при высокой нагрузке возможен overshoot на 1 сообщение. Для differentiated limits — acceptable. +2. **Domain export с plaintext passwords** ([`internal/domain/transfer.go`](../internal/domain/transfer.go)) — by design, но высокий риск утечки файла. +3. **`macro()` dual lookup** ([`internal/milter/milter.go`](../internal/milter/milter.go)) — workaround для Postfix/go-milter; permanent, not a bug. + +### Рекомендации + +| # | Действие | Приоритет | Модель | +|---|----------|-----------|--------| +| L1 | **Предрелизный security review** (§ D) — обязательный гейт | **P0** | **Fable** | +| L2 | Send-log gap mitigation (persist read offset / reconcile stuck rows) — опционально | P2 | Opus | +| L3 | Transaction wrap для rate limit count+insert — опционально | P3 | Opus | + +--- + +## 7. Legacy-код и миграции + +### SQL-миграции + +- [`0001_init.sql`](../internal/store/migrations/0001_init.sql) — initial schema +- [`0002_sessions.sql`](../internal/store/migrations/0002_sessions.sql) — sessions (plan B.1) +- Механизм: `PRAGMA user_version`, embedded FS, transactional apply — **чистый**, без legacy branches в коде. + +### Архивная документация + +- [`docs/archive/specification-v1.0.md`](archive/specification-v1.0.md) — historical; помечен «не источник истины». +- **Рекомендация:** оставить в archive; в коде заменить «spec 7.x» на актуальные doc-ссылки. + +### Legacy patterns в runtime + +| Элемент | Статус | Действие | +|---------|--------|----------| +| Phase 0–14 comments | Historical noise | Cleanup (Haiku) | +| `macro()` brace workaround | Permanent Postfix compat | Оставить, уже документировано | +| Legacy charset (windows-1251) in milter | Keep raw header on decode fail | Оставить | +| In-memory sessions | **Удалено** (B.1 → SQLite) | Done | +| `copytruncate` log rotation | **Заменено** (B.2 → rename+reload) | Done | + +**Перспектива удаления:** единственный кандидат на cleanup — **phase-комментарии** и **архив spec** (оставить файл, убрать ссылки из кода). + +--- + +## 8. GUI: «костыли» и оптимизация компоновки + +### Стек + +Go `html/template` + HTMX polling + [`panel.css`](../internal/web/static/panel.css) + [`panel.js`](../internal/web/static/panel.js). **Нет React/Vue** — минимальный footprint. + +### Поиск маркеров долга + +**TODO / FIXME / HACK / kostyl — 0 вхождений** по всему репозиторию. + +### Осознанные компромиссы (не костыли) + +| Компромисс | Файл | Обоснование | +|------------|------|-------------| +| External CSS/JS only (no inline) | `panel.css`, `panel.js` | CSP `default-src 'self'` | +| HTMX `includeIndicatorStyles: false` | `layout.html` | Avoid CSP exception | +| Block layout for applications (not table) | `panel.css` | 4 cols + 6 controls don't fit 48rem | +| Two-row nav | `panel.css` | Session block vs page links width | +| Page-specific max-width (48/64/24rem) | `panel.css` | Monitoring vs forms | +| Subject ellipsis via inner `` | `panel.css` | `max-width` on `