Ревью 2026 09 08 code quality review

stepan edited this page 2026-09-13 00:17:00 +03:00

Table of Contents

• Ревью качества кода «Дейл» (2026-09-08)
• Сводка
• A. Безопасность (приоритет 1)
• B. Корректность / потеря данных (приоритет 2)
• C. Архитектура / дублирование (приоритет 3)
• D. Мёртвый код (кандидаты на удаление)
• E. Что соответствует хорошим практикам (подтверждено)
• F. Рекомендуемый порядок исправлений
• Статус исправлений (2026-09-08, после ревью)

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

Перенесено из репозитория (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 закрыт.

Документы

• Home
• ТЗ
• Техническая документация
• Инструкция пользователя
• API-карта
• Код-стайл

Процесс

• Бэклог
• Статус разработки
• Код-стайл аудит

Историческое

• Планы • Ревью • Спеки • Архитектура