diff --git a/docs/plans/2026-07-16-conflict-review-cohort-audit.md b/docs/plans/2026-07-16-conflict-review-cohort-audit.md new file mode 100644 index 0000000000..2e0e2a9d7c --- /dev/null +++ b/docs/plans/2026-07-16-conflict-review-cohort-audit.md @@ -0,0 +1,81 @@ +# Conflict-review cohort and persisted-data audit (plan Task 1) + +**Date:** 2026-07-16 +**Scope:** Task 1 of [`2026-07-13-sync-simplification-plan.md`](2026-07-13-sync-simplification-plan.md). Blocks the conflict-review rollback (Task 6) and authorizes the producer freeze. +**Baseline:** master `6f88775ea2`. Feature under audit: conflict review / conflict journal, merged `962c5bbeb1` (PR #8874, 2026-07-11). + +## Decision summary + +| Question | Decision | +| ----------------------------------------------------- | --------------------------------------------------------------------------------------------------- | +| Producer freeze before the next release cut? | **Yes** — freeze both producers now (landed with this document). | +| Are Snap `edge` / Play `internal` supported cohorts? | **No** — dogfood/pre-release. No export or migration obligation. | +| Journal retention / export / deletion policy | **No export.** Rely on the existing 14-day / 200-row expiry; delete store, reader, UI and marker together in Task 6. | + +These are product decisions, recorded explicitly rather than inferred from release tags, per the Task 1 acceptance criteria. + +## 1. Distribution channels carrying `962c5bbeb1` + +`git tag --contains 962c5bbeb1` matches no release tag (only the `issue-8983-verbose` working tag). It is on `master`. v18.14.0 was cut 2026-07-10 and does not contain it. + +| Channel | Trigger | From master? | Public? | Evidence | +| ------------------------------ | ---------------------- | ------------ | ------------------------ | ----------------------------------------------------------- | +| **Snap Store `edge`** | every push to `master` | **Yes** | **Yes — unauthenticated** | `.github/workflows/build.yml:2-6`, `:174-191` | +| **Google Play `internal`** | every push to `master` | **Yes** | Opt-in testers (Play caps at 100) | `.github/workflows/build-android.yml:135-150` | +| GHCR `supersync:latest` | push to `master` | Yes | Server image only — no review UI | `.github/workflows/supersync-docker.yml:3-14` | +| GitHub Release (desktop) | tag `v*` | No | Yes | `build.yml:136-141`, `:515-524` | +| Web app | `release: published`, non-prerelease | No | Yes | `build-update-web-app-on-release.yml:3-4`, `:11` | +| Play production / iOS / stores | tag `v*` | No | Yes | `build-android.yml:190-198`, `build-ios.yml:2-7` | +| Cloudflare Pages preview | `pull_request` | No | Yes (URL in PR) | `pr-preview-build.yml:3-6` | + +**Two cohorts already run the feature today.** Snap `edge` is the material one: it is public, requires no invitation, and snapd auto-refreshes subscribers. The Play `internal` track auto-updates its testers on-device by design (`build-android.yml:129-134`). Subscriber counts for both live in Snap Store / Play Console telemetry and are not knowable from the repo. + +**Not exposed:** Electron desktop ships **no auto-updater** (`electron-builder.yaml:65-70`; the `autoUpdater` block in `electron/start-app.ts:474-486` is commented out) and master builds use `--publish never`; the web app deploys only on published non-prerelease releases; there is no nightly/canary release channel. + +**Consequence:** the persisted-data obligation began at the first master push after `962c5bbeb1`, not at a future tag. The next release cut does not *create* the obligation — it expands it from these two pre-release cohorts to the entire stable fleet, which is what the freeze prevents. + +## 2. Stable baseline vs master + +- v18.14.0 (deployed stable): schema **v2**, op-log DB **v7**. +- master: schema **v4**, op-log DB **v10**. +- Both the v2→v3 replace/patch barrier and the v3→v4 marked-project-delete barrier are unreleased. + +Unless reverted before the next tag, schema v3 compatibility, schema v4 delete-wins behaviour, and conflict review reach stable **together**. The deployed stable fleet stays v2/DB 7 until that cut. + +## 3. What the journal persists + +`ConflictJournalEntry` (`src/app/op-log/sync/conflict-journal.model.ts:98-112`) stores `entityTitle` plus `fieldDiffs` (`:66-93`), whose `localVal`/`remoteVal` hold **arbitrary entity field values copied verbatim** from op payloads — by design: "capture the discarded (losing) side of a conflict verbatim" (`:8-10`). There is no field allowlist; `NOISE_FIELDS` affects classification only, not storage. `kind: 'action'` diffs persist raw action payloads (`:85-92`) and are the widest content surface. + +So rows contain **real user content** (task/note/project titles and discarded field values, including note bodies). + +The blast radius is nonetheless small: + +- **Device-local only.** Standalone IndexedDB `SUP_CONFLICT_JOURNAL` v1, store `conflicts` (`model.ts:181-185`), deliberately separate from the op-log `SUP_OPS` DB. +- **Never uploaded.** No sync/upload path reads the journal; sync code only calls `record()` and `clearAll()`. The only readers are UI. +- **Not in backups or exports.** `BackupService.loadCompleteBackup` builds solely from NgRx `AppDataComplete`, and the journal is not in NgRx; the DB constants appear nowhere outside the journal's own files. +- **Self-expiring.** `JOURNAL_RETENTION_DAYS = 14`, `JOURNAL_MAX_ENTRIES = 200`, pruned by `pruneOnStart()` (APP_INITIALIZER, `main.ts:313-321`) and opportunistically in `record()` above 220 rows. + +**Known gap (accepted):** the 14-day age bound is enforced only on app start or when the row count crosses 220, so an always-on desktop can hold rows past 14 days, bounded at ~220 rows. With the writer frozen no new rows accrue, and the next app start prunes the rest. + +**Known gap (accepted):** there is no user-facing way to clear the journal — the review page offers only keep/flip. `clearAll()` is reachable only via dataset replacement (`backup.service.ts:183`) or raw op-log rebuild (`operation-log-sync.service.ts:2174`). Adding a clear button was rejected: it grows UI surface on a feature slated for deletion, and with the writer frozen the content expires on its own. + +## 4. Retention decision + +No export path is owed, because both carrying cohorts are pre-release (§1) and the data never left the device (§3). + +Task 6 must delete the writer, store, reader, UI, route, banner, badge **and** the `SUP_CONFLICT_JOURNAL_CLEARED_BEFORE` localStorage marker together. The marker is a cross-profile privacy fail-safe (#9045): `clearAll()` swallows IndexedDB errors, so if `db.clear()` fails the rows physically survive, and the marker is what hides them from every read path until `pruneOnStart()` reclaims them. **It must not be stranded** — a marker left behind with the store deleted protects nothing, and a store left behind with the marker deleted exposes profile A's titles to profile B. + +Deleting the journal's IndexedDB is itself part of Task 6: dropping the store code without deleting `SUP_CONFLICT_JOURNAL` would leave user content on disk with no code path to reach or prune it. A live constraint already recorded in-code (`conflict-journal.service.ts:68-70`) is that any future "reset app data" flow clearing localStorage must also clear the journal DB. + +## 5. What this audit does not authorize + +Per the Task 1 stop condition, this audit authorizes **removal of producers only**. It does not authorize schema downgrade, nor reader removal, while supported stored data remains possible. Task 6 remains gated on the preserve list in the plan (schema v3/v4 barriers, delete-wins, #9048 cascade recovery, #9035 clientId tiebreak, #9025 LWW projectId sanitization, #9045 decrypt-path footprint auth). + +## 6. Action taken + +The producer freeze landed with this document — see `remote-ops-processing.service.ts`, the single production entry point into `autoResolveConflictsLWW`: + +- `disableDisjointMerge: true` — conflicts resolve by whole-entity LWW, the behaviour of every released version to date, so the stable fleet gains no merge behaviour it would later be migrated off. +- `disableConflictJournal: true` — no new rows are persisted. + +Both are caller-wired rather than global, so `ConflictResolutionService` keeps the capability intact for its own tests and the freeze reverts by deleting two lines. Rows already written on edge/internal builds stay readable and expire on their own; the full rollback proceeds on its own schedule in Task 6. diff --git a/src/app/op-log/sync/conflict-resolution-persistence.integration.spec.ts b/src/app/op-log/sync/conflict-resolution-persistence.integration.spec.ts index 8f0e5253f7..9cee29d412 100644 --- a/src/app/op-log/sync/conflict-resolution-persistence.integration.spec.ts +++ b/src/app/op-log/sync/conflict-resolution-persistence.integration.spec.ts @@ -1,3 +1,4 @@ +import { signal } from '@angular/core'; import { TestBed } from '@angular/core/testing'; import { Action, ActionReducer, Store } from '@ngrx/store'; import { of } from 'rxjs'; @@ -242,6 +243,9 @@ describe('ConflictResolutionService persistence (integration, real store)', () = const conflictJournal = jasmine.createSpyObj( 'ConflictJournalService', ['record'], + // Real signal, not a spy: `unreviewedCount` is a signal on the service, and + // the content-conflict banner reads it to decide whether to offer REVIEW. + { unreviewedCount: signal(0) }, ); conflictJournal.record.and.resolveTo(); const syncConflictBanner = jasmine.createSpyObj( diff --git a/src/app/op-log/sync/conflict-resolution.service.spec.ts b/src/app/op-log/sync/conflict-resolution.service.spec.ts index 2162990abb..23f055c9d5 100644 --- a/src/app/op-log/sync/conflict-resolution.service.spec.ts +++ b/src/app/op-log/sync/conflict-resolution.service.spec.ts @@ -11,6 +11,7 @@ import { OperationLogStoreService } from '../persistence/operation-log-store.ser import { SnackService } from '../../core/snack/snack.service'; import { BannerService } from '../../core/banner/banner.service'; import { BannerId } from '../../core/banner/banner.model'; +import { T } from '../../t.const'; import { ValidateStateService } from '../validation/validate-state.service'; import { of } from 'rxjs'; import { @@ -600,6 +601,114 @@ describe('ConflictResolutionService', () => { expect(mockSnackService.open).not.toHaveBeenCalled(); }); + describe('content-conflict banner REVIEW action', () => { + // This banner fires off the resolutions, NOT off the journal, so its + // REVIEW action has to be gated on the journal actually holding rows — + // otherwise the producer freeze (or a swallowed record() failure) sends + // the user to an empty review page at the exact moment they are worried + // about a discarded edit. + const buildContentLossConflicts = (): EntityConflict[] => { + const now = Date.now(); + return [ + createConflict( + 'task-1', + [ + { + ...createOpWithTimestamp('local-1', 'client-a', now - 1000), + payload: { + actionPayload: { + task: { id: 'task-1', changes: { title: 'My local title' } }, + }, + entityChanges: [], + }, + }, + ], + [ + { + ...createOpWithTimestamp('remote-1', 'client-b', now), + payload: { + actionPayload: { + task: { id: 'task-1', changes: { title: 'Remote title' } }, + }, + entityChanges: [], + }, + }, + ], + ), + ]; + }; + + const getJournal = (): ConflictJournalService => + (service as unknown as { conflictJournal: ConflictJournalService }) + .conflictJournal; + + const openContentBanner = async (): Promise> => { + const openBannerSpy = spyOn(TestBed.inject(BannerService), 'open'); + mockStore.select.and.returnValue(of({ id: 'task-1', title: 'Remote title' })); + const conflicts = buildContentLossConflicts(); + mockOperationApplier.applyOperations.and.resolveTo({ + appliedOps: conflicts[0].remoteOps, + }); + await service.autoResolveConflictsLWW(conflicts); + return openBannerSpy; + }; + + it('omits the REVIEW action when the journal has nothing to review', async () => { + // record() no-ops, so the unreviewed count stays 0 — the state produced + // both by the producer freeze and by a swallowed record() failure. + spyOn(getJournal(), 'record').and.resolveTo(); + + const openBannerSpy = await openContentBanner(); + + const banner = openBannerSpy.calls.mostRecent().args[0]; + expect(banner.id).toBe(BannerId.SyncConflictContentResolved); + // Message + built-in dismiss = exactly the released v18.14.0 banner. + expect(banner.action).toBeUndefined(); + }); + + it('keeps the REVIEW action when the journal holds unreviewed entries', async () => { + spyOn(getJournal(), 'record').and.resolveTo(); + ( + getJournal() as unknown as { + _unreviewedCount: { set: (v: number) => void }; + } + )._unreviewedCount.set(2); + + const openBannerSpy = await openContentBanner(); + + const banner = openBannerSpy.calls.mostRecent().args[0]; + expect(banner.action?.label).toBe(T.F.SYNC.CONFLICT_REVIEW.BANNER_REVIEW); + }); + }); + + // Callee half of the producer freeze: remote-ops-processing.service.spec.ts + // asserts the production caller PASSES the flag, but without this, deleting + // the gate inside autoResolveConflictsLWW leaves every spec green while the + // fleet silently resumes persisting the discarded side of every conflict. + // The journal-ON default is already covered by the #8956 multi-entity test. + it('records nothing when disableConflictJournal is set (producer freeze)', async () => { + const journal = (service as unknown as { conflictJournal: ConflictJournalService }) + .conflictJournal; + const recordSpy = spyOn(journal, 'record').and.resolveTo(); + const now = Date.now(); + const conflicts = [ + createConflict( + 'task-1', + [createOpWithTimestamp('local-1', 'client-a', now - 1000)], + [createOpWithTimestamp('remote-1', 'client-b', now)], + ), + ]; + mockOperationApplier.applyOperations.and.resolveTo({ + appliedOps: conflicts[0].remoteOps, + }); + + await service.autoResolveConflictsLWW(conflicts, [], { + disableConflictJournal: true, + }); + + expect(recordSpy).not.toHaveBeenCalled(); + }); + it('escapes task titles before putting them in the innerHTML banner (XSS guard)', async () => { const bannerService = TestBed.inject(BannerService); const openBannerSpy = spyOn(bannerService, 'open'); diff --git a/src/app/op-log/sync/conflict-resolution.service.ts b/src/app/op-log/sync/conflict-resolution.service.ts index 4af18fbbf2..6178f1d995 100644 --- a/src/app/op-log/sync/conflict-resolution.service.ts +++ b/src/app/op-log/sync/conflict-resolution.service.ts @@ -124,6 +124,22 @@ interface ResolvedConflicts { interface AutoResolveConflictsLwwOptions { callerHoldsOperationLogLock?: boolean; disableDisjointMerge?: boolean; + /** + * Skip conflict-journal emission entirely (observe-only hook, so this can + * never change which op resolution picks). + * + * Set by the production caller as a PRODUCER FREEZE ahead of the + * conflict-review rollback: journal rows capture the discarded side of a + * conflict verbatim (titles, arbitrary field values), and that device-local + * data obligation must not expand to the stable fleet on the next release + * tag while the feature is still slated for removal. + * + * Ceiling: rows already written on edge/internal builds stay readable and + * expire on their own (14 days / 200 rows). Upgrade path: the store, reader, + * UI and the `SUP_CONFLICT_JOURNAL_CLEARED_BEFORE` marker are deleted + * together in the conflict-review rollback, after which this option goes too. + */ + disableConflictJournal?: boolean; remoteApplyLifecycleOwnedByCaller?: boolean; } @@ -1720,12 +1736,14 @@ export class ConflictResolutionService { ); } - for (const plan of lwwPlans) { - await this._journalResolution(plan); - } - for (const merged of successfulMergedResolutions) { - if (writtenMergedOpIds.has(merged.mergedOp.id)) { - await this._journalMergedResolution(merged.plan); + if (!options.disableConflictJournal) { + for (const plan of lwwPlans) { + await this._journalResolution(plan); + } + for (const merged of successfulMergedResolutions) { + if (writtenMergedOpIds.has(merged.mergedOp.id)) { + await this._journalMergedResolution(merged.plan); + } } } @@ -1816,16 +1834,25 @@ export class ConflictResolutionService { const named = labels.join(', '); const taskList = contentConflicts.length > MAX_NAMED ? `${named} …` : named; + // This banner fires off the resolutions, not off the journal, so REVIEW must + // be gated on the journal actually holding rows: under the producer freeze + // (or a swallowed `record()` failure) it would otherwise land the user on the + // review page's empty state. Without the action this is exactly the released + // v18.14.0 banner — message + built-in dismiss (no action2). Count is fresh: + // the journal loop awaits `record()`, which refreshes it, before this step. + const hasEntriesToReview = this.conflictJournal.unreviewedCount() > 0; + this.bannerService.open({ id: BannerId.SyncConflictContentResolved, ico: 'sync_problem', msg: T.F.SYNC.B.CONTENT_CONFLICT_RESOLVED, translateParams: { taskList }, - // SPAP-15: REVIEW opens the conflicts page; DISMISS auto-renders (no action2). - action: { - label: T.F.SYNC.CONFLICT_REVIEW.BANNER_REVIEW, - fn: () => this.syncConflictBanner.navigateToReview(), - }, + action: hasEntriesToReview + ? { + label: T.F.SYNC.CONFLICT_REVIEW.BANNER_REVIEW, + fn: () => this.syncConflictBanner.navigateToReview(), + } + : undefined, }); } diff --git a/src/app/op-log/sync/remote-ops-processing.service.spec.ts b/src/app/op-log/sync/remote-ops-processing.service.spec.ts index a14979454a..3f4814c4e2 100644 --- a/src/app/op-log/sync/remote-ops-processing.service.spec.ts +++ b/src/app/op-log/sync/remote-ops-processing.service.spec.ts @@ -427,6 +427,53 @@ describe('RemoteOpsProcessingService', () => { expect(JSON.stringify(summary)).not.toContain('private'); }); + // Producer freeze for the conflict-review rollback: this is the only + // production entry point into autoResolveConflictsLWW, so if these two flags + // are ever dropped the stable fleet silently starts persisting the discarded + // side of every conflict verbatim again. Delete this test with the freeze. + it('should freeze both conflict-review producers on the production resolve path', async () => { + const localOp = { + id: 'local-op', + entityType: 'TASK', + entityId: 'task-1', + payload: { title: 'local title' }, + } as Operation; + const remoteOp = { + id: 'remote-op', + entityType: 'TASK', + entityId: 'task-1', + payload: { title: 'remote title' }, + schemaVersion: 1, + } as Operation; + spyOn(service, 'detectConflicts').and.resolveTo({ + nonConflicting: [], + conflicts: [ + { + entityType: 'TASK', + entityId: 'task-1', + localOps: [localOp], + remoteOps: [remoteOp], + suggestedResolution: 'manual', + }, + ], + }); + conflictResolutionServiceSpy.autoResolveConflictsLWW.and.resolveTo({ + localWinOpsCreated: 0, + }); + vectorClockServiceSpy.getEntityFrontier.and.resolveTo(new Map()); + + await service.processRemoteOps([remoteOp]); + + expect(conflictResolutionServiceSpy.autoResolveConflictsLWW).toHaveBeenCalledWith( + jasmine.any(Array), + jasmine.any(Array), + jasmine.objectContaining({ + disableDisjointMerge: true, + disableConflictJournal: true, + }), + ); + }); + it('should drop operations if migrateOperation returns null', async () => { const remoteOps: Operation[] = [ { id: 'op1', schemaVersion: 1 } as Operation, diff --git a/src/app/op-log/sync/remote-ops-processing.service.ts b/src/app/op-log/sync/remote-ops-processing.service.ts index 8a333f73a5..51c3e137a5 100644 --- a/src/app/op-log/sync/remote-ops-processing.service.ts +++ b/src/app/op-log/sync/remote-ops-processing.service.ts @@ -437,10 +437,26 @@ export class RemoteOpsProcessingService { // Auto-resolve conflicts using Last-Write-Wins strategy. // Piggyback non-conflicting ops so they're applied with resolved conflicts. // Validation failure is surfaced via the session-validation latch. + // + // PRODUCER FREEZE for the conflict-review rollback. Both flags are set + // here, at the only production entry point, so the feature's two + // producers stop at the fleet boundary while the service keeps the + // capability intact for its own tests and for a one-line revert: + // - disableDisjointMerge: conflicts resolve by whole-entity LWW, which + // is the behaviour of every released version to date, so the stable + // fleet gains nothing it would later have to be migrated off. + // - disableConflictJournal: stop persisting the discarded side of a + // conflict verbatim, so that device-local data obligation does not + // expand beyond the edge/internal builds that already carry it. + // Reverting the freeze = drop these two lines. const lwwResult = await this.conflictResolutionService.autoResolveConflictsLWW( conflicts, nonConflicting, - { callerHoldsOperationLogLock: true }, + { + callerHoldsOperationLogLock: true, + disableDisjointMerge: true, + disableConflictJournal: true, + }, ); localWinOpsCreated = lwwResult.localWinOpsCreated; return;