vvzvlad/gitmost
1
0
Fork 0
Code Issues 14 Pull Requests 12 Packages Projects Releases Activity

fix: bug batch — #161 #190 #207 #159 + #206 findings #212

Merged
vvzvlad merged 18 commits from fix/mainline-bugs-2026-06-26 into develop 2026-06-26 17:43:56 +03:00
Conversation 2 Commits 18 Files Changed 35 +2609 -242
Ghost commented 2026-06-26 06:27:08 +03:00
Copy Link

Багфикс-батч (одна ветка, коммит-на-баг). Все проверки зелёные вместе: server+client tsc, editor-ext, server jest 589, + интеграционные тесты на реальном PG.

Закрывает

  • Closes #161 — New chat во время стрима первого хода сбрасывает чат (а не только бейдж роли)
  • Closes #190 — понятная ошибка валидации tool-call вместо сырого zod (роняемый pageId в параллельной партии)
  • Closes #207 — move-TOCTOU цикл A↔B (транзакция + advisory-lock) + depth-guard на рекурсивные CTE
  • Closes #159 — остаток аудита: cost cap для анонимного share-ассистента (токен-бюджет за скользящие сутки). 8/10 уже в #185, move-цикл закрыт коммитом #207.

Частично (#206 — ред-тим отчёт)

Исправлено 4 подтверждённых бага: общие аттачи при дублировании страницы, ретрай транзиентного сбоя автосейва, cycle-guard на out-of-order move (drop поддерева), dedup коллизий unique-id. Остальные findings: footnote — не баг; markdown-export кастомных узлов / empty-guard / history-snapshot — фича/продуктовое/рефактор (отложены, см. отчёт в треде).

Коммиты (9, по багу)

161, 190, 207×2, 206×4, 159×1.

🤖 Generated with Claude Code

Багфикс-батч (одна ветка, коммит-на-баг). Все проверки зелёные вместе: server+client tsc, editor-ext, server jest 589, + интеграционные тесты на реальном PG. ## Закрывает - Closes #161 — New chat во время стрима первого хода сбрасывает чат (а не только бейдж роли) - Closes #190 — понятная ошибка валидации tool-call вместо сырого zod (роняемый pageId в параллельной партии) - Closes #207 — move-TOCTOU цикл A↔B (транзакция + advisory-lock) + depth-guard на рекурсивные CTE - Closes #159 — остаток аудита: cost cap для анонимного share-ассистента (токен-бюджет за скользящие сутки). 8/10 уже в #185, move-цикл закрыт коммитом #207. ## Частично (#206 — ред-тим отчёт) Исправлено 4 подтверждённых бага: общие аттачи при дублировании страницы, ретрай транзиентного сбоя автосейва, cycle-guard на out-of-order move (drop поддерева), dedup коллизий unique-id. Остальные findings: footnote — не баг; markdown-export кастомных узлов / empty-guard / history-snapshot — фича/продуктовое/рефактор (отложены, см. отчёт в треде). ## Коммиты (9, по багу) 161, 190, 207×2, 206×4, 159×1. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Ghost added 9 commits 2026-06-26 06:27:08 +03:00
fix(ai-chat): New chat during first-turn stream resets the chat, not just the role badge (#161) 5b59a70e3f 
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fix(ai-chat): clearer tool-call validation error for dropped pageId in parallel batches (#190) ef7d04d1e7 
In-app AI-chat tools used bare zod schemas, so when the model dropped a
required arg (typically pageId) in a parallel/batch tool call, the AI SDK
forwarded zod's raw "expected string, received undefined" text to the model
— not actionable. Add a centralized modelFriendlyInput(shape) wrapper that
keeps the exact JSON Schema contract (required/description/constraints via
z.toJSONSchema draft-7) but replaces the raw zod text with a message naming
each missing/invalid parameter and reminding the model not to drop ids like
pageId in parallel batches. No value is guessed/backfilled (cf. #159).

Applied to every in-app tool: the sharedTool() builder and all inline
inputSchema in ai-chat-tools.service.ts, plus public-share-chat-tools.service.ts.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fix(page): make movePage cycle-check + update atomic to prevent concurrent A<->B cycles (#207) 00ca4ff3d6 
The server-side move cycle guard (getPageBreadCrumbs) and the move UPDATE ran
as two separate, unlocked statements. Two concurrent moves ("A under B" and
"B under A") could each read the same pre-write acyclic snapshot, both pass the
guard, then persist A.parentPageId=B AND B.parentPageId=A — a parent/child
cycle (TOCTOU, #207 #7).

Run the cycle check and the UPDATE inside one transaction (executeTx) guarded
by a per-space advisory lock (pg_advisory_xact_lock, held until COMMIT) so all
moves within a space serialize: the second mover blocks until the first commits
and then sees the freshly written parent, so its guard rejects the cycle.
getPageBreadCrumbs gains an optional trx so the check runs on the locked snapshot.

Adds an integration test driving two opposing concurrent movePage calls and
asserting no cycle ever persists and exactly one move is rejected. Updates the
movePage unit-test stubs for the new transactional path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fix(page): add cycle/depth guard to recursive tree-traversal CTEs (#207) c4807022f2 
getPageBreadCrumbs (ancestor CTE) and forceDelete (descendant CTE) used
withRecursive + unionAll with no CYCLE clause or depth cap. If a parent/child
cycle already exists in the data (e.g. one slipped in via the #7 TOCTOU race),
both queries loop forever — hang / statement timeout. Worse, the move guard
itself runs the ancestor CTE, so a cycle would disable the very guard meant to
prevent it (#207 #8).

Add a depth counter bounded by MAX_PAGE_TREE_DEPTH to both recursive CTEs; the
walk stops at the cap, so a cycle yields a bounded result instead of hanging.
Real page trees are only a few levels deep, so the cap never truncates a
legitimate result. getPageBreadCrumbs selects an explicit column list (not
selectAll) so the internal depth counter never leaks into the breadcrumb shape.

Adds an integration test that seeds an A<->B cycle directly and asserts both
getPageBreadCrumbs and forceDelete return bounded / complete under a short
connection-level statement_timeout instead of hanging.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fix(page): copy shared attachments for every referencing page on duplicate (#206) c919d4f636 
attach-1: when the same attachmentId was referenced by more than one page
in a duplicated subtree, the per-attachmentId map held only a single copy
entry, so the last page processed clobbered the others. The downstream
ownership guard (`attachment.pageId !== oldPageId`) then matched at most one
page and skipped the lone DB row entirely: no blob copied, no new row, every
copy's image 404'd. Key the map to a list of entries and copy one blob/row
per referencing page; drop the now-incorrect ownership guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fix(tree): cycle-guard placeByPosition so out-of-order moves don't drop subtrees (#206) 3d47c306fa 
ui-state-races-1: the server-authoritative move path (placeByPosition, via
applyMoveTreeNode) lacked the isDescendant cycle guard that drag-drop `move`
has. When move events arrive out of order so the destination parent is still
nested inside the moved node's own subtree, remove(source) dropped the whole
subtree (incl. the future parent) and insertByPosition could not re-place it —
the node and all descendants silently vanished with no error/refetch.

Add the isDescendant guard to placeByPosition (returns same ref, like its other
no-op cases) and short-circuit applyMoveTreeNode on the same condition BEFORE
the placed===prev remove-fallback (which would otherwise still drop the
subtree). Leave the tree untouched so a later corrective event / reconnect
reconcile fixes it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fix(collab): retry transient store failures so autosave edits aren't lost (#206) 770ba70541 
persist-1: onStoreDocument wrapped the page write in a try/catch that only
logged and swallowed the error, then resolved "successfully". hocuspocus
destroys/unloads the in-memory Y.Doc right after the hook resolves (the only
copy of the latest edit), so a transient DB error (deadlock, serialization
failure, dropped connection) silently lost the edit. Worse, the post-store
branch ran on the partially-assigned `page`, broadcasting a phantom
"page.updated" and enqueueing a history snapshot for content never written.

Wrap the write in a small bounded retry (3 attempts) so the save is
re-attempted while we still hold the doc, and clear `page` on failure so the
success-only side effects never report a save that didn't happen.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fix(editor-ext): dedupe colliding unique ids on import/normalize (#206) 6bb9dfdc86 
editor-pm-7: addUniqueIdsToDoc only FILLED missing ids and never deduplicated
existing ones, so a copy/paste or bulk-JSON duplicate that kept its attrs.id
produced two nodes sharing an id. MCP addressed edits (patch_node /
delete_node "before/after id") then hit the wrong node or both.

Walk the configured-type nodes in document order: the first occurrence of an
id keeps it (stable anchor), later duplicates are reassigned a fresh id.
transclusionSource ids are cross-reference keys (references resolve a source by
this id), so they are only filled-when-missing, never reassigned, to avoid
orphaning their references.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fix(ai-chat): add per-workspace rolling-day token budget for anonymous share assistant (#159) 1d610b3a62 
The anonymous public-share assistant only capped the COUNT of requests
(100/hour/workspace), not their cost. One accepted turn runs the agent loop
up to stepCountIs(5), re-sending the whole client-held transcript as input on
every step, while maxOutputTokens caps only the output; the request window is
hourly with no daily ceiling, so a steady stream at the cap sustains ~24x its
count per day. Counting requests therefore does not bound the owner's LLM bill
(red-team finding #5).

Add a second cost contour: a cluster-wide, sliding-window per-workspace TOKEN
budget over a rolling day. It is checked read-only BEFORE a turn streams (429,
no request slot consumed, nothing spent) and the turn's real usage
(totalUsage: input re-sent per step + output, summed across all steps) is
recorded once it finishes via streamText onFinish. Fails closed on the check
(deny when Redis can't prove we're under budget); best-effort on the record.
Env-overridable via SHARE_AI_WORKSPACE_TOKEN_BUDGET_PER_DAY (default 1M/day).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vvzvlad added the bug label 2026-06-26 15:49:29 +03:00
vvzvlad commented 2026-06-26 16:42:24 +03:00
Owner
Copy Link

Code review — PR #212: батч багфиксов (#161 new-chat reset, #190 tool-error messages, #207 move-cycle TOCTOU/CTE, #159 share-AI token budget, #206 attachment-dup / id-dedupe / persistence / tree-race)

Вердикт: Request changes. Логика корректна и без security/regression-замечаний, но PR вводит новый operator-facing env-seam и закрывает пять багов без единой правки CHANGELOG.md / .env.example (нарушение задокументированной конвенции проекта), а самая рискованная ветка фикса дедупликации id (NO_REASSIGN для transclusionSource) не покрыта ни одним тестом. Это must-fix перед merge.

Объём: дифф develop…fix/mainline-bugs-2026-06-26 (merge-base 3ddc329b), 27 файлов, +1918/−206. Прогнаны параллельные аспектные ревьюеры (security, stability, conventions, documentation, regressions, test-coverage, simplification, architecture) + judge-проход.

Must fix before merge

  • [documentation] Задокументировать новый env-var SHARE_AI_WORKSPACE_TOKEN_BUDGET_PER_DAY в .env.example — apps/server/src/core/ai-chat/public-share-workspace-limiter.ts:343, .env.example:185-189
    Подтверждено: лимитер читает process.env.SHARE_AI_WORKSPACE_TOKEN_BUDGET_PER_DAY (default 1_000_000, docstring называет это «overridable seam»), но PR не трогает .env.example, где соседние knob'ы SHARE_AI_WORKSPACE_MAX_PER_HOUR (стр. 185) и SHARE_AI_MAX_OUTPUT_TOKENS (стр. 189) задокументированы. Оператор не может найти центральную ручку фикса #159. Fix: добавить в блок anonymous-share-assistant рядом со SHARE_AI_WORKSPACE_MAX_PER_HOUR закомментированную запись # SHARE_AI_WORKSPACE_TOKEN_BUDGET_PER_DAY=1000000 с комментарием — cluster-wide per-workspace rolling-day бюджет токенов (input+output), fail-closed при ошибке Redis, default 1 000 000.

  • [conventions] Добавить записи [Unreleased] в CHANGELOG.md со ссылками на #161 #190 #207 #159 #206 — CHANGELOG.md:11-12
    Подтверждено: PR не трогает CHANGELOG.md, при том что проект ведёт секцию ## [Unreleased] по Keep-a-Changelog с цитированием issue по мере мержа (уже перечислены #163, #146/#147; недавние PR #185/#186 правили этот файл). Пять пользовательских багфиксов и новый operator-facing default остаются недокументированными, и release-cut (AGENTS.md шаг 4) теряет per-issue контекст. Fix: добавить под ## [Unreleased] ### Fixed-буллеты для #161/#190/#207/#206 и отдельный ### Security-буллет для per-workspace rolling-day token budget #159 (default 1M/day, env-overridable), в стиле окружающих записей.

  • [test-coverage] Добавить тест на новую ветку NO_REASSIGN (transclusionSource) в дедупликации id — packages/editor-ext/src/lib/unique-id/unique-id.util.ts:73-90
    Подтверждено: дублирующийся id на transclusionSource намеренно НЕ переназначается (!NO_REASSIGN.has(node.type.name)), а отсутствующий — заполняется. Это самый рискованный путь фикса: transclusionSource — реальный сконфигурированный UniqueID-тип в серверном пути (collaboration.util.ts:72), его id — cross-reference key, и переназначение осиротит ссылки на transclusion (тихая потеря целостности данных). Новый unique-id.util.test.ts конфигурирует только ["heading","paragraph"] и не содержит ни одного упоминания transclusionSource (grep: 0) — регрессия, выкинувшая NO_REASSIGN, прошла бы сьют. Fix: добавить transclusionSource в конфигурируемые типы и кейсы: (a) два узла с общим transclusionSource id ОБА сохраняют его, (b) узел transclusionSource без id получает новый.

Non-blocking

  • [simplification] Упростить двойную защиту totalUsage в onFinish — apps/server/src/core/ai-chat/public-share-chat.service.ts:273-275
    Подтверждено по коду: в AI SDK onFinish всегда получает объект totalUsage (нулевыми могут быть только его поля), поэтому const u = totalUsage ?? ({} as typeof totalUsage) и последующий u?.…-optional-chaining мертвы. Не баг, соответствует defensive-стилю файла. Fix (опц.): убрать промежуточный u и ?-цепочку, оставив field-level ?? 0 (которые реальны): const tokens = totalUsage.totalTokens ?? (totalUsage.inputTokens ?? 0) + (totalUsage.outputTokens ?? 0);.

  • [stability] Учесть, что PAGE_UPDATED в movePage теперь срабатывает до COMMIT — apps/server/src/core/page/services/page.service.ts:959-1001
    movePage теперь оборачивает updatePage в executeTx, а updatePages() эмитит EventName.PAGE_UPDATED синхронно внутри транзакции (page.repo.ts:172); раньше шло на auto-commit this.db. При откате после updatePage, но до commit, слушатель уже поставит job на reindex несостоявшегося move. Влияние мало: cycle-rejection бросает до updatePage (событие не эмитится), reindex-listener перечитывает БД (индексирует корректное un-moved состояние), а PAGE_MOVED-broadcast корректно гейтится на committed numUpdatedRows. Итог — редкий spurious reindex, не неверный broadcast. Fix (опц.): отложить эмит PAGE_UPDATED до резолва executeTx либо эмитить только при numUpdatedRows > 0.

  • [test-coverage] Покрыть fallback извлечения токенов в onFinish (provider без totalTokens) — apps/server/src/core/ai-chat/public-share-chat.service.ts:266-276
    Замыкание считает spend как u?.totalTokens ?? (u?.inputTokens ?? 0) + (u?.outputTokens ?? 0) и шлёт в recordShareTokens, но спека не мокает streamText / не запускает stream(), так что ветка суммы input+output не проверяется. Математика бюджета и проводка recordShareTokens покрыты хорошо — тонкий пробел, не блокер. Fix: юнит-тест, вызывающий форму onFinish напрямую с totalUsage без totalTokens (ассерт суммы) и второй — с totalTokens (ассерт приоритета).

Architecture & design (forward-looking, non-blocking)

1. Дублирование двух per-workspace Redis sliding-window лимитеров (public-share-workspace-limiter.ts: PublicShareWorkspaceLimiter request-count + новый PublicShareWorkspaceTokenBudget). Структуры почти идентичны (sliding-window LOG над per-workspace sorted set, ZREMRANGEBYSCORE-trim, PEXPIRE, уникальный member <…>:<counter>:<random>, fail-closed log-and-deny, parallel resolveShareAi… env-reader + createPublicShare… factory + test-fake). Различия: request-лимитер делает check-AND-ZADD атомарно, бюджет делит read-only CHECK и отдельный RECORD ZADD и кодирует токены в leading integer member'а. Корректно и независимо протестировано сегодня; цена — drift-risk и двойная поддержка (есть реальный второй use-case).

  • Опция A — извлечь общий RedisSlidingWindowLog примитив (trim + member-uniqueness + PEXPIRE + fail-closed) (effort: m). Pros: один владелец оконной семантики/TTL/fail-closed, будущие фиксы — в одном месте, общий fake-redis harness. Cons: Lua-скрипты различаются (atomic consume vs read-only check + record, count vs token-sum), общий core — в основном JS-обвязка, абстракция рискует выйти тонкой; обобщение Lua под оба режима усложнит более простой request-лимитер.
  • Опция B — оставить два класса, вынести только не-Lua дубли (uniqueMember(), resolveEnvPositiveInt(), fail-closed eval-wrapper, test-fake) (effort: s). Pros: убирает явный boilerplate почти без риска абстракции; каждый лимитер хранит свою целевую Lua и читается ясно. Cons: два Lua-скрипта и две оболочки классов остаются параллельными — концептуальное дублирование частично сохраняется.
  • Опция C — оставить как есть (effort: s). Pros: каждый контур самодостаточен и максимально читаем изолированно. Cons: любое изменение общей семантики надо зеркалить в двух файлах и двух fake'ах, где они могут тихо разойтись.
  • Рекомендация ревьюера: Опция B — забирает большую часть выигрыша по поддержке при near-zero риске; не блокирует merge.

2. placeByPosition's === prev как перегруженный sentinel + дублированный cycle-guard в tree-socket-reducers.applyMoveTreeNode (apps/client/src/features/page/tree). Cycle-guard написан дважды: в treeModel.placeByPosition (возврат tree, когда destination parent — потомок source) и в applyMoveTreeNode (if (newParentId && isDescendant(prev, id, newParentId)) return prev;). Причина — placeByPosition сигналит «отказ» возвратом той же ссылки (=== prev), но reducer УЖЕ трактует placed === prev как «destination parent не загружен на этом клиенте» и делает fallback remove(source) — именно та потеря поддерева, которую гард предотвращает. === prev теперь означает два разных исхода, неразличимых для caller'а. Корректно и протестировано; риск — будущий caller повторно введёт subtree-drop баг.

  • Опция A — вернуть discriminated result ({ tree, status: 'placed' | 'noop-parent-missing' | 'noop-cycle' }) (effort: m). Pros: убирает дублированный isDescendant из reducer'а, модель — единый источник «почему move отклонён», remove-fallback срабатывает только для реально незагруженного родителя, снимает comment-coupling. Cons: меняет сигнатуру и все call-site/тесты на === prev; больший blast radius, чем сам баг; чуть больше церемонии на happy-path.
  • Опция B — оставить reference-return, но не удалять при cycle: вынести treeModel.canReparent(tree, sourceId, parentId), который зовут и гард, и reducer (effort: s). Pros: один именованный предикат владеет cycle-правилом, минимум churn сигнатуры. Cons: reducer всё ещё принимает собственное pre-call решение — двусмысленность === prev (parent-missing vs cycle) технически остаётся, дедуплицируется только правило, не sentinel.
  • Опция C — оставить дублированный гард как есть (effort: s). Pros: минимальное изменение, обе копии протестированы. Cons: === prev остаётся перегруженным; будущий caller может вернуть subtree-drop; правило живёт в двух местах, синхронизируемых вручную.
  • Рекомендация ревьюера: без сильного предпочтения — на усмотрение автора; если tree-sync слой будет расти (out-of-order/conflict handling), окупается Опция A; для одноразового случая Опция C защитима. Не блокирует merge.
## Code review — PR #212: батч багфиксов (#161 new-chat reset, #190 tool-error messages, #207 move-cycle TOCTOU/CTE, #159 share-AI token budget, #206 attachment-dup / id-dedupe / persistence / tree-race) **Вердикт: Request changes.** Логика корректна и без security/regression-замечаний, но PR вводит новый operator-facing env-seam и закрывает пять багов без единой правки `CHANGELOG.md` / `.env.example` (нарушение задокументированной конвенции проекта), а самая рискованная ветка фикса дедупликации id (`NO_REASSIGN` для `transclusionSource`) не покрыта ни одним тестом. Это must-fix перед merge. _Объём: дифф `develop`…`fix/mainline-bugs-2026-06-26` (merge-base `3ddc329b`), 27 файлов, +1918/−206. Прогнаны параллельные аспектные ревьюеры (security, stability, conventions, documentation, regressions, test-coverage, simplification, architecture) + judge-проход._ ### Must fix before merge - **[documentation] Задокументировать новый env-var `SHARE_AI_WORKSPACE_TOKEN_BUDGET_PER_DAY` в `.env.example`** — `apps/server/src/core/ai-chat/public-share-workspace-limiter.ts:343`, `.env.example:185-189` Подтверждено: лимитер читает `process.env.SHARE_AI_WORKSPACE_TOKEN_BUDGET_PER_DAY` (default `1_000_000`, docstring называет это «overridable seam»), но PR не трогает `.env.example`, где соседние knob'ы `SHARE_AI_WORKSPACE_MAX_PER_HOUR` (стр. 185) и `SHARE_AI_MAX_OUTPUT_TOKENS` (стр. 189) задокументированы. Оператор не может найти центральную ручку фикса #159. Fix: добавить в блок anonymous-share-assistant рядом со `SHARE_AI_WORKSPACE_MAX_PER_HOUR` закомментированную запись `# SHARE_AI_WORKSPACE_TOKEN_BUDGET_PER_DAY=1000000` с комментарием — cluster-wide per-workspace rolling-day бюджет токенов (input+output), fail-closed при ошибке Redis, default 1 000 000. - **[conventions] Добавить записи `[Unreleased]` в `CHANGELOG.md` со ссылками на #161 #190 #207 #159 #206** — `CHANGELOG.md:11-12` Подтверждено: PR не трогает `CHANGELOG.md`, при том что проект ведёт секцию `## [Unreleased]` по Keep-a-Changelog с цитированием issue по мере мержа (уже перечислены #163, #146/#147; недавние PR #185/#186 правили этот файл). Пять пользовательских багфиксов и новый operator-facing default остаются недокументированными, и release-cut (AGENTS.md шаг 4) теряет per-issue контекст. Fix: добавить под `## [Unreleased]` `### Fixed`-буллеты для #161/#190/#207/#206 и отдельный `### Security`-буллет для per-workspace rolling-day token budget #159 (default 1M/day, env-overridable), в стиле окружающих записей. - **[test-coverage] Добавить тест на новую ветку `NO_REASSIGN` (`transclusionSource`) в дедупликации id** — `packages/editor-ext/src/lib/unique-id/unique-id.util.ts:73-90` Подтверждено: дублирующийся id на `transclusionSource` намеренно НЕ переназначается (`!NO_REASSIGN.has(node.type.name)`), а отсутствующий — заполняется. Это самый рискованный путь фикса: `transclusionSource` — реальный сконфигурированный UniqueID-тип в серверном пути (`collaboration.util.ts:72`), его id — cross-reference key, и переназначение осиротит ссылки на transclusion (тихая потеря целостности данных). Новый `unique-id.util.test.ts` конфигурирует только `["heading","paragraph"]` и не содержит ни одного упоминания `transclusionSource` (grep: 0) — регрессия, выкинувшая `NO_REASSIGN`, прошла бы сьют. Fix: добавить `transclusionSource` в конфигурируемые типы и кейсы: (a) два узла с общим `transclusionSource` id ОБА сохраняют его, (b) узел `transclusionSource` без id получает новый. ### Non-blocking - **[simplification] Упростить двойную защиту `totalUsage` в `onFinish`** — `apps/server/src/core/ai-chat/public-share-chat.service.ts:273-275` Подтверждено по коду: в AI SDK `onFinish` всегда получает объект `totalUsage` (нулевыми могут быть только его поля), поэтому `const u = totalUsage ?? ({} as typeof totalUsage)` и последующий `u?.…`-optional-chaining мертвы. Не баг, соответствует defensive-стилю файла. Fix (опц.): убрать промежуточный `u` и `?`-цепочку, оставив field-level `?? 0` (которые реальны): `const tokens = totalUsage.totalTokens ?? (totalUsage.inputTokens ?? 0) + (totalUsage.outputTokens ?? 0);`. - **[stability] Учесть, что `PAGE_UPDATED` в `movePage` теперь срабатывает до COMMIT** — `apps/server/src/core/page/services/page.service.ts:959-1001` `movePage` теперь оборачивает `updatePage` в `executeTx`, а `updatePages()` эмитит `EventName.PAGE_UPDATED` синхронно внутри транзакции (`page.repo.ts:172`); раньше шло на auto-commit `this.db`. При откате после `updatePage`, но до commit, слушатель уже поставит job на reindex несостоявшегося move. Влияние мало: cycle-rejection бросает до `updatePage` (событие не эмитится), reindex-listener перечитывает БД (индексирует корректное un-moved состояние), а `PAGE_MOVED`-broadcast корректно гейтится на committed `numUpdatedRows`. Итог — редкий spurious reindex, не неверный broadcast. Fix (опц.): отложить эмит `PAGE_UPDATED` до резолва `executeTx` либо эмитить только при `numUpdatedRows > 0`. - **[test-coverage] Покрыть fallback извлечения токенов в `onFinish` (provider без `totalTokens`)** — `apps/server/src/core/ai-chat/public-share-chat.service.ts:266-276` Замыкание считает spend как `u?.totalTokens ?? (u?.inputTokens ?? 0) + (u?.outputTokens ?? 0)` и шлёт в `recordShareTokens`, но спека не мокает `streamText` / не запускает stream(), так что ветка суммы input+output не проверяется. Математика бюджета и проводка `recordShareTokens` покрыты хорошо — тонкий пробел, не блокер. Fix: юнит-тест, вызывающий форму `onFinish` напрямую с `totalUsage` без `totalTokens` (ассерт суммы) и второй — с `totalTokens` (ассерт приоритета). ### Architecture & design (forward-looking, non-blocking) **1. Дублирование двух per-workspace Redis sliding-window лимитеров** (`public-share-workspace-limiter.ts`: `PublicShareWorkspaceLimiter` request-count + новый `PublicShareWorkspaceTokenBudget`). Структуры почти идентичны (sliding-window LOG над per-workspace sorted set, ZREMRANGEBYSCORE-trim, PEXPIRE, уникальный member `<…>:<counter>:<random>`, fail-closed log-and-deny, parallel `resolveShareAi…` env-reader + `createPublicShare…` factory + test-fake). Различия: request-лимитер делает check-AND-ZADD атомарно, бюджет делит read-only CHECK и отдельный RECORD ZADD и кодирует токены в leading integer member'а. Корректно и независимо протестировано сегодня; цена — drift-risk и двойная поддержка (есть реальный второй use-case). - **Опция A — извлечь общий `RedisSlidingWindowLog` примитив (trim + member-uniqueness + PEXPIRE + fail-closed)** (effort: m). _Pros:_ один владелец оконной семантики/TTL/fail-closed, будущие фиксы — в одном месте, общий fake-redis harness. _Cons:_ Lua-скрипты различаются (atomic consume vs read-only check + record, count vs token-sum), общий core — в основном JS-обвязка, абстракция рискует выйти тонкой; обобщение Lua под оба режима усложнит более простой request-лимитер. - **Опция B — оставить два класса, вынести только не-Lua дубли** (`uniqueMember()`, `resolveEnvPositiveInt()`, fail-closed eval-wrapper, test-fake) (effort: s). _Pros:_ убирает явный boilerplate почти без риска абстракции; каждый лимитер хранит свою целевую Lua и читается ясно. _Cons:_ два Lua-скрипта и две оболочки классов остаются параллельными — концептуальное дублирование частично сохраняется. - **Опция C — оставить как есть** (effort: s). _Pros:_ каждый контур самодостаточен и максимально читаем изолированно. _Cons:_ любое изменение общей семантики надо зеркалить в двух файлах и двух fake'ах, где они могут тихо разойтись. - _Рекомендация ревьюера:_ Опция B — забирает большую часть выигрыша по поддержке при near-zero риске; не блокирует merge. **2. `placeByPosition`'s `=== prev` как перегруженный sentinel + дублированный cycle-guard в `tree-socket-reducers.applyMoveTreeNode`** (`apps/client/src/features/page/tree`). Cycle-guard написан дважды: в `treeModel.placeByPosition` (возврат `tree`, когда destination parent — потомок source) и в `applyMoveTreeNode` (`if (newParentId && isDescendant(prev, id, newParentId)) return prev;`). Причина — `placeByPosition` сигналит «отказ» возвратом той же ссылки (`=== prev`), но reducer УЖЕ трактует `placed === prev` как «destination parent не загружен на этом клиенте» и делает fallback `remove(source)` — именно та потеря поддерева, которую гард предотвращает. `=== prev` теперь означает два разных исхода, неразличимых для caller'а. Корректно и протестировано; риск — будущий caller повторно введёт subtree-drop баг. - **Опция A — вернуть discriminated result** (`{ tree, status: 'placed' | 'noop-parent-missing' | 'noop-cycle' }`) (effort: m). _Pros:_ убирает дублированный `isDescendant` из reducer'а, модель — единый источник «почему move отклонён», remove-fallback срабатывает только для реально незагруженного родителя, снимает comment-coupling. _Cons:_ меняет сигнатуру и все call-site/тесты на `=== prev`; больший blast radius, чем сам баг; чуть больше церемонии на happy-path. - **Опция B — оставить reference-return, но не удалять при cycle: вынести `treeModel.canReparent(tree, sourceId, parentId)`, который зовут и гард, и reducer** (effort: s). _Pros:_ один именованный предикат владеет cycle-правилом, минимум churn сигнатуры. _Cons:_ reducer всё ещё принимает собственное pre-call решение — двусмысленность `=== prev` (parent-missing vs cycle) технически остаётся, дедуплицируется только правило, не sentinel. - **Опция C — оставить дублированный гард как есть** (effort: s). _Pros:_ минимальное изменение, обе копии протестированы. _Cons:_ `=== prev` остаётся перегруженным; будущий caller может вернуть subtree-drop; правило живёт в двух местах, синхронизируемых вручную. - _Рекомендация ревьюера:_ без сильного предпочтения — на усмотрение автора; если tree-sync слой будет расти (out-of-order/conflict handling), окупается Опция A; для одноразового случая Опция C защитима. Не блокирует merge.
Ghost added 3 commits 2026-06-26 17:01:19 +03:00
docs: document SHARE_AI_WORKSPACE_TOKEN_BUDGET_PER_DAY and changelog the bug-fix batch (#161 #190 #207 #206 #159) acf2241e23 
Document the new per-workspace rolling-day token-budget env var in
.env.example alongside the existing share-assistant cost knobs, and add
[Unreleased] Fixed entries for #161/#190/#207/#206 plus a Security entry
for the #159 token budget.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
test(editor-ext): cover the transclusionSource NO_REASSIGN branch in id dedupe (#206) d32ad73158 
A colliding transclusionSource id is deliberately NOT reassigned (its id is a
cross-reference key), while a missing id is still filled. Add coverage for both:
two sources sharing an id keep it (red if the NO_REASSIGN guard is removed), and
a source with no id gets a fresh one.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
refactor(ai-chat): simplify share onFinish token extraction and cover the fallback (#159) eb5c8e6611 
onFinish always receives a totalUsage object, so the `?? {}` guard and
optional chaining were dead. Extract the field-level extraction into a
recordTurnUsage method (totalTokens, else input+output) and unit-test that
recordShareTokens receives the summed value when totalTokens is absent and the
authoritative total when present.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Ghost added 6 commits 2026-06-26 17:09:47 +03:00
perf(ai-chat): throttle stream + memoize markdown to stop CPU spikes on long runs 99d0cb8773 
On long agent runs (dozens of tool calls) the desktop app froze at 100% CPU with
no user interaction: useChat updated state on every streamed token, and
MessageItem/ReasoningBlock re-parsed the whole transcript's markdown (the marked
pipeline + DOMPurify) on every delta. Per-turn work grew quadratically and
saturated the main thread; the SSE stream drove it, so it hung "on its own".

- chat-thread: pass experimental_throttle (50ms) to useChat so the streamed
  messages state re-renders at most ~20 Hz instead of once per token.
- message-item: memoize MessageItem on a cheap per-message content signature
  (the streaming tail still re-renders as it grows; finalized rows are skipped),
  and render each text part via a memoized MarkdownPart so finalized parts are
  not re-parsed. The signature includes usage.reasoningTokens so the
  authoritative "Thinking - N tokens" count still snaps in at finish-step.
- reasoning-block: memoize the markdown render (useMemo on the text) and wrap the
  component in React.memo.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Merge remote-tracking branch 'gitea/develop' into fix/ai-chat-stream-perf 2f058a6e40 
# Conflicts:
#	apps/client/src/features/ai-chat/components/reasoning-block.tsx
test(review): address PR #182 review — tests + extract messageSignature, CHANGELOG 63c26042ba 
Resolve the PR #182 code-review (Request changes) on top of the already-merged
develop (the merge commit preserves both the markdown useMemo and the
collapseBlankLines fix in reasoning-block.tsx).

- Extract messageSignature from message-item.tsx into utils/message-signature.ts
  (matches the feature's "pure UIMessage helper + colocated test" convention) and
  export arePropsEqual so the memo seam is unit-testable. No logic change.
- Add utils/message-signature.test.ts covering every change signal (text grows,
  part appended, state flip, output appears, errorText appears, usage.reasoningTokens
  arriving on finish-step, metadata error/finishReason) plus the negative
  content-identical-clone case.
- Add components/message-item.test.ts for arePropsEqual (each prop diff -> false,
  identity fast-path -> true, same-content-different-object -> true, changed -> false).
- Add components/message-item-memo.test.tsx: render-level proof that finalized text
  parts are not re-parsed when only a tail part grows (MarkdownPart memo).
- CHANGELOG: add the user-facing 100% CPU freeze fix under [Unreleased] / Fixed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
docs(ai-chat): document load-bearing invariant of messageSignature memo eafd15f0ef 
PR #182 review (post-fix pass) surfaced two latent correctness risks in the
new MessageItem memo: the per-message signature tracks only [type, text length,
state, error/output presence] + metadata, so a part kind whose VISIBLE content
can change WITHOUT changing those fields would silently freeze a stale row.
Neither is reachable with the current toolset (tool output is set once;
streaming is append-only with a fixed id), so the correct fix is to harden the
documented invariant rather than hash output content on every delta (getPage
returns full page content — hashing it per-delta would tax the hot path this
PR optimizes).

Add a WARNING in messageSignature naming the two future triggers (a tool that
streams `preliminary` output; a client-side regenerate/edit that mutates a
finalized row in place) and the required action (extend the signature).

No behavior change (comment only). vitest src/features/ai-chat 189/189 pass,
tsc clean for the touched files.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
test(ai-chat): simplify msg factory and lock signature↔render coupling cb61274187 
Address non-blocking review items on the AI-chat stream-perf PR:

- Drop the unused `metadata` param from the `msg` test factory in
  message-item.test.ts; no caller passed it.
- Add a per-part-kind coupling guard to message-signature.test.ts that, for
  each part kind rendered today (text, reasoning, tool-*) plus the metadata
  banners, asserts that mutating a field the MessageItem render body DRAWS
  flips messageSignature — an executable lock for the load-bearing memo
  invariant documented in message-signature.ts.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
merge: fold #182 (AI-chat stream throttle + render memoization) into #212 df50f23d58
vvzvlad commented 2026-06-26 17:22:27 +03:00
Owner
Copy Link

Code review (re-review) — PR #212: батч багфиксов после правок

Вердикт: Approve. Все три прошлых блокера закрыты по существу (не косметически), а новый код дельты — рефактор учёта токенов в share-AI и тесты — корректен и покрыт; регрессий и новых блокеров нет.

Ре-ревью дельты 1d610b3a..df50f23d (12 файлов, +697/−42); в дельту влит и #182 (ревьюился отдельно, чисто). Аспекты: security, stability, conventions, documentation, regressions, test-coverage, simplification (параллельные ревьюеры + judge).

Статус прошлых блокеров

  • env-var в .env.example — закрыт. SHARE_AI_WORKSPACE_TOKEN_BUDGET_PER_DAY=1000000 задокументирован с описанием rolling-day семантики, FAILS-CLOSED при недоступном Redis и дефолта (.env.example, блок на строках ~190–197).
  • CHANGELOG [Unreleased] (#161 #190 #207 #159 #206) — закрыт. Все пять записей присутствуют и разнесены по верным секциям: #161/#190/#207/#206 в Fixed, #159 в Security; формулировки содержательные, не заглушки.
  • тест ветки NO_REASSIGN (transclusionSource) — закрыт. unique-id.util.test.ts регистрирует реальный TransclusionSource тип и прогоняет настоящий addUniqueIdsToDoc: один тест проверяет, что коллизия id НЕ переназначается (оба узла сохраняют "src"), второй — что отсутствующий id всё равно заполняется. Совпадает с реализацией (NO_REASSIGN = new Set(["transclusionSource"]), needsNewId = currentId == null || (isDuplicate && !NO_REASSIGN.has(...))). Не поверхностно.

Must fix before merge

Нет.

Non-blocking

Нет.

Test coverage

Покрыто. Единственная новая логика дельты — извлечение тела onFinish в метод recordTurnUsage (public-share-chat.service.ts) — покрыта тремя кейсами в public-share-chat.spec.ts: fallback-сумма input+output при отсутствии totalTokens, трактовка отсутствующих компонент как 0, приоритет авторитетного totalTokens. Удаление прежнего guard'а totalUsage ?? {} безопасно: AI SDK типизирует totalUsage в onFinish как необязуемый LanguageModelUsage, а внутренние поля метод гасит через ?? 0. Слитый #182 (throttle/memo) интегрирован без следов конфликтов: messageSignature импортируется и используется в memo(MessageItem, arePropsEqual), experimental_throttle: STREAM_THROTTLE_MS подключён с очисткой таймера — внутренности не переразбирались.

## Code review (re-review) — PR #212: батч багфиксов после правок **Вердикт: Approve.** Все три прошлых блокера закрыты по существу (не косметически), а новый код дельты — рефактор учёта токенов в share-AI и тесты — корректен и покрыт; регрессий и новых блокеров нет. _Ре-ревью дельты `1d610b3a..df50f23d` (12 файлов, +697/−42); в дельту влит и #182 (ревьюился отдельно, чисто). Аспекты: security, stability, conventions, documentation, regressions, test-coverage, simplification (параллельные ревьюеры + judge)._ ### Статус прошлых блокеров - **env-var в `.env.example`** — закрыт. `SHARE_AI_WORKSPACE_TOKEN_BUDGET_PER_DAY=1000000` задокументирован с описанием rolling-day семантики, FAILS-CLOSED при недоступном Redis и дефолта (`.env.example`, блок на строках ~190–197). - **CHANGELOG `[Unreleased]` (#161 #190 #207 #159 #206)** — закрыт. Все пять записей присутствуют и разнесены по верным секциям: #161/#190/#207/#206 в `Fixed`, #159 в `Security`; формулировки содержательные, не заглушки. - **тест ветки `NO_REASSIGN` (transclusionSource)** — закрыт. `unique-id.util.test.ts` регистрирует реальный `TransclusionSource` тип и прогоняет настоящий `addUniqueIdsToDoc`: один тест проверяет, что коллизия id НЕ переназначается (оба узла сохраняют `"src"`), второй — что отсутствующий id всё равно заполняется. Совпадает с реализацией (`NO_REASSIGN = new Set(["transclusionSource"])`, `needsNewId = currentId == null || (isDuplicate && !NO_REASSIGN.has(...))`). Не поверхностно. ### Must fix before merge Нет. ### Non-blocking Нет. ### Test coverage Покрыто. Единственная новая логика дельты — извлечение тела `onFinish` в метод `recordTurnUsage` (`public-share-chat.service.ts`) — покрыта тремя кейсами в `public-share-chat.spec.ts`: fallback-сумма input+output при отсутствии `totalTokens`, трактовка отсутствующих компонент как 0, приоритет авторитетного `totalTokens`. Удаление прежнего guard'а `totalUsage ?? {}` безопасно: AI SDK типизирует `totalUsage` в `onFinish` как необязуемый `LanguageModelUsage`, а внутренние поля метод гасит через `?? 0`. Слитый #182 (throttle/memo) интегрирован без следов конфликтов: `messageSignature` импортируется и используется в `memo(MessageItem, arePropsEqual)`, `experimental_throttle: STREAM_THROTTLE_MS` подключён с очисткой таймера — внутренности не переразбирались.
vvzvlad merged commit e3b23e0d26 into develop 2026-06-26 17:43:56 +03:00
vvzvlad deleted branch fix/mainline-bugs-2026-06-26 2026-06-26 17:45:16 +03:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
bug
Something isn't working
 epic
Large multi-phase effort spanning many changes
 feature
New functionality request
 idea
Idea / proposal for discussion
 needs-human
эскалация: нужно решение человека
 review/approved
в последнем ревью нет открытых blocking-находок
 review/changes-requested
последнее ревью оставило открытые blocking-находки
 review/needs
head не ревьюился (head != reviewed_head)
No Label bug
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent_coder agent_reviewer vvzvlad
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vvzvlad/gitmost#212
Delete Branch "fix/mainline-bugs-2026-06-26"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?