Deal/docs/superpowers/reviews/2026-09-08-code-quality-review.md

Ревью качества кода «Дейл» (2026-09-08)

Исторический документ этапа 8 (ревью, 2026-09-08). Актуальное состояние — docs/superpowers/STATUS.md и docs/technical/Техническая-документация-Дейл.md.

Многоосевое ревью (корректность/читаемость/архитектура/безопасность/производительность) бэкенда и фронтенда. Проводилось 5 ревьюерами по непересекающимся зонам (чтение; правок не вносилось), ключевые находки перепроверены по коду. Проект НЕ git. Метки: [Critical]/[Required]/[Nit]/[Optional] (Required = исправить до прода; Nit = желательно; Optional = задел).

Сводка

Зона	Объём	Critical	Required	Nit	Optional
Frontend (Vue3, JS)	25 файлов / 11.3k LOC	0	6	6	1
Core-каркас (Api/Infrastructure/Contracts)	~370 файлов	0	9	7	5
Модули Kanban/Pipeline/Projects	~140 файлов	0	10	5	2
Модули Settings/Telegram/Tenants/Discovery	~130 файлов	0	7	8	3
gRPC-сервисы (telegram/ai/ml) + proto	~135 файлов	0	8	8	4
Итого	~1100 файлов	0	40	34	15

Общий вердикт: код высокого качества — чистая port&adapter-архитектура, 1 тип=1 файл, тенант- изоляция через схему на тенанта спроектирована сильно, SQL параметризован, XSS/секреты на фронте и в сервисах чистые. Найдено 0 критических дыр класса «ключ наружу/доступ к чужому тенанту». Ниже — что требует исправления и что стоит улучшить. Подробности по зонам — в рабочем журнале сессии (5 отчётов субагентов с file:line); здесь — консолидированный список.

---

A. Безопасность (приоритет 1)

1. [Required] SSRF через baseUrl ИИ-провайдера. Deal.Infrastructure/Integrations/AiConnectionChecker.cs (проверка ok:false/true) + PATCH настроек разрешает тенанту задать произвольный baseUrl (в т.ч. http://127.0.0.1:... — подтверждено acceptance-логом task-6). На не-local провайдере ключ API уходит на указанный адрес → аутентифицированный тенант мультитенантного SaaS получает blind-сканер внутренней сети/метаданных. Исправить: резолв DNS + запрет private/link-local/loopback при проверке и вызове (или egress-фильтр); не принимать переопределение хоста для каталоговых провайдеров.
2. [Required] Rate-limit и анти-брутфорс выключены по умолчанию. Deal.Api/Program.cs (регистрация лимитера), RateLimitOptions дефолт Enabled=false → без env в проде нет ни лимитов, ни LoginAttemptGuard. compose.prod форсирует true, но дефолт кода опасен при запуске вне compose. Исправить: стартовая проверка «Production ⇒ RateLimit:Enabled задан явно» (fail-closed).
3. [Required] CORS fail-open при пустом allowlist. Program.cs (AddCors): пустой Security:AllowedOrigins = любой origin + AllowCredentials (задумано для dev). Исправить: в Production пустой список = отказ на старте; «any origin» только в Development.
4. [Required] Код инвайта пишется в audit_log сырым. JoinEndpoint.cs — capability-токен в вечном аудите операторов. Исправить: не логировать код (или его SHA-256).
5. [Required] Пароль: минимум 4 символа. AuthEndpoints.cs, JoinEndpoint.cs. Для публичного SaaS — минимум 8–10 + проверка на границе; единая константа.
6. [Required] Политика «ключ не перезаписывается маской» не реализована. SettingsService.cs (aiConfigs и tgKeys): PATCH со значением-маской (например sk-1…90ab, ≥8 симв., без enc:) зашифрует маску и безвозвратно потеряет ключ. Комментарий «пустой/маска → не меняется» не подкреплён кодом. Исправить: не шифровать значение, содержащее … (U+2026) либо пустое; тест на roundtrip.
7. [Required] DDL прикладной ролью на старте и из tenant-ручки. TenantProvisioningService.cs, FtsMaintenance.cs — CREATE SCHEMA/Migrate/INDEX на каждом старте и /api/admin/fts/rebuild. В проде это нарушение least privilege. Исправить: отдельные креды мигратора и runtime; fts-rebuild — операторской ручкой.
8. [Required] TenantId без инварианта формата. Deal.SharedKernel/Tenants/TenantId.cs — значение идёт в Search Path строки подключения и в DDL; new TenantId(внешняя_строка) = connection-string-инъекция. Сейчас все потоки дают Guid, но тип не защищён. Исправить: конструктор от Guid / валидация 32 hex.
9. [Required] gRPC-сервисы: нет серверных лимитов на входные данные. AiServiceImpl, MlServiceImpl, TelegramServiceImpl — контракты фиксируют лимиты («ядро обрежет»), но сервис их не enforcement: платные LLM-вызовы на мегабайтных промптах, гигантские SQLite-транзакции. Исправить: INVALID_ARGUMENT на границе + MaxReceiveMessageSize.
10. [Required] mTLS по умолчанию выключен — тихая деградация до plaintext. MtlsOptions.cs — отсутствие/опечатка env молча даёт plaintext+только service-token. Исправить: fail-closed для Production (или warn-on-startup) как для session-ключа.
11. [Required] Инвайт: не проверяется существование/статус тенанта. JoinService.cs — активация по «битому» инвайту даёт FK-500 или пользователя на несуществующем тенанте.
12. [Required] AddUsageAsync не атомарно. ITenantLimitStore.cs — read-modify-write теряет списания при параллельных ИИ-вызовах. Исправить: UPDATE ... SET Used=Used+@n.
13. [Required] Echo-маска: секрет ≤8 символов отдаётся как есть. SettingsService.Mask — маскировать всегда (кроме пустого).

B. Корректность / потеря данных (приоритет 2)

14. [Required] Потеря данных при параллельных мутациях JSON-массивов проектной карточки. ProjectsService.cs (add_comment/add_link/remove_link), ProjectFilesService.cs: комментарии/ссылки/ файлы дописываются «read → PATCH полной заменой массива» без версии/транзакции; double-click теряет запись. Исправить: append одним SQL (jsonb ||/array_append) или optimistic concurrency по updated_at.
15. [Required] Коллизия objectKey файла. ProjectFilesService.cs — «проект/карточка/мс_имя»: две загрузки в одну мс = перезапись объекта. Исправить: случайный суффикс / id записи в ключе.
16. [Required] Дедуп-pump не атомарен. PipelineWorkerService.cs — Exists→Claim→create без проверки результата claim — два конкурентных прохода создадут две карточки. Исправить: повторный Exists/ проверка результата Claim перед созданием.
17. [Required] Move из trash/archive на доску минует снятие спам-сигнала. CardsService.cs — валидируется только цель; «spam +1» не снимается (unlearn только в restore). Исправить: запрет исхода из archive/trash/taken в MoveLeadAsync (или симметричный unlearn).
18. [Required] Параллельные пустые catch { } в модулях Telegram/Discovery — сбои зеркала/превью/ backfill невидимы (ILogger в модулях не используется). Исправить: логировать.
19. [Required] ChangePassword (фронт) шлёт захардкоженный oldPassword='admin'. store.js, SettingsView.vue — после смены пароля повторная смена невозможна, и пароль живёт в реактивном state. Исправить: поле «текущий пароль», не хранить пароль в store.
20. [Required] boot() роняет всё приложение одним сбоем (фронт). store.js: параллельные get без .catch — падение /api/rates (например) = toast «Сервер недоступен» + разлогин. Исправить: необязательные секции в индивидуальные .catch; разлогин только при 401.
21. [Required] applySettings затирает несохранённые промпты (фронт). store.js — автосейв тумблера применяет полный ответ и перезаписывает textarea промптов. Исправить: применять только запатченные ключи.
22. [Required] Гонки устаревших ответов поиска (фронт). store.js — старый ответ может перетереть свежий/очищенный. Исправить: seq-токен/AbortController.
23. [Required] DeleteExpiredSessionsAsync на каждое разрешение сессии. AuthService.cs, OperatorAuthService.cs — глобальный DELETE по public-таблицам в hot-path каждого запроса. Исправить: фоновый цикл или «с вероятностью N%»/логин.
24. [Required] ServiceTokenInterceptor проверяет токен только для unary RPC — первый же server-streaming RPC пройдёт без проверки; то же в access-логе. Исправить: все 4 handler'а.
25. [Required] gRPC-логгер не логирует «прочие» исключения (только OCE/RpcException) — 500-эквивалент уходит мимо лога. Исправить: catch (Exception) → log + RpcException.
26. [Required] Heartbeat/reconnect без таймаута — зависший ConnectAsync последовательно блокирует все тенанты и shutdown. Исправить: CancelAfter на попытку.
27. [Required] QR: отмена RPC до первого URL не отменяет фоновую задачу — «скрытая» авторизация. Исправить: отменять саму задачу при отмене ожидания.
28. [Required] TelegramBackfill fire-and-forget Task.Run из tenant-запроса без in-flight guard (параллельные полные перечитывания); фоновые задачи не отслеживаются хостом. Исправить: гейт операции
    • токен остановки хоста.
29. [Required] int.Parse(apiId) из пользовательской KV-настройки TelegramEndpoints.cs — FormatException маскируется под 400 «не подключён». Исправить: TryParse + понятная ошибка.

C. Архитектура / дублирование (приоритет 3)

30. [Required] 9 независимых реализаций чтения настроек (GetAsync+JsonDocument.Parse+дефолт) в Settings/IncomingRules/RatesService/Discovery*/DialogsService — расхождение семантики уже видно. + ~8 копий KV-хелперов (ReadBool/ReadInt/ReadString/ReadStringList) и 3 копии LoadRatesAsync в Kanban/Pipeline/Projects. Исправить: один публичный снапшот настроек в Settings или SharedKernel + общий RatesCacheReader.
31. [Required] Обвязка gRPC-сервисов (ServiceTokenInterceptor/RpcCallLogging/MtlsOptions/MtlsCertificates/ Logging + Host) скопирована в 3 независимых sln. Исправить: общий проект Deal.Grpc.Hosting.
32. [Required] Большие файлы: PipelineWorkerService (914), KanbanStore (726), DiscoveryStore (632), ProjectsService (576), ProjectsEndpoints (568), CardsService (475), Program.cs (695), LocalFieldsParser (438), GrpcTelegramClient (447), TelegramIngressService (409); фронт: SettingsView.vue (1779), DiscoveryView.vue (1243), store.js (2434). Исправить: декомпозиция (см. ниже).
33. [Required] Фронт: MoveMenu вешает document-слушатель на каждую карточку (сотни карточек → сотни слушателей). Исправить: один глобальный обработчик + id открытого меню в store.
34. [Required] Фронт: квадратичные пересчёты колонок. store.js — filter+sort на каждую колонку/ счётчик при каждом ре-рендере. Исправить: один computed Map<colId, sorted[]>.
35. [Nit] Дублирование доменных констант между модулями (EmptyCommentDetail/JustNowLabel/MlSpamLabel/ DefaultChannelHue/PlannedStage-литералы) и расхождение предиката «активные правила» (Kanban vs AiClassifyContextBuilder) — вынести в единые реестры.
36. [Nit] Middleware сессий (Session vs OperatorSession) и токен-генераторы (SessionTokens/ InviteCodeGenerator/TenantAdminService) дублируются — обобщить.
37. [Nit] Легаси-ссылки на строки Python-прототипа в XML-doc (L177–191 и т.п.) — устаревают; оставить «зачем/инвариант», убрать номера строк.
38. [Nit] Форматтеры времени и «знание» о контактах/типах файлов в 3–4 местах (фронт) — единый модуль форматов и словари меток.
39. [Nit] window.prompt в renameBoard на фоне единого ConfirmDialog; дубликаты 86400000; ширины колонок sm/md/lg в 3 местах — константы/единый RenameDialog.

D. Мёртвый код (кандидаты на удаление)

• Фронт: utils.js fileTypeInfo/EXT_KINDS/KIND_LABELS (не импортируется); store.js — curName/fmtMoney вне store, moveLead-мёртвая ветка, trashLead-пустой if, openDialog (не используется), checkReminders (нигде не вызывается); опция «mock»-курсов — проверить, жив ли режим на бэкенде.
• Бэкенд: Kanban DemoLeadFactory недостижимый fallback PrimaryContact; DiscoverySearchErrorCounter — singleton-счётчик без TTL/эвикции и с межтенантным ключом (переделать per-tenant или чистить).

E. Что соответствует хорошим практикам (подтверждено)

• Тенант-изоляция сильная: схема на тенанта через Search Path, TenantDbContext запрещён вне tenant-запроса (fail-fast), AsyncLocal сбрасывается в finally, gRPC-ингресс берёт tenant-id только из metadata, SSE per-tenant.
• SQL параметризован везде (FromSqlInterpolated/ExecuteSqlInterpolated); массовые операции — ExecuteUpdate/Delete; комментарии-батчи без N+1; AsNoTracking.
• Секреты не покидают систему: ключи шифруются (enc:+nonce‖ct‖tag), наружу маски; токены сессий — SHA-256 хэши; пароли Argon2id; куки httpOnly+SameSite=Lax; fail-closed service-token (с явным гардом «пусто≠пусто»); path traversal защищён (SessionStore/ModelPool валидируют tenant-id как имя файла).
• Фронт: XSS-аудит чистый (v-html только через экранирующий renderSourceMessage со схемами http/tg), токенов в localStorage нет (httpOnly-кука), все target=_blank с rel=noreferrer.
• Чистая архитектура port&adapter в модулях (нет EF/HTTP в Application), DTO-рекорды, DI-Registrar'ы, направленные зависимости без циклов, константы-каталоги вместо магических строк.

F. Рекомендуемый порядок исправлений

1. Безопасность (A1–A13) — до любого прода. Точечные правки + тесты.
2. Потеря данных/корректность (B14–B29) — гонки, дедуп, маски, boot/applySettings фронта.
3. Архитектура (C30–C34) — вынос общего grpc-hosting, снапшот настроек, декомпозиция больших файлов, фронт: leadsByCol-компьютед и глобальный слушатель меню.
4. Чистка мёртвого кода (D) + реестры констант (C35–C39) — в рамках рефакторингов, не отдельно.
5. Заделы (Optional) — пагинация колонок, виртуализация списков, LRU для кэшей сессий WTelegram, батчинг провижининга схем, MinIO tenant-префикс, per-request size-лимиты загрузок, single-flight DiscoveryWorker.

---

Статус исправлений (2026-09-08, после ревью)

Выполнено в ходе rework-захода (детали — .superpowers/sdd/deal-stage8-quality-rework/progress.md и docs/superpowers/STATUS.md). Тесты: core 1135/1135, telegram 118/118, ai 52/52, ml 38/38, фронт npm run build OK.

A. Безопасность — закрыто (A1–A13):

• A1 SSRF: SettingsService — baseUrl каталоговых облачных провайдеров не переопределяется (только local/custom); AiConnectionChecker — запрет private/loopback/link-local адресов (в т.ч. 169.254.169.254).
• A2/A3: fail-closed в Production (RateLimit:Enabled обязателен, CORS-allowlist непустой, conn-string без фолбэка) — стартовые проверки Program.cs.
• A4: код инвайта в аудите → SHA-256 codeHash (3 события, тесты обновлены).
• A5: пароль минимум 8 (единый AuthService.MinNewPasswordLength).
• A6: PATCH с маской ключа («…») больше не шифрует маску (терялся бы ключ); A13: короткие секреты маскируются всегда (MaskSecret), apiId остаётся как есть (не секрет).
• A7: DDL (провижининг схем/миграции) — опциональная мигратор-строка ConnectionStrings:DealMigrator (ConnectionStringProvider.ForSchemaDdl); dev/тесты — прежнее поведение.
• A8: TenantId — инвариант 32 hex (Guid N), фабрика FromGuid.
• A9: gRPC-сервисы — лимиты входных данных (INVALID_ARGUMENT) + MaxReceiveMessageSize=4MiB.
• A10: mTLS fail-closed в Production (сервисы).
• A11: JoinService — целевой тенант обязан существовать и быть активным до резервирования кода.
• A12: атомарный инкремент токенов (UPDATE ... UsedTokens=UsedTokens+@n) для Npgsql; EF-путь для InMemory.
• Доп.: int.TryParse apiId; ResolveSession учитывает статус пользователя; очистка протухших сессий — вне hot-path.

B. Корректность/потеря данных — закрыто (B14–B29): атомарные append (comment/link/file) в ProjectStore (1 SQL), objectKey файла с id записи, дедуп-pump атомарен (Claim→bool), move из trash/archive/taken запрещён, пустые catch логируются (DiscLog/ILogger), фронт: смена пароля (oldPass), boot с .catch, applySettings не затирает промпты, seq-токены поиска; интерцепторы gRPC на все 4 вида RPC, логгер catch(Exception), reconnect с таймаутом, QR-cancel; backfill с in-flight guard + lifetime-токеном.

C. Архитектура — закрыто: C30 (единый TenantSettingsSnapshot вместо ~9 копий чтения настроек и 3 копий LoadRatesAsync; удалён клон RateTable.cs), C31 (общий src/grpc-hosting/Deal.Grpc.Hosting; 15 файлов дублей удалены), C32-декомпозиция (KanbanStore→5, PipelineWorkerService→8, DiscoveryStore→5, ProjectsService→4, CardsService→3, SettingsService→6, DiscoveryWorkerService→6 partial; фронт: store.js→слайсы store/, вынесены Telegram/Stop/Scope-вкладки SettingsView, DiscoveryCandidateCard), C33/C34 (MoveMenu, leadsByCol), C36 (UrlSafeToken). Закрыто после ревью (2026-09-09): C35 — общие реестры MlLearningLabels/SourceDefaults в Deal.Contracts (метки обучения ML «spam»/«t:hire»/«t:order» и дефолтный цвет источника «#666») вместо дублей MlSpamLabel/DefaultChannelHue/DefaultDialogHue/SpamLabel в Pipeline/Discovery/Telegram/Infrastructure; единый предикат «активные правила» — AiClassifyContextBuilder переведён на ColumnRules.HasActiveRules (Kanban; было расхождение Count>0 vs терм после trim); реестр ProjectStages (9 id-констант вместо литералов) и общий CardsService.JustNowLabel (Projects/адаптер KanbanStore); DiscoverySearchErrorCounter — TTL-эвикция (см. D). Задел: полный вынос остальных вкладок SettingsView (риск без e2e).

D. Мёртвый код: удалён (фронт: fileTypeInfo/EXT_KINDS/curName/fmtMoney/openDialog/checkReminders и др.; бэкенд: недостижимый PrimaryContact DemoLeadFactory и др.). DiscoverySearchErrorCounter — добавлена TTL-эвикция записей (EntryTtlSeconds=1 ч, ленивая при Next/Reset, часы инъекцией; +4 теста) — задел D закрыт.