From 78cc0194928085d45e2905381972347b2d0eb791 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sun, 28 Jun 2026 23:45:45 +0300 Subject: [PATCH] fix(#248 F1): remove dead intentional-clear escape hatch from empty-guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 review F1 (maintainer decision: variant A). The store-side empty-guard's `context?.intentionalClear === true` branch was dead: `intentionalClear` is never set in production (connection context is {user, actor, aiChatId}); it appeared only in the guard and a hand-injected spec, so the guard already blocked empty-over-non-empty unconditionally. - persistence.extension.ts: drop the dead branch; the guard now simply skips empty-over-non-empty, full stop. Reference issue #251 (real intentional-clear UX) in the comment where the branch was. - persistence-store.spec.ts: remove the misleading "persists an intentional clear" escape-hatch test (false coverage — green only because the flag was injected by hand). Real guard tests (empty-over-empty allowed, empty-over-non-empty blocked, etc.) kept. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../extensions/persistence-store.spec.ts | 27 ++----------------- .../extensions/persistence.extension.ts | 18 +++++++------ 2 files changed, 12 insertions(+), 33 deletions(-) diff --git a/apps/server/src/collaboration/extensions/persistence-store.spec.ts b/apps/server/src/collaboration/extensions/persistence-store.spec.ts index c8ac68b9..8cf5fd0d 100644 --- a/apps/server/src/collaboration/extensions/persistence-store.spec.ts +++ b/apps/server/src/collaboration/extensions/persistence-store.spec.ts @@ -210,8 +210,8 @@ describe('PersistenceExtension.onStoreDocument — Approach-A boundary snapshot' // but not the STORE path, so an empty doc (a client/agent glitch, a bad // merge, an emptying transclusion) was written straight over the page and the // content was wiped silently. The store-side empty-guard now skips the write - // when the incoming doc is empty and the stored page is non-empty, unless an - // explicit intentional-clear signal is present. + // when the incoming doc is empty and the stored page is non-empty. A real + // intentional-clear UX is tracked separately in issue #251. it('does NOT overwrite non-empty content with a momentarily-empty live doc (persist-6)', async () => { const emptyDoc = { type: 'doc', content: [{ type: 'paragraph' }] }; const document = ydocFor(emptyDoc); @@ -229,29 +229,6 @@ describe('PersistenceExtension.onStoreDocument — Approach-A boundary snapshot' expect(historyQueue.add).not.toHaveBeenCalled(); }); - // persist-6 — a legitimate clear is NOT broken: with the explicit - // intentional-clear signal, emptying a non-empty page still persists. - it('persists an intentional clear of a non-empty page (persist-6 escape hatch)', async () => { - const emptyDoc = { type: 'doc', content: [{ type: 'paragraph' }] }; - const document = ydocFor(emptyDoc); - pageRepo.findById.mockResolvedValue({ - ...persistedHumanPage('IGNORED'), - content: doc('IMPORTANT RICH CONTENT'), - }); - - await ext.onStoreDocument({ - documentName: `page.${PAGE_ID}`, - document, - context: { - user: { id: USER_ID, name: 'Alice' }, - actor: 'user', - intentionalClear: true, - }, - } as any); - - expect(pageRepo.updatePage).toHaveBeenCalledTimes(1); - }); - // persist-6 — a brand-new / already-empty page is unaffected: an empty store // over empty stored content is not blocked (it short-circuits as unchanged). it('does not block an empty store over an already-empty page (persist-6)', async () => { diff --git a/apps/server/src/collaboration/extensions/persistence.extension.ts b/apps/server/src/collaboration/extensions/persistence.extension.ts index 47fadf8f..0739a8c6 100644 --- a/apps/server/src/collaboration/extensions/persistence.extension.ts +++ b/apps/server/src/collaboration/extensions/persistence.extension.ts @@ -217,21 +217,23 @@ export class PersistenceExtension implements Extension { // when the live doc isEmpty); the STORE path did not, so an empty // serialization was written straight over the page, wiping it // silently. Skip the write when the incoming doc is an empty - // paragraph doc AND the stored page is non-empty — unless the writer - // sends an explicit intentional-clear signal (a deliberate - // select-all + delete), the one case where emptying is the user's - // intent. New/empty pages are unaffected (stored content is already - // empty), and an unchanged doc was already short-circuited above. - const intentionalClear = context?.intentionalClear === true; + // paragraph doc AND the stored page is non-empty. New/empty pages are + // unaffected (stored content is already empty), and an unchanged doc + // was already short-circuited above. + // + // This unconditionally blocks empty-over-non-empty: a deliberate + // select-all + delete is currently indistinguishable from a glitch at + // this layer, so data-loss prevention wins. A real intentional-clear + // UX (a distinct signal threaded from the client) is tracked in issue + // #251; do not re-add an escape hatch here without that signal. if ( - !intentionalClear && isEmptyParagraphDoc(tiptapJson as any) && page.content && !isEmptyParagraphDoc(page.content as any) ) { this.logger.warn( `Skipping store for ${pageId}: empty live doc would overwrite ` + - `non-empty persisted content (no intentional-clear signal)`, + `non-empty persisted content`, ); page = null; return;