Lucas WintherandClaude Opus 5.5
be04fab098
Captures the owner's requirements and decisions from the planning conversation, current branch/worktree state, open tasks 17-19, pending owner decisions and the controller's rulings so a new agent can resume. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
137 lines
12 KiB
Markdown
137 lines
12 KiB
Markdown
# Task 8 report: sync engine, snapshots, diff, apply, Changes inbox
|
||
|
||
**Status:** DONE_WITH_CONCERNS (the concerns are scope gaps, listed below; no known defects)
|
||
**Branch:** `task/8` (worktree `/home/user/STTracker/.worktrees/t8`, based on feat/phase1 33631ae)
|
||
|
||
## Commits
|
||
- `37a0b0b` feat(core): source record snapshots with stable hash and gzipped versions
|
||
- `1975e97` feat(core): sync engine and changes inbox (includes the docs)
|
||
|
||
## What was built
|
||
- `packages/core/src/sync/snapshot.ts`: `normalizeRecord`, `stableStringify`, `sameValue`, `hashRecord` (sha256 of stable JSON of `{kind, work, entry|engagement|listen|list, deleted, mergedIntoKey}`, raw excluded), `encodeVersion`/`decodeVersion` (base64 of `Bun.gzipSync` of `{normalized, raw}`).
|
||
- `packages/core/src/sync/apply.ts`: `applyRecord` handles one non-listen record.
|
||
- Same hash: only `last_seen` moves.
|
||
- New record: `matchWork` in its own transaction, then one apply transaction. It creates or patches the entry (with a root-ancestor entry), or ensures the entry and then de-duplicates or adds the engagement plus its `engagement_sources` row. It writes an `added` `auto_applied` event.
|
||
- Changed record: diffs against the previous version, decides each field against `field_sources`, and applies or files pending `changed`/`conflict` events, never filing the same pending event twice (anti-flip-flop). It also handles `deleted: true` (pending `removed`) and a new `mergedIntoKey` (pending `merged`).
|
||
- Mapped records are never re-matched, and deleted targets are never revived.
|
||
- Also exports `markRemoved`, the field mappers, `finishedWithin24h` and `sameStartInstant`.
|
||
- `packages/core/src/sync/run.ts`: `runSync(deps, plugin, connectionId, mode)`, plus the `Connection`, `SyncDeps`, `SyncMode` and `RunSummary` types.
|
||
- Checks that the plugin matches the connection.
|
||
- Books the run in `source_runs` (status, counts, log, error) and updates the connection's `last_run_at` and `status`: `error` on failure, `error`→`active` on success.
|
||
- Batches listens (500 at a time).
|
||
- Marks absent records removed only after a `full` pull that finished with zero record errors.
|
||
- `packages/core/src/sync/changes.ts`:
|
||
- `listChanges`: status defaults to `pending`; filter by `connectionId`; pages newest-first by UUIDv7 id with `nextCursor`.
|
||
- `resolveChange`: claims the event with a conditional update. It handles accept, keep_mine and ignore for changed/conflict/removed/merged events, including the admin-merge events (`toWorkId`) that `adminMergeWorks` already writes.
|
||
- `packages/core/src/sync/listens.ts`: `ingestListens(db, userId, source, records, actor?)`. Insert-or-ignore on (user, source, source_key). Matches only through `mbids.recording`: an existing work, or a new shared `track` work with the `musicbrainz_recording` id, retried on a unique-key race. It never renames an existing work.
|
||
- Docs:
|
||
- `docs/ARCHITECTURE.md`: core map and the real sync data flow.
|
||
- `docs/DECISIONS.md`: new entry at the top covering hash composition, the per-field decision, anti-flip-flop, no-revive, root ancestor, never re-matching, dedupe, removal and merge accept.
|
||
- `docs/TODO.md`: 5 known gaps.
|
||
|
||
## Files changed
|
||
packages/core/src/sync/{snapshot,apply,run,changes,listens}.ts; packages/core/test/sync/{helpers.ts,run.test.ts,listens.test.ts,snapshot.test.ts}; docs/{ARCHITECTURE,DECISIONS,TODO}.md.
|
||
|
||
## TDD evidence
|
||
RED (before any `src/sync` existed): `bun test packages/core/test/sync`
|
||
```
|
||
error: Cannot find module '../../src/sync/listens.ts' ...
|
||
0 pass
|
||
3 fail
|
||
3 errors
|
||
```
|
||
GREEN, SQLite: `bun test packages/core/test/sync` → `20 pass, 0 fail`.
|
||
GREEN, both DBs: `TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres bun test packages/core/test/sync` → `37 pass, 0 fail` (17 DB tests × 2 dialects + 3 pure snapshot tests).
|
||
Full suite: SQLite `230 pass, 0 fail`; both DBs `347 pass, 0 fail`.
|
||
|
||
The implementation passed on its first run, so I checked the tests by mutation. I broke each rule in turn, re-ran, and restored the code:
|
||
| Mutation | Test that failed |
|
||
|---|---|
|
||
| apply to "user"-owned fields | user-edited rating; two connections disagree |
|
||
| drop the anti-flip-flop check | two connections disagree |
|
||
| remove after a failed pull | failed pull marks nothing removed |
|
||
| disable dedupe | same play from two sources |
|
||
| skip the same-hash short-circuit | second identical pull |
|
||
| revive a deleted entry on a new record | *nothing at first*; I extended the no-revive test (new entry + engagement records on a deleted entry's work), and it now fails under this mutation |
|
||
|
||
Tests cover all mandatory Review-focus cases plus: root-ancestor entry (including an existing deleted root left untouched); a `changes` pull never removing by absence; `deleted: true` → removed, then ignore; no revive; an id-less (CSV) record updated through its mapping with no new draft; `resolveChange` scoped to the user and rejecting non-pending events; `listChanges` paging and connection filter; plugin/connection mismatch refused; listens through `runSync` (counted, not snapshotted, never removed).
|
||
|
||
## Typecheck
|
||
`bun run typecheck` → `$ tsc --noEmit -p tsconfig.json` with no errors.
|
||
|
||
## Choices worth reviewing
|
||
- **`changed` payload `from`.** It is the current STTracker value (what accept replaces), not the previous source value.
|
||
- **Version payload.** It stores `{normalized, raw}` so the next change can be diffed field by field. The schema has no other place for the previous normalised record.
|
||
- **A new record on an existing live entry.** It goes through the same per-field decision. Otherwise a first sync of a new connection would overwrite the user's fields, because `upsertEntry` re-stamps every key it is sent.
|
||
- **Dedupe.**
|
||
- Day-precision dates match when ≤ 1 day apart; two datetimes match when ≤ 24 h apart as instants.
|
||
- It never matches an engagement already sourced by the same connection.
|
||
- On a match, it fills only fields that are empty and that no source has set.
|
||
- **Record errors.** Any record error (for example, an invalid rating) fails the run and skips removal; the other records are still applied. A persistently bad record therefore keeps the connection in `error`. That is visible, but strict.
|
||
- **Reappearance.** A removed record that comes back retires its still-pending `removed` event as `ignored`.
|
||
- **Events and counts.**
|
||
- Auto-applied field changes write one `changed` `auto_applied` event per field.
|
||
- `counts.changed` counts records whose hash changed (excluding deletions).
|
||
- `counts.removed` counts records marked removed, even unmapped ones, which get no event.
|
||
- **`ingestListens` actor.** It takes an optional 5th parameter, `actor`, for `work_history`. The default is `plugin:<source>:ingest`.
|
||
|
||
## Concerns (in docs/TODO.md)
|
||
1. `list` records (Trakt, Hardcover) are snapshotted but **not applied**. The brief gives no rules for them, so I did not invent any; this needs a ruling.
|
||
2. Accepting `removed` on an engagement de-duplicated across sources soft-deletes it even if another source still reports it.
|
||
3. Stale pending changed/conflict events for superseded values stay pending.
|
||
4. Concurrent runs of the same connection are not locked (Task 9).
|
||
5. `ingestListens` makes one lookup and one insert per listen.
|
||
|
||
## Fix round 1
|
||
|
||
**Merge.** `git merge feat/phase1` produced conflicts only in docs: ARCHITECTURE (the core map row), DECISIONS (both new entries kept, sync entry first) and TODO (both kept; my now-fixed lines dropped). Committed as `b2e6e19`.
|
||
|
||
**Commits**
|
||
- `6efb97e` fix(core): batch listen ingestion per 500
|
||
- `0604c32` fix(core): re-point source records when a per-user move merges entries
|
||
- `a003c3e` fix(core): sync review round 1: lists, supersede, dedupe conflicts, retries
|
||
|
||
**Findings**
|
||
1. **Dedupe.**
|
||
- Empty fields that no source has set are filled.
|
||
- Every other incoming field goes through `decideFields` against the duplicate, so a disagreeing value files a `conflict` or a pending `changed`.
|
||
- Exception: the date that identified the play (`matchedOn`) is skipped, because its difference is within the dedupe tolerance by definition. Otherwise every Trakt-datetime vs CSV-date pair would conflict.
|
||
2. **Merged-away records.** `markRemoved` sets `deleted_at` but neither counts nor files a removal when `merged_into_key` is set or a pending `merged` event exists. When a record is reported deleted, `merged_into_key` is kept so the merge stays visible.
|
||
3. **ID-less retries.** Right after `matchWork`, the `source_records` row is committed with `current_hash ""` and `target_entity "work"`/`target_id = workId`. A retry sees the pending hash and reuses that work. List items without IDs first reuse the user's own live private work with the same media type and title.
|
||
4. **Move-merges.** `moveEntry` (entries.ts) re-points `source_records` (`target_entity "entry"`) from the merged-away entry to the survivor in the same transaction. Engagements keep their ids when re-parented. The now-redundant re-point in `changes.ts` was removed.
|
||
5. **keep_mine test.** After keep_mine, reruns file nothing. A new value from B (65) gives a pending `changed {from: 80, to: 65}` with no new conflict, and the rating stays 80.
|
||
6. **Listens.** Per batch of 500: one `source_key IN (…)` lookup, one resolve per distinct recording (outside the transaction), then one transaction with a multi-row insert-or-ignore. The per-row pre-check is gone. The test ingests 1200 listens plus an in-call duplicate, then re-ingests idempotently.
|
||
|
||
**Controller rulings**
|
||
7. **Lists.**
|
||
- A new record creates a list (kind `list`, description `Imported from <plugin id>`); items are matched outside the transaction and mapped through `target_entity "list"`.
|
||
- On change, name and description go through the per-field rules. Ownership comes from `user_history`: name and description belong to whoever last changed them, and items belong to `"user"` once any user edit exists. A user-owned field gets a pending `changed`; items carry the whole incoming item set.
|
||
- Otherwise items are replaced with the new `lists.replaceListItems` (one history row). That is a small addition to lists.ts.
|
||
- A removed record gets a pending `removed`; accepting it calls `softDeleteList`.
|
||
8. **Multi-source removal.** Accepting `removed` on an engagement deletes this connection's `engagement_sources` row. The engagement is soft-deleted only when no source rows remain.
|
||
9. **Superseding.** A newer pending value for the same (record, field) resolves the older event as `ignored` with `payload.supersededBy = <new id>`. The same value is still never filed twice. One addition: if the source comes round to the current STTracker value, its pending events for that field are resolved `ignored` with `reason: "source_agrees"`.
|
||
|
||
**Minors**
|
||
- `Object.hasOwn(STATUS_FOR, action)`.
|
||
- Accept or keep_mine on a gone target resolves the event as `ignored` with `payload.reason = "target_gone"`. A deleted entry is followed through the user's work alias to a live survivor where one exists. `mergeTargetWork` also accepts a pending-hash record (`target_entity "work"`).
|
||
- Counts: `committed()` gathers each record's (and each removal's) counts and log in scratch state and adds them to the run only after commit.
|
||
- New test: a record-level error fails the run, skips removal, and does not count the rolled-back record.
|
||
|
||
**TDD evidence**
|
||
- RED: the new and changed tests against the pre-fix `src/` (`git stash push -- packages/core/src`) gave 9 fails: conflict/keep_mine/supersede, dedupe conflict, merged-away no removal, ID-less retry, record-error counts, move-merge, multi-source removal, target_gone, lists.
|
||
- The 1200-listen test passed on the old code too: it guards correctness, and the change is the query shape.
|
||
- GREEN:
|
||
- `bun test packages/core`: 142 pass, 0 fail.
|
||
- The same with `TEST_POSTGRES_URL=…`: 273 pass, 0 fail.
|
||
- Whole repo `bun test`: 251 pass, 0 fail.
|
||
- `bun run typecheck`: clean.
|
||
|
||
**Docs**
|
||
- DECISIONS: the sync entry was updated for superseding, committed matches and move re-pointing, dedupe conflicts, merged/multi-source removal, counts, listen batching, lists and gone targets.
|
||
- ARCHITECTURE: data-flow step 3.
|
||
- TODO: the remaining sync item is the per-connection run lock (Task 9).
|
||
|
||
**Remaining concerns**
|
||
- Ownership of list `items` turns to "user" after *any* user edit, including a rename. That follows the ruling literally.
|
||
- Superseded and "source_agrees" retirements change event status automatically, but never touch user data.
|