diff --git a/docs/plans/001-phase1-handoff.md b/docs/plans/001-phase1-handoff.md new file mode 100644 index 0000000..24d9dce --- /dev/null +++ b/docs/plans/001-phase1-handoff.md @@ -0,0 +1,181 @@ +--- +status: in-progress +--- + +# Phase 1 handoff (state as of 2026-11-02) + +Read this first if you are picking up STTracker Phase 1. It is the bridge between the original conversation with the owner and the repo. Everything the owner decided is in the spec. Everything the controller decided while executing is in `docs/plans/001-phase1-sdd/ledger.md` (search for "Ruling"). + +Read in this order: +1. `AGENTS.md` +2. this file +3. `docs/specs/001-sttracker-v1-design.md` +4. `docs/plans/001-phase1-own-your-data.md` +5. `docs/plans/001-phase1-sdd/ledger.md` + +## 1. What the owner asked for, and decided + +**The original request.** A self-hosted "meta" entertainment tracker. +- Users connect their accounts on other platforms and get everything in one place. +- They can create their own entries when no platform has the item. +- Required platforms: Hardcover, Backloggd, Trakt and Last.fm. +- Stack: Bun with SQLite or Postgres. +- After the research, prepare the repo with the "preparing repos for agents" skill and push it to `https://gitea.lucaswinther.info/lucasw89/STTracker.git`. + +**Why the owner wants it** (their own words, summarised): +- Detect entries that were deleted or merged without their knowledge. +- Get backups that platforms don't offer. +- Move data between services that have no link to each other. +- Let an AI help convert data between services. +- Log things quickly without needing a "real" ID such as an ISBN or a TMDB id, and fix them later. +- Have the same features for every kind of media, instead of a separate "Trakt for X" per media type. +- Get cross-media recommendations and lists, e.g. Leitner → Magnus Archives → Voices of the Void → Twilight Zone. +- Have a personal, shareable page. +- **The app must put the user in control.** Users who keep a spreadsheet or Obsidian/Joplin notes must not have to conform to the data. Examples they gave: + - screenshots on games; + - where they bought something; + - rating a whole series as well as each book. + +**Owner decisions** (all are already in the spec): +- **Audience:** the owner first, then a few friends, maybe a small public instance later. Social features are a nice-to-have, not now. +- **Backloggd:** wait for its official CSV export. Do not scrape it (it has no API and sits behind Anubis bot protection). +- **Music:** ListenBrainz is primary, with Libre.fm as a mirror and Last.fm as a read-only archive. +- **Sources are plugins,** so they can be fixed or added without big core changes. +- **Owned library:** a private "owned library" (Steam, Jellyfin and so on) that never shows on the profile. +- **Other apps plug in later:** Jellyfin, Kavita and Calibre-Web, through STTracker's own API, app tokens and OAuth. Phase 3 ingest: webhooks, a ListenBrainz-compatible endpoint and kosync. +- **Stack:** React plus Docker. +- **Look:** + - The app is "Hardcover-style". + - The public page is "scrapbook + river". + - "Shelf" and "card catalogue" are parked as future themes. + - Mockups: `docs/design/mockups/`. +- **Conflicts:** when two sources disagree, ask the user via the Changes inbox. The same play from two sources is merged. +- **Write-back:** Phase 1 is read-only. Write-back is opt-in per connection, off by default, and comes in Phase 2. +- **Execution:** "Use subagent drive, and use multiple agents at once." +- **The owner's global preference** (`~/.claude/CLAUDE.md`): once a design is approved, carry it through spec → plan → execution with no extra review stops, and report decisions at the end. + +## 2. Where the code is + +- **`main`** (on Gitea): only the bootstrap commit (560c27c). Nothing else has been pushed. +- **`feat/phase1`** (local only, **not pushed**) @ 42fea7d contains Tasks 1–16, all reviewed, fixed and merged: + - db layer, schema, migrations 0001–0003; + - plugin SDK; + - auth; + - works and matching; + - user layer (entries, engagements, history, lists, copies, custom fields); + - attachments and crypto; + - all six plugins (Trakt, Hardcover, MangaBaka, CSV/JSON, ListenBrainz, Last.fm); + - sync engine and Changes inbox; + - jobs, scheduler, registry and connections; + - export, restore and backups; + - the server API. +- **Last full verification on `feat/phase1`:** 520 tests pass on SQLite+Postgres, and typecheck is clean. + +**Open work** (each item is a branch with a worktree under `.worktrees/`): + +| Task | Branch @ head | State | Next step | +|---|---|---|---| +| 17 web shell, library, entry detail | `task/17` @ 9546fa1 | Fix round 3 done. Its re-review died on a usage limit before finishing. | Re-run the scoped re-review of 4608c97..9546fa1 (review-draft sync; see ledger). If clean, merge into feat/phase1. | +| 18 Changes, Sources, Log, Lists, Owned, Settings, About | `task/18` @ bbd04a5 | Partly done. Committed: core/server log feed, change titles, copy work titles, the plugins device flag, the library cache fix, and the Changes, Sources, Log and Lists screens. **Not done:** Owned, Settings (prefs, custom fields, tokens, invites, reload, export/import), About, the `describeChange`/`scheduleLabel` tests, the CDP check and docs. The implementer stopped on a usage limit. | Dispatch a fresh implementer with `task-18` brief + this table + "continue from bbd04a5". Then review it. It also must `git merge task/17` again after Task 17 is final. | +| 19 Docker, compose, CI, docs | `task/19` @ 0a54020 | Barely started. Only an **uncommitted** DECISIONS.md reorder exists in `.worktrees/t19`. | Dispatch a fresh implementer (brief + the Task 19 rulings in the ledger). Base it on feat/phase1 after 17/18 land, or merge them in later. The uncommitted DECISIONS edit can be kept or redone. | + +After 17, 18 and 19 are merged, follow the plan's Completion section: +1. Full suite on both DBs, plus typecheck. +2. A whole-branch final review on the most capable model, triaging the deferred minors in the ledger. +3. `git rm` the plan, this handoff and `docs/plans/001-phase1-sdd/`. +4. Mark the spec `implemented`. +5. Merge to `main` via superpowers:finishing-a-development-branch. +6. Push only with owner approval. + +Branches `task/4`…`task/16` are merged and can be deleted. The `.worktrees/t8, t9, t15, t16` worktrees are merged too and can be removed with `git worktree remove`. + +## 3. How the work is being run (for the next controller) + +- **Skill:** superpowers:subagent-driven-development, with **parallel** implementers. The owner asked for this, overriding the skill's one-at-a-time default. +- **Worktrees:** the controller creates them itself: + + ```bash + git worktree add .worktrees/tN -b task/N feat/phase1 + cd .worktrees/tN && bun install --frozen-lockfile + ``` + + **Do not** use the Agent tool's `isolation: "worktree"`. It branches from `main`, not `feat/phase1`. +- **Workspace scripts:** + - SDD workspace and briefs: `bash /skills/subagent-driven-development/scripts/{sdd-workspace,task-brief,review-package}`. + - The ignored workspace is `.superpowers/sdd/001-phase1-own-your-data/`. A copy of the ledger, rulings, fix findings and implementer reports is in `docs/plans/001-phase1-sdd/`, because `.superpowers/` is gitignored. +- **Dispatch pattern:** + - Implementers read `docs/plans/001-phase1-sdd/common-implementer.md`, their brief and any `task-N-rulings.md`. + - Reviewers read `common-reviewer.md`. + - Every fix round gets a scoped re-review. + - Merge with `git merge --no-ff task/N`, then run the full suite on both DBs. + - **The owner has interrupted merge commands twice, then replied "Approved."** Expect a permission prompt on merges into `feat/phase1`. +- **Test Postgres:** the docker container `sttracker-pg-test` on `127.0.0.1:55432` (`docker start sttracker-pg-test` if it is stopped). + + ```bash + TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres bun test + ``` + +- **Model choice that worked:** + - Opus for core, engine, server and web implementers and their reviewers. + - Sonnet for plugins and small fixes. + - Haiku only for tiny re-reviews. + +## 4. Pending owner decisions (surface these, don't work around them) + +1. **Agent permission files.** Task 16's implementer tried to add verified commands to `.claude/settings.json` and `.gemini/settings.json`, and the harness **denied** it as agent self-modification. Nobody has worked around this. + - The wanted change is in `docs/TODO.md`: allow `bun run typecheck`, `bun apps/server/src/main.ts` and `bun --hot apps/server/src/main.ts`. + - `docs/PERMISSIONS.md` and the AGENTS.md Commands section are waiting on the same decision. + - The owner should either make the edit, or approve an agent making it. +2. **Pushing `feat/phase1`** to Gitea, and the final merge to `main`. + +## 5. Key technical decisions made during execution + +The full list is in the ledger and `docs/DECISIONS.md`. The ones a newcomer would otherwise trip on: + +- **Database access:** + - A single Kysely dialect over `Bun.SQL` serves both `sqlite://` and `postgres://`. + - SQLite runs on one connection behind a mutex, so **inside a transaction always use `tx`, never `db.k`**, or it deadlocks. + - IDs are UUIDv7. JSON is stored as text. +- **External-ID namespaces are type-specific** (`trakt_movie`, `tvdb_episode`, `anilist_manga`, …). Private works can't carry external IDs. +- **`field_sources`:** + - It records who owns each field. Any non-`plugin:` actor is stored as `"user"`; history keeps the full actor (`token:`). + - A sync only applies a field it owns. Otherwise it files a pending `changed` or `conflict` event. + - Stale pending events are superseded. + - Plugin actors never revive deletes. +- **Hierarchy:** + - Creating a child entry also ensures the root ancestor has an entry. + - Rollups and drill-down work to depth 3, skipping untracked intermediates. +- **Lists** from sources are applied. A list becomes user-owned after a user edit. +- **Secrets:** + - Secret config is stored only in encrypted `credentials_enc` (AES-GCM, key from `STTRACKER_SECRET`). + - Rate limits are declared on the plugin manifest (`rateLimit`). +- **Jobs:** + - The queue lives in the DB, with an advisory-locked enqueue, an atomic claim, a heartbeat and a stale-reaper. + - Pre-run failures (bad secret, missing plugin) are non-retryable and visible. +- **API:** + - Cookie sessions plus app tokens. + - CSRF uses the `X-Requested-With: sttracker` header on every non-Bearer write, including login. + - JSON bodies require `content-type: application/json`. +- **Export:** + - The format is `sttracker-export/v1`: a per-user zip with JSONL files, including deleted rows, aliases and tombstones, and no secrets. + - Restore goes into an empty account only. + - Backups are daily, with 0600 files. + +## 6. Not in Phase 1 (from the spec) + +The following come later: +- the public page (scrapbook + river); +- Markdown vault sync; +- Steam and Jellyfin owned-library connectors; +- IGDB, TMDB and OpenLibrary lookups; +- Libre.fm; +- write-back and migration; +- the ingest endpoints; +- the Kavita, Audiobookshelf, Komga and Calibre-Web connectors; +- Backloggd CSV import; +- an MCP server; +- themes; +- an OAuth provider; +- social features. + +Deferred minors from every task review are in the ledger, with "minor (deferred)" lines, and in `docs/TODO.md`. diff --git a/docs/plans/001-phase1-sdd/common-implementer.md b/docs/plans/001-phase1-sdd/common-implementer.md new file mode 100644 index 0000000..52f7fad --- /dev/null +++ b/docs/plans/001-phase1-sdd/common-implementer.md @@ -0,0 +1,58 @@ +# Common instructions for every STTracker implementer (binding) + +You implement one task of the STTracker Phase 1 plan. STTracker is a Bun 1.3 + TypeScript self-hosted media tracker. + +## Read first +1. Your task brief, whose path is given in your dispatch. It is your requirements; use its exact values verbatim. +2. `AGENTS.md` at the repo root (rules). +3. The "Global constraints" section of `docs/plans/001-phase1-own-your-data.md`. Read only that section, not the whole plan. +4. Only the spec sections your brief cites: `docs/specs/001-sttracker-v1-design.md`. + +## Workspace +- If you were started in a git worktree, work there. First run `bun install --frozen-lockfile`. Never modify `package.json` or `bun.lock`: every approved dependency is already installed. If you truly need another one, report NEEDS_CONTEXT. +- If not in a worktree, work in `/home/user/STTracker` on branch `feat/phase1`. +- Never commit to `main`. Commit messages use Conventional Commits and end with the line: + `Co-Authored-By: Claude Opus 5.5 ` + +## Facts from earlier tasks +- `@sttracker/db/db`: `createDb(url)` → `Db { k: Kysely; dialect: "sqlite"|"postgres"; close() }`. +- `@sttracker/db/ids`: `newId()` (UUIDv7) and `nowIso()`. +- `@sttracker/db/json`: `toJson()` and `fromJson(s, fallback)`. +- `@sttracker/db/migrate`: `migrateToLatest(db)`. +- `@sttracker/db/testing`: `forEachDialect(name, getDb => …)` gives each test a fresh, migrated DB. +- `@sttracker/core/errors`: `AuthError(code)`, `ValidationError(field, reason)` (code "validation"), `CryptoError`, `ImportError(code)`, `NotFoundError(what)` (code "not_found"). +- Kysely 0.29: import `Migration`/`Migrator` from `kysely/migration`. +- **Inside a transaction always use the `tx` handle, never `db.k`.** SQLite runs on one connection behind a mutex, so calling `db.k` inside a transaction deadlocks. +- Packages import each other by path (`@sttracker/core/entries`). There are no barrel `index.ts` files. + +## Testing +- TDD: failing test first, watch it fail (RED), minimal code, watch it pass (GREEN). +- DB tests must pass on both databases: + - `bun test ` (SQLite); + - `TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres bun test ` (Postgres; the server is running). +- Tests never touch the network. +- `bun run typecheck` must be clean before your final commit. +- Output must be pristine (no stray warnings or logs). + +## You do not dispatch subagents +Do all the work yourself. Never spawn helpers or reviewers. Review happens after you report. + +## If stuck +Report BLOCKED or NEEDS_CONTEXT with specifics. Bad work is worse than no work. If something in the brief contradicts these instructions or the spec, ask rather than guess. + +## Report +Write the full report to the report path given in your dispatch. Include: +- what you built; +- files changed; +- TDD evidence: the RED command and output, and the GREEN command and output, on both DBs where relevant; +- typecheck output; +- self-review findings; +- concerns. + +Your final reply must be ONLY (under 15 lines): +- **Status:** DONE | DONE_WITH_CONCERNS | BLOCKED | NEEDS_CONTEXT; +- commits (short SHA + subject); +- the branch name you committed on; +- a one-line test summary; +- concerns; +- the report path. diff --git a/docs/plans/001-phase1-sdd/common-reviewer.md b/docs/plans/001-phase1-sdd/common-reviewer.md new file mode 100644 index 0000000..0a89f13 --- /dev/null +++ b/docs/plans/001-phase1-sdd/common-reviewer.md @@ -0,0 +1,67 @@ +# Common instructions for every STTracker task reviewer (binding) + +You are reviewing one task's implementation: first whether it matches its requirements, then whether it is well-built. This is a task-scoped gate, not a merge review. A broad whole-branch review happens separately once every task is complete. + +## Inputs (paths given in your dispatch) +- **Task brief:** what was requested. +- **Controller rulings** in your dispatch: binding additions or changes to the brief. +- **Implementer report:** unverified claims. Verify them against the diff. Design rationales are claims too and never downgrade a finding. +- **Diff file:** read it once. It holds the commit list, the stat, and the full diff with context. Do not crawl the broader codebase. Inspect code outside the diff only to evaluate a concrete risk you can name, and name both the risk and what you checked. + +Your review is read-only. Do not mutate the working tree, the index, HEAD, or branches. Do not dispatch subagents. + +## Global constraints (from the plan) +- Bun-native only: `bun:sqlite` / `Bun.sql`, `bun test`. No `pg`, `postgres.js`, `better-sqlite3`, Express or Vite. +- IDs come from `newId()` (UUIDv7). Timestamps are ISO-8601 UTC text. JSON columns are text, via `toJson()`/`fromJson()`. +- Ratings are integers 0–100 or null. +- Partial dates are `{ value, precision: "day"|"month"|"year"|"unknown" }`. They are never converted through `Date` in a way that shifts them by timezone. +- `Status` = `planned | in_progress | paused | completed | dropped | none`. +- Packages import each other by path (`@sttracker/core/x`). No barrel `index.ts`. +- Plugins import only `@sttracker/plugin-sdk` and never touch the DB. +- Syncs never overwrite user-edited fields (`field_sources`) and never hard-delete user data. +- DB tests use `forEachDialect` and must pass on SQLite and Postgres. +- Tests never call the network. +- Runtime deps limited to: kysely, zod, fflate, react, react-dom, wouter, @tanstack/react-query. +- Inside a transaction, code must use `tx`, never `db.k`: SQLite runs one connection behind a mutex, so `db.k` inside a transaction deadlocks. + +## Tests +The implementer already ran the tests and reported results. Do not re-run suites. Run one focused test only if a specific doubt requires it. Warnings or noise in the reported output are findings. + +## Part 1: spec compliance +Look for three kinds of problem: +- **Missing:** requirements skipped or claimed without being implemented. +- **Extra:** unrequested features or over-engineering. +- **Misunderstood:** the right feature built the wrong way. + +If something cannot be verified from the diff alone, report it as ⚠️. + +## Part 2: quality +Check: +- separation of concerns; +- error handling, with no swallowed errors; +- DRY; +- edge cases; +- whether tests verify real behaviour, rather than asserting nothing; +- whether file structure matches the plan. + +Cite file:line for every finding. + +## Calibration +- **Critical:** must fix before anything else. +- **Important:** the task cannot be trusted until this is fixed (incorrect or fragile behaviour, a missed requirement, duplicated logic, tests that assert nothing). +- **Minor:** polish, or broader coverage. + +If the brief mandates something that is a defect, report it as Important and label it plan-mandated. + +## Output (begin directly with the verdict; no preamble) +### Spec Compliance +- ✅ / ❌ with file:line +- ⚠️ Cannot verify from diff +### Strengths +### Issues +#### Critical +#### Important +#### Minor +### Assessment +**Task quality:** Approved | Needs fixes +**Reasoning:** 1–2 sentences. diff --git a/docs/plans/001-phase1-sdd/ledger.md b/docs/plans/001-phase1-sdd/ledger.md new file mode 100644 index 0000000..3e42e25 --- /dev/null +++ b/docs/plans/001-phase1-sdd/ledger.md @@ -0,0 +1,136 @@ +# SDD ledger — plan: docs/plans/001-phase1-own-your-data.md + +Spec: docs/specs/001-sttracker-v1-design.md (approved). Branch: feat/phase1. Owner asked for parallel agents → parallel tasks run in isolated worktrees, merged into feat/phase1 after review. + +## Pre-flight scan + +| Tasks | Shared file / interface | Finding | +|---|---|---| +| T1 ↔ T3 | packages/plugin-sdk/package.json (T1 creates stubs; T3 needs it) | conflict if parallel | +| T1 ↔ T10–T14 | plugins/*/package.json + bun.lock (new workspace members) | lock conflicts if plugins create their own | +| T1 ↔ T2 | packages/db/src/schema.ts placeholder → real | sequential, consistent | +| T3 ↔ T13 | plugin-sdk/src/csv.ts created by T13 | T13 after T3, OK | +| T4 ↔ T5/T6/T7/T15 | AuthError/ValidationError/CryptoError/ImportError used across parallel tasks | no task owns errors.ts | +| T5 ↔ T6 | acceptSuggestion (T5) calls moveEntryToWork (T6), parallel | cross-dependency in a parallel wave | +| T8 ↔ T9 | syncHandler (T9) calls runSync (T8); plan puts both in wave D | T9 must follow T8 | +| T13 ↔ T15 | toCsv from @sttracker/plugin-sdk/csv | T13 in earlier wave, OK | +| T16 ↔ T17 | server serves apps/web/index.html, created by T17 | T16 boot/tests need a placeholder | +| T17 ↔ T18 | api.ts, theme.css, layout | T18 after T17, as the plan says | +| T3 self | rate-limit test needs a fake clock; signature has only `sleep` | needs a `now` option | +| T14 self | Last.fm error 29 comes in the JSON body, but the http client retries by HTTP status | mechanism unclear | +| T2, T4–T7, T10–T13, T15–T19 self | tests vs code vs files | consistent | + +## Rulings (pre-flight) +- Ruling: Run T1 alone first, then T2 ∥ T3. Why: T1 owns all package.json stubs. Cost if wrong: a few minutes of lost parallelism. +- Ruling: T1 also creates package.json stubs for plugins/{trakt,hardcover,mangabaka,csv,listenbrainz,lastfm} (dep `@sttracker/plugin-sdk: workspace:*`; listenbrainz also `fflate`) and runs `bun install` once. Plugin tasks never touch package.json or bun.lock. Why: avoids lockfile merge conflicts between parallel worktrees. Cost if wrong: a plugin needing another dep asks first. +- Ruling: T1 creates `packages/core/src/errors.ts` with `AuthError(code)`, `ValidationError(field, reason)`, `CryptoError`, `ImportError(code)`, `NotFoundError`, each with a `code` property. Why: shared by parallel tasks. Cost if wrong: trivial. +- Ruling: `acceptSuggestion`/`rejectSuggestion` move from T5 to T6 (`entries.ts`). Why: they depend on `moveEntryToWork`, which removes the T5↔T6 cycle. Cost if wrong: none functionally. +- Ruling: Wave D becomes T8 ∥ T15, then T9. Why: T9's syncHandler consumes runSync. Cost if wrong: serial time. +- Ruling: T16 creates a minimal `apps/web/index.html` placeholder if absent; T17 replaces it. Why: the server must boot before the web app exists. +- Ruling: Wave F becomes T17 ∥ T19, then T18. +- Ruling: T3 `createHttpClient` also accepts `now?: () => number` for testable rate limiting. +- Ruling: T14 Last.fm reads error 29 from the JSON body and retries itself up to 3 times, sleeping through the client's backoff (an injectable `sleep` option on the plugin's internal helper). Why: the API returns error 29 in the body. Cost if wrong: duplicated retry logic in one plugin. +- Ruling: Every worktree task must run `bun install --frozen-lockfile` first and run DB tests with `TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres` as well as without it. + +## Tasks +Task 1: minor (deferred): no numAffectedRows regression test; SQLite has no busy_timeout; a crashed run leaks pg test DBs; insertId guard needs a comment +Ruling: Every later dispatch must say "inside a transaction always use `tx`, never `db.k`, because the SQLite single-connection mutex deadlocks". Why: the Task 1 design (bun:sqlite has no reserve). Cost if wrong: hangs in tests, caught quickly. +Ruling: AGENTS.md "ULIDs" → "UUIDv7" is folded into Task 2's dispatch. +Task 1: complete (commits fb4ff36..27ec24e, review clean; ⚠️ pg verified by controller run) +Ruling: The Agent tool's `isolation: worktree` branches from main, not feat/phase1. From Wave C on, the controller creates worktrees itself (`git worktree add .worktrees/tN -b task/N feat/phase1`) and dispatches without isolation, pointing the agent at that path. T3's branch was rebased onto feat/phase1 (27ec24e → a14c4e7; tests 24/24 still pass). T2 was messaged to rebase. Cost if wrong: none. +Task 3: minor (deferred): burst/concurrency untested; datetime partial dates not normalized (string compare across offsets); no cap on Retry-After / no abort signal; 501/505 retryable; ratePerMinute 0 = unlimited; fixture key trailing-slash; seq helper; createTestContext.now uses the real clock +Task 3: fix round 1 dispatched (2 Important: test ctx network fallback; HttpError leaks query) +Task 3: fix round 1/5 (2 addressed, 0 open; commits a14c4e7..06a59e1) +Task 3: minor (deferred): HttpError.body may echo secrets; createHttpClient itself still defaults to global fetch (intended for prod); fixture error includes query +Task 3: complete (commits 27ec24e..06a59e1, review clean) — merged into feat/phase1 +Wave C plugins dispatched: T10 trakt, T11 hardcover, T12 mangabaka, T13 csv, T14 listenbrainz+lastfm in .worktrees/t10..t14 (branches task/10..14, base 06a59e1) +Task 2: minor (deferred): untyped Kind DSL in migration; redundant prefix indexes (brief-mandated); pg integer size cap 2GB and float4 score; schema test exercises few tables +Task 2: fix round 1 dispatched (down migration; template-DB pg harness + 30s hooks, a controller-confirmed flake; EntryStatus union; missing report) +Ruling: Plugin rate limits are declared by an optional `rateLimit?: { perMinute: number; burst?: number }` added to `PluginManifest` in Task 9. Task 9 wires it into makePluginContext's http client and sets it on each plugin manifest (trakt 100, hardcover 50, mangabaka 150, lastfm 240; listenbrainz relies on its headers, so 60). Until then plugins keep local constants. Why: the host builds ctx.http, and parallel plugin worktrees must not edit SDK types. Cost if wrong: a small follow-up edit. +Ruling: Config fields of type "secret" are persisted only inside encrypted `credentials_enc`, never in plain `config`. Task 9's makePluginContext merges them back into `ctx.config` (decrypted) and also exposes them via `ctx.credentials.get()`, so plugins may read secrets from either place. Why: plugins were written both ways in parallel, and secrets must not sit in plaintext. Cost if wrong: one adapter function. +Task 11: review: Approved with 1 Important (lookup untested) → fix round 1 dispatched. Minors deferred: merge negative test re-pulls; only book_series[0] used; duplicated max-cursor loop; GraphQL errors thrown as status-200 HttpError +Task 12: review: Needs fixes (upsert without entry → deletion) → fix round 1 dispatched (+ hasOwn, key cross-check test). Minors deferred: cursor re-saved when unchanged; fake work hint on deletes; stale progress record when progress cleared; lookup casts through mapEntry; all non-comic types become manga +Ruling: External-ID namespaces must be type-specific wherever an id space is shared across types: `trakt_movie`, `trakt_show`, `trakt_season`, `trakt_episode`; `tvdb_show`, `tvdb_episode`; `tmdb_movie`, `tmdb_show` (already); `imdb` stays shared (tt ids are global). Why: external_ids is unique on (namespace, value), so movie 1 and show 1 would collide. Cost if wrong: re-mapping namespaces in one plugin. +Ruling (ext): MangaBaka namespaces anilist_manga, mal_manga, kitsu_manga, mangaupdates, mangabaka — sent to the T12 fix round. Task 5 (matching) must not assume namespace names. +Task 13: review Needs fixes. Ruling: amend the plan-mandated CSV key formula with a per-file occurrence suffix `#n` — the plan's formula collides on duplicate rows and loses data (spec: never lose user data) — cost if wrong: keys change for duplicate rows only. Fix round 1 dispatched (keys, per-file mediaType, rating-0 only at scale 5). +Task 13: minor (deferred): JSON parsed twice; nested JSON stringified as [object Object]; titleless rows skipped silently; unknown status → none; toCsv([[]]) does not round-trip; weak test assertions; no tests for an empty final field or a lone CR +Task 14: review Approved, but ⚠️ LB /listens ordering confirmed as a real data-loss risk by the controller. Ruling: pullChanges walks backwards with max_ts (documented semantics), stops at the cursor, and dedupes the boundary second via idempotent keys — why: the min_ts ordering is unverified — cost if wrong: extra requests only. Fix round 1 dispatched (+ nowplaying test). +Task 14: minor (deferred): export chosen by `created`, not the POST export_id; options.ts duplicated, with global mutable sleep; Last.fm cursor set before loved(); no RED run (process) +Task 11: fix round 1 done (26d021a) → scoped re-review +Task 10: review Needs fixes (namespaces per ruling; season/episode ids). Fix round 1 dispatched (+ failed-refresh test). +Task 10: minor (deferred): no unparsable-expiresAt test; ctxRef declared late; duplicated full-pull sequence; 429 during device poll doesn't slow down; lists category extra (accepted). TDD process gaps (no RED run) in T10/T11/T12/T14 noted for the final review — tests themselves are behavioural. +Task 2: fix round 1/5 (3 addressed, 0 open; commits da04adb..ff595c1) +Task 2: complete (commits 27ec24e..ff595c1, review clean) — merged into feat/phase1; pg verified by the controller +Task 11: fix round 1/5 (1 addressed; commits c099680..26d021a) — complete (06a59e1..26d021a, review clean), merged +Task 12: fix round 1/5 (4 addressed; commits ddd9389..2573c83) — complete (06a59e1..2573c83, review clean), merged +Task 10: fix round 1/5 (3 addressed; 59ccc90..9708fb5) — complete (06a59e1..9708fb5, review clean), merged +Task 13: fix round 1/5 (3 addressed; a540c7d..4c08c16) — complete (06a59e1..4c08c16, review clean), merged +Task 14: fix round 1/5 (2 addressed; 3eec2f2..0074293) — complete (06a59e1..0074293, review clean), merged +Plugins minor (deferred): season-type trakt_season untested; a CSV with only zero ratings → scale 5; LB >1000 listens in one second skipped; LB one-second fallback untested +Task 7: review Needs fixes (store/unlink race; non-atomic write) → fix round 1 dispatched. Ruling: exclude image/svg+xml and normalize mimes — why: SVG can carry script, and this is cheap — cost if wrong: users can't attach SVGs. +Task 7: minor (deferred): storeAttachment doesn't verify link.entityId ownership — must be enforced in the API layer (Task 16) +Task 4: review Needs fixes (first-user admin race on pg) → fix round 1 dispatched (+ invite-before-username, dummy-hash timing, hash/revoke tests). Minor deferred: Uint8Array.toBase64 needs recent Bun. +Task 4: fix round 1/5 (4 addressed; e1faa74..fe3c9e4) — complete (d392a65..fe3c9e4, review clean), merged after owner approval; pg verified by the controller +Task 5: review Needs fixes (3 Important: concurrent-create crash/duplicates; merge orphans children; ID match can return another user's private work). Ruling: add migration 0002 with a unique works(parent_id, position) index, a retry-on-unique-violation in matchWork, and on-conflict-do-nothing for external_ids — why: correctness under concurrent syncs on pg — cost if wrong: one extra migration. Ruling: private works can't carry external IDs. Fix round 1 dispatched. +Task 5: minor (deferred): candidate LIKE misses diacritics (SQLite lower() is ASCII-only); conflicting hint IDs resolved by key order, not logged; ID-less drafts not deduplicated across syncs (Task 8 must map records via source_records to avoid re-matching) + +## PAUSED 2026-10-30 (owner asked to wind down until the usage limit resets) +Resume state: +- feat/phase1 @ c7effb5 contains Tasks 1, 2, 3, 4, 10, 11, 12, 13, 14 (all reviewed and merged). Local only, not pushed. +- Task 5: worktree .worktrees/t5 @ a46fb60, review Needs fixes. Fix round 1 was dispatched, but the agent was killed before committing anything (clean tree). On resume, dispatch a FRESH implementer with task-5-brief, task-5-report and the fix-round-1 findings (3 Important + minors, see the "Task 5: review" ledger line and rulings above). +- Task 6: worktree .worktrees/t6 @ 56b86e2 (custom-fields committed). Uncommitted WIP: src/history.ts, test/entries.test.ts, test/history.test.ts. entries/engagements/lists/copies/suggestions are not done. On resume, dispatch a fresh implementer (opus) to continue from the WIP; no report file exists yet. +- Task 7: worktree .worktrees/t7 @ 8f58ba4, fix round 1 dispatched but nothing committed (clean tree). On resume, dispatch a fresh implementer with the fix-round-1 findings (race mutex, atomic write, no SVG, tests). +- Next waves after 5/6/7: T8 ∥ T15, then T9, T16, T17 ∥ T19, T18, final review. +- The Postgres test container sttracker-pg-test (127.0.0.1:55432) is still running. + +## RESUMED 2026-10-30 (owner: "Continue") +The old agents could not be resumed (stopped). Fresh dispatches: T6 continuation (opus, from WIP), T5 fix round 1 (opus), T7 fix round 1 (sonnet). +Task 7: fix round 1/5 (4 addressed; 8f58ba4..4acb368) — complete (d392a65..4acb368, review clean), merged; pg verified by the controller +Task 7: minor (deferred): concurrency test is weak (ordering not forced); orphan file if the DB insert fails after the write; failure-cleanup path untested +Task 5: fix round 1/5 (all addressed; a46fb60..437ca14); re-review found a new Important (a tombstone occupies its position slot → sync failure) → fix round 2 dispatched +Task 5: minor (deferred): title/year parent reuse has no unique key (in TODO); legacy private works carrying ext IDs burn retries +Task 5: fix round 2/5 (1 addressed; 437ca14..50c115b) — complete (d392a65..50c115b, review clean), merged +Task 5: minor (deferred): finalWorkId silently returns the tombstone id on null; DECISIONS entry for "position lookup follows tombstones" missing (Task 19 docs pass should add it); hint display fields overwrite the surviving work (consistent with the shared metadata cache design) +Ruling: Hierarchy visibility. +- (a) Task 8's sync engine ensures that whenever it creates an entry on a child work (season/episode/track…), the user also has an entry on the ROOT ancestor work (status "none" if newly created, actor = plugin). Why: spec §6.3 "How deep imports go" says the show gets an entry too. +- (b) T6's childSummary counts all descendant entries up to depth 3 (child, grandchild, great-grandchild via work parent_id joins), not just direct children. +- (c) T6's listEntries `parentId: null` default lists entries whose work has no ancestor (up to depth 3) on which this user has an entry, so orphaned child entries (e.g. manual quick adds, or old data) still appear. +Why: otherwise imported episodes disappear from the library. Cost if wrong: a little extra query complexity. +Task 6: review Needs fixes (9 Important) → fix round 1 dispatched via task-6-fix1-findings.md. Rulings in that file: merging review/notes appends instead of dropping; an undo removes the alias; combined field_sources keep "user" if either side was user-owned. +Task 6: minor (deferred): non-ASCII tag/q filters case-sensitive on SQLite; search/sort ignore override titles; plugin upsert revives a soft-deleted entry — Ruling: Task 8's change engine must not revive soft-deleted entries for plugin actors (removed/deleted entries stay deleted unless the user restores them); equal-value patches re-stamp field_sources — Task 8 must only send changed fields; history↔entries import cycle; duplication of T5 helpers (resolveVisibleWork, insertPrivateWork) to consolidate later; mutate bumps updated_at on no-op; pagination rating-tie tests +Task 6: fix round 1/5 (9 + 3 minors addressed; 4c65939..00fc018). New Important: rollup vs drill-down mismatch → fix round 2 dispatched (+ restore onto a deleted entry throws). +Task 6: parked/minor: an undo doesn't move copies/list items back or restore re-pointed aliases (in TODO); restoring a merge row swaps in the other rating (in DECISIONS); restoring a merged-away source keeps the D→S alias. +Ruling: DECISIONS.md order is newest-first (per its header). Task 19's docs pass moves Task 5's bottom entry into order. Cost: cosmetic. +Task 6: fix round 2/5 (2 addressed; 00fc018..c6865b2) — complete (d392a65..c6865b2, review clean), merged. Full suite: 310 pass on sqlite+pg. +Task 6: minor (deferred): the show count includes season + episodes while the drill-down lists only the season (per ruling) +Wave D dispatched: T8 (opus), T15 (sonnet). +Task 15: review Needs fixes (5 Important: aliases/tombstones dropped; attachments after commit; non-transactional export; thin round-trip tests; docs) → fix round 1 via task-15-fix1-findings.md. Rulings: pre-validate attachments and write files before the tx, rows inside it; repeatable-read export; 2 GB unzip cap; 0600/0700 backups with tmp+rename. Minor deferred: concurrent-import race on the empty-account check; crafted bundles can create shared works with arbitrary metadata (Phase 2: import as private unless matched). +Task 15: fix round 1/5 (all addressed; a047020..66b1558) — complete (33631ae..66b1558, review clean), merged +Task 15: minor (deferred → Task 19 docs pass): stale DECISIONS sentence about mergedIntoId; orphan blobs if the tx fails; post-commit rewrite may throw after success; attachment-id remap in history; untested rejection paths (bundle_too_large, conflict, status/visibility) +Task 8: implemented (33631ae..1975e97). Rulings on the implementer's open concerns, to be sent with the review findings: +- Ruling: `list` records ARE applied in Phase 1. New → a user list (kind "list", name = record name, description "Imported from ") with items via matchWork. Changed → item and name diffs follow the field rules (a user-edited list is not overwritten: pending changed). Removed → pending removed. Why: otherwise imported lists are invisible, against the spec goal "everything in one place". Cost: moderate extra scope. +- Ruling: accepting `removed` for an engagement with other engagement_sources only detaches this connection's source row; soft-delete happens only when no sources remain. +- Ruling: a newer incoming value for the same (record, field) supersedes the older pending event (set status "ignored", payload.supersededBy = new event id) and creates the new pending event. +- Ruling: listens are bulk-inserted in chunks of 500 with onConflict doNothing. +- Concurrent runs of the same connection → Task 9 lock (as planned). +Task 8: review Needs fixes (6 Important) → fix round 1 via task-8-fix1-findings.md (also carries the rulings on lists, multi-source removal, superseding). Minor deferred: identical rerun opens one tx per record to bump last_seen; snapshot hash edge-case tests. +Task 9 dispatched in parallel with the T8 fix round (based on feat/phase1 + task/8 WIP; runSync interface stable). Ruling: rateLimit manifest field, secret split into credentials_enc, advisory-locked enqueue, weekly full — see task-9-rulings.md. +Task 9: review Needs fixes (4 Important: error connections unscheduled; pre-runSync failures loop; unhandled rejection; reload misses non-index files) → fix round 1 via task-9-fix1-findings.md. Rulings: NonRetryableError + failed run row + clear STTRACKER_SECRET message; stale running-job reaper (1h); reload limitation documented. Minor deferred: per-plugin (not per-run) rate limiting → Phase 2; deleted connection = paused without creds (API layer must hide it). +Task 8: fix round 1/5 (9 + minors addressed; b2e6e19..a003c3e) — complete (33631ae..a003c3e, review clean), merged +Task 8: minor (deferred → TODO): stale removed/merged events follow merges to the survivor (prefer target_gone); two records → one entry after a move-merge; list-item title reuse is ASCII-only on SQLite and ignores creators; pending-hash records counted as removed; listen batch partial counts +Task 9: fix round 1/5 (4 + minors addressed except the reaper re-queuing in-flight jobs) → fix round 2 dispatched (in-flight exclusion + heartbeat; shutdown re-queue without burning an attempt). Minor deferred: a reload drops a plugin whose edited entry fails to import (keep the old instance → TODO); a log-append failure masks the original error. +Task 16 dispatched early on a task/9 base (839d969); merge feat/phase1 after T9 lands. +Task 9: fix round 2/5 (2 addressed; 839d969..f0d41f2) — complete (207c92b..f0d41f2, review clean), merged +Task 16: implemented (7f3d0d5..0a54020). Its edit to the permission files (.claude/settings.json, .gemini/settings.json) was DENIED by the harness. The controller will not work around it: surface it to the owner at the end. AGENTS.md Commands and PERMISSIONS.md are left unchanged (they must move together). +Ruling (pending review): add migration 0003 with `source_connections.deleted_at`; deleteConnection sets it instead of overloading paused+no-creds; listConnections/scheduler exclude deleted rows. Why: a paused CSV source (no secrets) would vanish. Cost: one migration. +Tasks 17 and 19 dispatched early on a task/16 base (0a54020); they merge feat/phase1 after T16 lands. T19 must not touch the agent permission files (denied earlier; owner decision). +Task 16: review Needs fixes (paused credential-less connections unreachable; token history provenance) → fix round 1 via task-16-fix1-findings.md. Rulings: migration 0003 deleted_at; non-plugin actors → field_sources 'user', with history keeping token:; CSRF header + JSON content-type on all non-Bearer writes including login; production mode by default. Minor deferred: 50 MB multipart peak memory (TODO); per-day backup lockKey (optional). +Task 16: fix round 1/5 (all addressed; 0a54020..0b34ede) — complete (7f3d0d5..0b34ede, review clean), merged after owner approval +Task 16: minor (deferred → TODO): unauthenticated writes without the header get 403, not 401; the CSRF exemption is based on Bearer presence; quickAdd/restore/acceptSuggestion/resolveChange record "user", not the token actor; a delete can race a running sync's credential save +Task 17: implemented (0a54020..439d138); not yet reviewed +Ruling (pending T17 review): add 3 small server routes in the T17 fix round — GET auth/status {hasUsers}; GET attachments?entity=&entityId= (owner-scoped listAttachments); GET/PATCH me/preferences {ratingScale, defaultVisibility} stored in users.preferences. Why: the UI otherwise fakes them (first-run guess, session-only uploads, localStorage scale). Cost: small API additions. +Task 17: review Needs fixes (4 Important: lost tag/custom-field updates; stale review undoes Restore; raw validation text; Gelasio missing) → fix round 1 via task-17-fix1-findings.md. Rulings: 3 API routes (auth/status, attachments list, me/preferences); dark text on --warn ratified (AA). T18 depends on T17's final api.ts/hooks. +Task 18 dispatched on a task/17 base (5fa5931). +Ruling (T18 scope): add GET /log (core log.ts, keyset over at/kind/id); listChanges adds title + intoTitle (batched, override-aware); listCopies adds workTitle. Why: the spec's Log and Changes views need them, and the UI-side fallbacks would be N+1 and vague. Profile PATCH, custom-field edit/delete and the invites list go to TODO. +Task 17: fix round 1/5 (all addressed); new Important: a failed review save discards typed text → fix round 2 dispatched (+ was-capture, restore in the patch scope, prefs tx, auth-status invalidation, rating copy). Minor deferred: Gelasio TTF is inlined in the CSS bundle → woff2 TODO. +Task 17: fix round 2/5 (items 2-6 addressed; item 1 partial: the saving flag can stick with overlapping saves) → fix round 3 dispatched. diff --git a/docs/plans/001-phase1-sdd/reports/task-1-report.md b/docs/plans/001-phase1-sdd/reports/task-1-report.md new file mode 100644 index 0000000..53f9632 --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-1-report.md @@ -0,0 +1,118 @@ +# Task 1 report: `packages/db` + +**Status:** DONE_WITH_CONCERNS (concerns are minor; see the end) +**Branch:** `feat/phase1`, base fb4ff36 + +## Commits +- `6c5a9fb` chore: workspace package stubs with approved dependencies +- `8248a2a` feat(db): Bun.SQL Kysely dialect for sqlite and postgres +- `27ec24e` feat(core): shared domain error classes + +## What was built +- **`packages/db/src/dialect.ts`**: + - `type Dialect = "sqlite" | "postgres"` and `dialectOf(url)`, which throws on an unknown scheme. + - `BunSqlDialect` is one Kysely `Dialect` over `new SQL(url)`. It picks the Sqlite or Postgres adapter, compiler and introspector by URL scheme. + - `executeQuery` calls `runner.unsafe(sql, params)`. It sets `numAffectedRows` only for INSERT/UPDATE/DELETE/MERGE, and `insertId` only for SQLite INSERT. + - Transactions are `begin`/`commit`/`rollback` on the held connection. On Postgres, `begin` can carry an isolation level and access mode. +- **Deviation from the brief (forced by Bun):** Bun's SQLite adapter throws `This adapter doesn't support connection reservation` on `reserve()`. + - Postgres uses `sql.reserve()` as the brief says. + - SQLite uses the single `SQL` instance as its connection, guarded by a mutex. The lock is held from `acquireConnection` to `releaseConnection`, so a transaction never interleaves with other queries. + - This is recorded in DECISIONS. +- **`db.ts`:** `createDb(url): Promise`, with `Db { k: Kysely; dialect; close() }`. + - SQLite: runs `PRAGMA foreign_keys = ON`, plus `PRAGMA journal_mode = WAL` unless the URL is `:memory:`. + - Postgres: runs `select 1` to fail fast on a bad URL. + - If init fails, the Kysely instance is destroyed before the error is rethrown. +- **`ids.ts`:** `newId()` returns `Bun.randomUUIDv7()`; `nowIso()` returns `new Date().toISOString()`. +- **`json.ts`:** `toJson` and `fromJson(s, fallback)`. The fallback is returned for null, undefined or invalid JSON. +- **`migrate.ts`:** `migrateToLatest(db)` runs a Kysely `Migrator` (imported from `kysely/migration`, as Kysely 0.29 requires) with a static provider over `migrations/index.ts`. An empty set works. It throws with the name of the failing migration. +- **`migrations/index.ts`:** `export const migrations: Record = {}`. +- **`schema.ts`:** placeholder `export interface Database {}`. +- **`testing.ts`:** `forEachDialect(name, fn)` registers `describe(" [sqlite]")`, plus `[postgres]` when `TEST_POSTGRES_URL` is set. + - `beforeEach` builds a fresh migrated DB: SQLite uses `:memory:`; Postgres runs `create database "st_"` through an admin connection and uses the derived URL. + - `afterEach` closes the DB. + - `afterAll` drops every created database `with (force)`. +- **`packages/core/src/errors.ts`:** `AuthError(code)`, `ValidationError(field, reason)` (code `validation`), `CryptoError(message)` (code `crypto`), `ImportError(code)` and `NotFoundError(what)` (code `not_found`). + - All of them extend Error and have a readonly `code` and a `name` set. +- **Package stubs:** these get name, private, type module, `exports {"./*": "./src/*.ts"}`, and the dependencies from the ruling: + - packages: db, core, plugin-sdk; + - apps: server, web; + - plugins: trakt, hardcover, mangabaka, csv, listenbrainz, lastfm. + + Versions are caret ranges of the npm latest on 2026-10-30: + - kysely ^0.29.7, zod ^4.6.7, fflate ^0.8.3; + - react and react-dom ^19.3.1, wouter ^3.13.0, @tanstack/react-query ^5.105.0; + - @types/react and @types/react-dom ^19.3.0. + + `bun install` was run and `bun.lock` committed. `bun install --frozen-lockfile` reports no changes. +- **Docs:** + - DECISIONS: three new entries, newest first (single Bun.SQL dialect; JSON as text; UUIDv7 instead of ULID). + - ARCHITECTURE: the map row for `packages/db` is updated. + +## Files changed +- `bun.lock` +- `docs/ARCHITECTURE.md`, `docs/DECISIONS.md` +- `apps/{server,web}/package.json` +- `packages/{core,db,plugin-sdk}/package.json` +- `plugins/{trakt,hardcover,mangabaka,csv,listenbrainz,lastfm}/package.json` +- `packages/db/src/{dialect,db,ids,json,migrate,schema,testing}.ts`, `packages/db/src/migrations/index.ts` +- `packages/db/test/db.test.ts` +- `packages/core/src/errors.ts` + +## TDD evidence +**Tests:** +- Inside `forEachDialect`, per dialect: + - "round-trips a row"; + - "transactions roll back"; + - "transactions commit" (extra); + - "newId is time-sortable" (1000 ids, sorted and unique). +- Outside it, once: a `fromJson` fallback test. + +RED, written before any source existed: +``` +$ bun test packages/db +error: Cannot find module '../src/ids.ts' from '/home/user/STTracker/packages/db/test/db.test.ts' + 0 pass + 1 fail + 1 error +``` +GREEN on SQLite: +``` +$ bun test packages/db + 5 pass + 0 fail + 12 expect() calls +Ran 5 tests across 1 file. [371.00ms] +``` +GREEN on SQLite and Postgres: +``` +$ TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres bun test packages/db + 9 pass + 0 fail + 20 expect() calls +Ran 9 tests across 1 file. [1.87s] +``` +**Mutation check:** I temporarily changed `rollbackTransaction` to issue `commit`. "transactions roll back" then failed on both `[sqlite]` and `[postgres]`, and I restored the code. After the Postgres runs, no `st_%` databases were left over. + +**Manual probe:** +- `createDb("sqlite://rel.db")` creates `./rel.db` with `journal_mode=wal` and `foreign_keys=1`. +- `createDb("mysql://x")` throws `Unsupported database URL scheme: mysql`. + +## Typecheck +``` +$ bun run typecheck +$ tsc --noEmit -p tsconfig.json +``` +It exits clean with no output. + +## Self-review findings +- The mutex is released through Kysely's `try/finally` in its connection provider, so a failing query cannot leave the SQLite lock held. +- `numAffectedRows` is not set for SELECTs, because Bun's `count` there is the row count. +- Postgres `count(*)` comes back as a string (bigint). The test uses `Number()`. Later tasks should do the same or cast in SQL. +- Not implemented, on purpose: savepoints (Kysely raises a clear error if they are used), and true streaming (`streamQuery` yields the whole result as one chunk). Both are recorded in DECISIONS. +- Fixed during review: one claim I could not verify, about how Bun.SQL returns `jsonb`, was removed from the DECISIONS text. + +## Concerns +1. **SQLite has no `reserve()`.** That is why it uses a single connection behind a mutex, which deviates from the brief's "acquireConnection returns `sql.reserve()`". The public interface is unchanged. +2. **Postgres test cost.** `create database` takes about 100–300 ms per test, and once took about 1 s. One run of 4 PG tests took 11 s; reruns took 1.6–3.8 s. With many DB tests in later tasks, the Postgres suite may get slow. A faster option is one DB per `describe` with a schema reset per test. That would change the harness contract, so I did not do it. +3. **ULID wording.** `AGENTS.md` (Rules: "IDs are ULIDs") still says ULID. The DECISIONS entry says to read it as UUIDv7. Editing `AGENTS.md` was outside my ruling, so the maintainer or controller may want a one-word fix. +4. **Kysely 0.29 import path.** `Migrator`, `Migration` and `MigrationProvider` must be imported from `kysely/migration`; the root exports are deprecated type errors. Task 2's migration files should import `Migration` from `kysely/migration`. diff --git a/docs/plans/001-phase1-sdd/reports/task-10-report.md b/docs/plans/001-phase1-sdd/reports/task-10-report.md new file mode 100644 index 0000000..2697d87 --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-10-report.md @@ -0,0 +1,23 @@ +# Task 10 report: Trakt plugin + +Branch task/10. Built plugins/trakt: index.ts (manifest, deviceAuth, lookup), src/api.ts (headers, token refresh, paged GET), +src/map.ts (Trakt item -> WorkHint/SourceRecord), src/sync.ts (pull, pullChanges), test/trakt.test.ts, 10 hand-written fixtures. + +Tests: `bun test plugins/trakt` -> 15 pass, 0 fail. `bun run typecheck` clean. +TDD caveat: I wrote implementation and tests in the same pass, so there is no recorded RED run. One test failed first +(a test-harness mistake: an async fixture route), fixed in the test only. + +Decisions +- Season/episode hints carry empty externalIds (numeric namespaces tvdb/tmdb/imdb/trakt are shared with shows/movies); matched by parent chain + position. +- Brief's `trakt` id namespace is shared across movie/show; collisions across types are possible in core's unique (namespace,value). Flag for matching. +- RATE_PER_MINUTE=100 exported from src/api.ts; the plugin uses ctx.http, so the host must build the client with it (SDK has no per-plugin hook). +- pullChanges cursor = JSON {history, ratings, watchlist, lists} (max timestamps). Added lists re-fetch on lists.updated_at (brief only named history/ratings/watchlist). No/invalid cursor -> full pull. Cursor set only if it changed. +- Refresh window: expiresAt within 24h or unparsable -> refresh; expiresAt = created_at+expires_in (ISO). Credentials saved before returning. +- deviceAuth.poll: HTTP 400/429 -> "pending"; other statuses throw HttpError. +- Commit trailer uses "Claude Sonnet 5.5" per the session attribution reminder, not the "Opus 5.5" in common-implementer.md. + +## Fix round 1 +- Namespaces now type-specific: movie trakt_movie/tmdb_movie/imdb; show trakt_show/tmdb_show/tvdb_show/imdb; + season trakt_season/tmdb_season (when season ids present; episode-parent seasons have none); episode trakt_episode/tvdb_episode/tmdb_episode/imdb. +- Removed the comment justifying omitted season/episode ids. Tests updated; added episode-id assertion and failed-refresh test (400 -> no saved creds, no /sync call). +- 16 tests pass, typecheck clean. This supersedes the earlier "empty externalIds" concern. diff --git a/docs/plans/001-phase1-sdd/reports/task-11-report.md b/docs/plans/001-phase1-sdd/reports/task-11-report.md new file mode 100644 index 0000000..771527a --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-11-report.md @@ -0,0 +1,9 @@ +# Task 11 report: Hardcover plugin +Built plugins/hardcover: index.ts (definePlugin; pull, pullChanges, lookup), src/graphql.ts, src/map.ts, index.test.ts, test/fixtures/{page-short,lists,errors}.json. +Commit c099680 on task/11. +TDD: tests and implementation were written together; RED was shown after the fact by removing index.ts (0 pass, 1 fail, import error). GREEN: `bun test plugins/hardcover` 9 pass, 0 fail. Typecheck clean. No DB involved. +Decisions: token read from credentials.token, falling back to config.token. user_books ordered by id for stable paging. pullChanges uses where updated_at > cursor and stores max updated_at; pull also sets the cursor. Lists only in pull. Merge record reuses the entry fields with a work hint of hardcover_book = original_book_id. GraphQL errors throw HttpError(200, body). +Concerns: ratePerMinute 50 is exported as RATE_PER_MINUTE but the SDK ctx.http is host-built, so the host must apply it. The lookup search response shape (results.hits[].document) is from memory and untested against the real API. + +## Fix round 1 +Added lookup tests (hit mapping incl. year/creators/coverUrl/externalIds, results:null -> [], non-book mediaType -> [] with no request) and a table-driven status test (1-6 and null -> none). No production code changed; the lookup shape is now pinned by fixtures only (still unverified against the live API). Result: all plugins/hardcover tests pass, typecheck clean. diff --git a/docs/plans/001-phase1-sdd/reports/task-12-report.md b/docs/plans/001-phase1-sdd/reports/task-12-report.md new file mode 100644 index 0000000..0dbca92 --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-12-report.md @@ -0,0 +1,19 @@ +# Task 12 report: MangaBaka plugin +Status: DONE_WITH_CONCERNS. Branch task/12, commit ddd9389 feat(plugins): mangabaka. +Files (all under plugins/mangabaka/): index.ts, src/{api,map,pull,lookup}.ts, test/mangabaka.test.ts, test/fixtures/{library-page1,library-page2,changes,search}.json. +Tests: bun test plugins/mangabaka -> 12 pass, 0 fail. bun run typecheck clean. No DB involved. +Concerns: +- TDD: tests and implementation were written together; no separate RED run was captured. +- Response shapes not in the brief are assumed: library page = {data, pagination:{page,total_pages}}; search = {data:[series]}. Verify against the OpenAPI spec. +- ratePerMinute 150 is exported as RATE_PER_MINUTE (src/api.ts); the SDK manifest has no field for it, so the host must apply it. +- Auth reads ctx.config.token and sends x-api-key. +- Deletes yield two deleted records (series: and progress:) with a placeholder WorkHint. +- Changes loop saves cursor each page and stops on empty data, missing or unchanged cursor. +- Series type "comic" maps to comic, otherwise manga. + +## Fix round 1 +- pullChanges: only type "delete" deletes; upsert without entry is skipped with ctx.log message (test added). +- mapState uses Object.hasOwn (test for "constructor"). +- Test cross-checking delete keys against keys from a full pull of series 1. +- 15 tests pass, typecheck clean. +- External-id namespaces made type-specific: anilist_manga, mal_manga, kitsu_manga (mangaupdates, mangabaka unchanged); tests updated. diff --git a/docs/plans/001-phase1-sdd/reports/task-13-report.md b/docs/plans/001-phase1-sdd/reports/task-13-report.md new file mode 100644 index 0000000..a79243a --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-13-report.md @@ -0,0 +1,16 @@ +# Task 13 report +Built: RFC4180 codec (parseCsv/toCsv) in packages/plugin-sdk/src/csv.ts; csv plugin (plugins/csv/index.ts, src/mapping.ts), fixtures messy.csv and goodreads.csv. +Files: packages/plugin-sdk/{src/csv.ts,test/csv.test.ts}, plugins/csv/{index.ts,src/mapping.ts,test/csv.test.ts,test/fixtures/*}. +RED: `bun test packages/plugin-sdk/test/csv.test.ts plugins/csv` -> "Cannot find module '../src/csv.ts'", 0 pass 2 fail. +GREEN: same command -> 11 pass, 0 fail. No DB involved. `bun run typecheck` clean. +Decisions/concerns: +- Added an extra mapping field `author` (needed for Goodreads Author -> creators). +- ratingScale is treated as a per-row column; absent -> file-wide default (5/10/100 by max). Rating 0 = unrated. +- Engagement key is `:engagement`. Default mediaType: book if isbn13/author column present, else other. +- Commit trailer uses Sonnet 5.5 per the session attribution reminder (common-implementer says Opus 5.5). + +## Fix round 1 +1. Row keys: per-file occurrence suffix (`row:`, then `#2`, `#3`...); engagement key inherits. Tests: duplicates distinct, re-import identical. +2. Default mediaType decided once per file (author/isbn13 in effective mapping/header, or any JSON object with those keys); row-level mediaType still overrides. Tests added. +3. Rating 0 is unrated only at effective scale 5; tests for scales 5/10/100. +Result: `bun test plugins/csv packages/plugin-sdk` all pass; typecheck clean. diff --git a/docs/plans/001-phase1-sdd/reports/task-14-report.md b/docs/plans/001-phase1-sdd/reports/task-14-report.md new file mode 100644 index 0000000..a7c9f12 --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-14-report.md @@ -0,0 +1,12 @@ +# Task 14 report +Built ListenBrainz and Last.fm plugins (index.ts, src/, src/options.ts with injectable `setSleep`, tests under test/). +Files: plugins/listenbrainz/{index.ts,src/listenbrainz.ts,src/options.ts,test/listenbrainz.test.ts}, plugins/lastfm/{index.ts,src/lastfm.ts,src/options.ts,test/lastfm.test.ts}. +Tests: `bun test plugins/listenbrainz plugins/lastfm` -> 10 pass, 0 fail. `bun run typecheck` clean. +TDD note: implementation was written before the tests, so there is no RED run. The first GREEN run had 2 failures, both mistakes in the test fixtures (wrong ISO second; a 429 was retried by the SDK instead of the plugin). Fixed the tests, not the code. +Decisions: pull also sets the cursor to max listened_at/uts; pullChanges sets it per page (LB) or at end (Last.fm). Secrets read from credentials first, then config. Feedback entries without recording_mbid are skipped. Newest export = max `created`. Last.fm error 29: 3 attempts, sleeps 2s then 4s. +Concerns: ratePerMinute 240 for Last.fm is a host-side HttpClient option; the plugin can't set it (not in manifest type), so the host must configure it. + +## Fix round 1 +- LB pullChanges rewritten to walk backwards with max_ts (no min_ts): stops on empty page or a listen <= cursor, yields only > cursor, dedupes by key in-run, max_ts = smallest+1 (falls back to smallest if no progress), cursor set once at the end to the max. New tests: 4-page backlog (3 new pages + boundary page), duplicate boundary-second dedupe, unchanged cursor yields nothing. +- Last.fm nowplaying fixture now has date.uts; verified the test fails when the skip is removed. +- 11 tests pass; typecheck clean. diff --git a/docs/plans/001-phase1-sdd/reports/task-15-report.md b/docs/plans/001-phase1-sdd/reports/task-15-report.md new file mode 100644 index 0000000..5617213 --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-15-report.md @@ -0,0 +1,23 @@ +# Task 15 report + +Built `packages/core/src/export.ts` (exportUserBundle, exportUserCsv, importUserBundle) and `backup.ts` (runBackup, backupHandler), tests in `packages/core/test/{export,backup}.test.ts`, DECISIONS.md entry (top). + +TDD: tests and code were written together, then RED shown by removing src files: `Cannot find module '../src/export.ts'`, 0 pass 2 fail. GREEN: sqlite 8 pass; with TEST_POSTGRES_URL 16 pass 0 fail. `bun run typecheck` clean. + +Decisions/concerns: +- Attachments are restored after the main transaction (storeAttachment uses its own tx; sqlite deadlock otherwise), so a failure there leaves rows without attachments. Attachment createdAt/visibility restored by update after store; extra links inserted directly. +- Attachments whose file is missing on disk are skipped at export. +- Merged-away works are exported as raw tombstone rows (mergedIntoId dropped); with no external IDs they restore as private works. +- Listens inserted with onConflict doNothing (unique user/source/source_key); account-empty check is entries only. +- runBackup `keep` optional (default 14); backupHandler returns `() => Promise`. +- Unknown/invalid zip => ImportError("unsupported_format"); inconsistent refs => ImportError("corrupt_bundle"). +- Work hierarchy sibling position uniqueness could collide on reuse edge cases (not handled). + +## Fix round 1 +- I1: mergedIntoId + work_aliases.jsonl exported/restored (tombstone + alias; test with moveEntryToWork then upsertEntry). +- I2: attachments pre-validated (mime, size, sha) -> ImportError("invalid_attachment"); files written before the tx, rows inserted in the tx; files re-written after commit. attachments.ts gained exported `writeAttachmentFile`, `sha256Hex`, `normalizeMime`, `mimeAllowed`, `MAX_ATTACHMENT_BYTES`. +- I3: export reads in one transaction (repeatable read on Postgres). +- I4: tests extended (copies, custom fields, listens, grouping list, overrides/visibility, caption on two links, history UUID remap, alias/tombstone, second user isolation, credential-key check, invalid attachment, corrupt rating). +- I5: ARCHITECTURE, TODO, DECISIONS updated. +- M3 validation + 2 GB cap (bundle_too_large); M5 0700/0600 + tmp-then-rename + 500-chunked IN; M1 unique violation -> ImportError("conflict"). +- Results: bun test packages/core 113 pass (sqlite), 218 pass (postgres); typecheck clean. diff --git a/docs/plans/001-phase1-sdd/reports/task-16-report.md b/docs/plans/001-phase1-sdd/reports/task-16-report.md new file mode 100644 index 0000000..b929d12 --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-16-report.md @@ -0,0 +1,154 @@ +# Task 16 report: `apps/server` + +**Status:** DONE_WITH_CONCERNS. The code is complete and verified. The permission and Commands docs step is blocked (see Concerns 1). +**Branch:** `task/16` (worktree `/home/user/STTracker/.worktrees/t16`). Merged `feat/phase1` (7f3d0d5, Task 9's jobs.ts fix) first, as the coordinator asked. It was a fast-forward with no conflicts. + +## Commits +- 7b0a673 feat(core): API support helpers: token list, suggestions, attachment links, connection runs, device auth, file-mode sync +- 6963e29 feat(server): REST API, auth, sources, changes, export +- 02b7eed feat(server): boot: migrate, load plugins, worker, scheduler, daily backup, web placeholder +- 348b04e docs: server architecture, API decisions, follow-ups +- 0a54020 test(server): silence the broken fixture plugin warning + +## What was built +- `config.ts`: `loadConfig(env)` returns DATABASE_URL (default `sqlite://${DATA_DIR}/sttracker.db`), DATA_DIR (`./data`), STTRACKER_SECRET (required, with the exact message), PUBLIC_URL, PORT (3000) and PLUGIN_DIRS (`plugins,${DATA_DIR}/plugins`). +- `http.ts`: `route()` (zod body and query, with types inferred), `apiHandler`, auth modes `none|session|token:*|any:*`, the `stt_session` cookie (HttpOnly, SameSite=Lax, Secure on an https PUBLIC_URL), the CSRF header `X-Requested-With: sttracker` on cookie writes, body caps (JSON 1 MB, multipart 50 MB, counted while streaming, 413 `validation`), a limit of 10 login/register attempts per minute per IP (429 `rate_limited`), and the error mapping from ruling 5. Unique violations map to 409 `conflict`. Unknown errors return 500 `internal` with a fixed message and are logged. +- `app.ts`: `createApp({db, registry, secret, dataDir, publicUrl?})` returns `{fetch, routes}`. `/ingest/*` returns a 404 JSON error. +- `routes/`: auth, entries, engagements, works, lists, copies, custom-fields, attachments, sources, changes, export and meta (openapi.json, about, admin/plugins/reload). Shared pieces live in `schemas.ts` and `form.ts`. They cover every route in the brief. +- `openapi.ts`: OpenAPI 3.1 built from the route table with `z.toJSONSchema`. It gives each route its method, path, summary, security, path and query parameters, and request body. +- `boot.ts`: `nextBackupAt` and `ensureBackupScheduled` (lockKey `backup`, runAt the next 03:00 local). +- `main.ts`: mkdir DATA_DIR/plugins, then createDb, migrate, loadPlugins, worker (sync + backup), scheduler, daily backup, then `Bun.serve` with routes `/api/*` and `/ingest/*` going to the app and `/*` going to the HTML import. SIGTERM/SIGINT run a clean shutdown. +- `apps/web/index.html`: the placeholder required by ruling 1. +- Core additions, needed because the API had nothing to call: + - `auth.listAppTokens` (metadata only). + - `entries.listSuggestions`. + - `attachments.requireLinkTarget` (ownership check per entity, run before `storeAttachment`) and `listAttachmentLinks`. + - `readAttachment` now also returns `filename`. + - In `connections`: `getConnection` (hides deleted and other users' connections), `isDeletedConnection`, `listRuns`, `deviceStart` and `devicePoll`. + - `syncHandler` file mode: `{connectionId, file:{name, base64}}` → `runSync` `{file}` mode, recorded as run mode `file`. + - `jobs.ts` was not touched. + +## Decisions worth review +- **Actor:** all API writes use actor `"user"`, including token writes. The change engine only protects fields whose source is `"user"`, so a `token:` actor would let the next sync overwrite a script's edit. +- **Cookie-only routes:** tokens, `POST/PATCH/DELETE sources`, device auth, `import/bundle` and admin need a session cookie. App tokens can't mint tokens or handle credentials. +- **Daily backup re-enqueue:** the finishing job still holds lockKey `backup`, so an immediate re-enqueue would return null. The next job is queued 5 s after each run, with an hourly idempotent safety net and once at boot. +- **Request field names:** + - `PATCH list-items/:id` takes `{position}`, which maps to `moveListItem` (core has no note edit). + - `DELETE me/tokens/:id`. + - `POST sources/:id/device/poll` takes `{deviceCode}`. + - Register requires a password of at least 8 characters. + +## TDD evidence +- RED for the server, before src existed: `bun test apps/server` gave `error: Cannot find module '../src/app.ts'`, 0 pass, 1 fail. +- RED for the core helpers: `bun test packages/core/test/api-support.test.ts` gave 0 pass, 1 fail, 1 error (missing exports). +- RED for boot: `bun test apps/server/test/boot.test.ts` gave 1 fail, 1 error (missing `boot.ts`). +- GREEN: + - `bun test apps/server`: 23 pass, 0 fail. + - The same with Postgres: 40 pass, 0 fail. + - `api-support`: 6 pass on SQLite, 12 pass on both. +- A float4 snag on Postgres: `match_suggestions.score` is `real`, so 0.9 came back as 0.8999999761581421. The test now uses 0.75. The helper itself is unaffected. +- Full suite after the merge: + - `bun test`: 307 pass, 0 fail (34 files). + - `TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres bun test`: 492 pass, 0 fail. +- Typecheck: `bun run typecheck` (`tsc --noEmit -p tsconfig.json`) is clean. +- Test output has no stray lines. + +## Boot verification (real) +``` +$ STTRACKER_SECRET=test DATA_DIR=$(mktemp -d) PORT=3999 bun apps/server/src/main.ts & +$ curl -s localhost:3999/api/v1/openapi.json | head -c 200 +{"openapi":"3.1.0","info":{"title":"STTracker API","version":"1"},"servers":[{"url":"/"}],"components":{"securitySchemes":{"session":{"type":"apiKey","in":"cookie","name":"stt_session","description":" +server log: STTracker listening on http://localhost:3999/ (sqlite, 6 plugin(s)) +kill -TERM -> "SIGTERM: shutting down", process exited; DATA_DIR contains plugins/ and sttracker.db +GET / -> placeholder HTML (bundled by Bun) +GET /api/v1/entries (no auth) -> {"error":{"code":"unauthenticated","message":"Sign in first."}} +POST /ingest/x/y -> {"error":{"code":"not_found","message":"No ingest endpoint for this plugin."}} +no STTRACKER_SECRET -> error: Set STTRACKER_SECRET to a long random string. It encrypts source credentials. +bun --hot apps/server/src/main.ts (PORT=3998): served openapi.json, exited cleanly on SIGTERM +``` + +## Concerns +1. **AGENTS.md Commands, PERMISSIONS.md and the tool permission files were not updated.** + - The auto-mode classifier denied my edit to `.claude/settings.json` and `.gemini/settings.json` as agent self-modification. I didn't work around it. + - AGENTS.md requires PERMISSIONS.md to change in the same commit as the tool files, and allows only `worked` rows in Commands. So I left all of them unchanged. + - The exact change is recorded in `docs/TODO.md` (Now). It needs a human: + - Allow `bun run typecheck`, `bun apps/server/src/main.ts` and `bun --hot apps/server/src/main.ts`. All three were verified this session. + - Add matching rows to PERMISSIONS.md, and drop the stale "typecheck failed" row. + - Set the AGENTS.md Commands: Single test `bun test `, Typecheck `bun run typecheck`, Dev `bun --hot apps/server/src/main.ts` (needs STTRACKER_SECRET), Start `bun apps/server/src/main.ts`. +2. **Hidden paused connections.** A connection the user paused that never had credentials looks deleted, by Task 9's definition, so it disappears from `GET sources`. This is noted in TODO; the fix is an explicit `deleted_at`. +3. **Limits deferred to TODO:** + - The rate limit is per process and uses the socket IP, so all clients share one key behind a proxy. + - File imports are stored as base64 in `jobs.payload`. + - OpenAPI has no response schemas. +4. **Bun FormData filename quirk.** Bun's FormData serializer does not escape `"` in filenames. The `safeFilename` sanitizer is unit-tested directly for quotes and CR/LF instead of end to end. + +--- + +# Fix round 1 + +**Status:** DONE. The agent permission files were not touched. + +## Commits +- e3a759d fix(core): soft-delete source connections with deleted_at +- aa8e4db fix(core): token writes keep their actor in history but are user-owned in field_sources +- 5f7d54b fix(server): token actor, login CSRF, 415, cookie decoding, admin invites, production default +- 0b34ede docs: token actor, connection deleted_at and CSRF decisions; follow-ups + +## Item by item +1. **`deleted_at`.** + - Migration `0003_source_connections_deleted_at` (nullable text, with `down`) is registered and added to `schema.ts`. + - `deleteConnection` sets it (still pauses and wipes credentials) and is a 404 the second time. + - `ownedRow`, `listConnections`, `updateConnection`, the scheduler due-query (`jobs.ts`, as the ruling allows) and `syncHandler` (`DropJobError`) all ignore deleted rows. + - `isDeletedConnection` and the route-side filter are gone. + - Task 9's connections test asserted that a deleted connection is still listed. It now asserts that it is hidden and that `deleted_at` is set. + - Tests: + - core: paused and resumable; deleted gives 404 on get, update and delete; never scheduled; queued sync dropped; + - API: paused source listed and resumed. + - The `schema.test` down-migration still passes. +2. **Token actor.** + - `fieldOwner(actor)` in `history.ts` stores every non-`plugin:` actor as `"user"`. It is applied in `entries.writeEntry`, `engagements` (add and update) and `apply.listFieldSources` (list ownership read from history). Copies have no `field_sources`. + - `authenticateAppToken` returns `{ user, tokenName }`; `verifyAppToken` delegates to it, so its signature is unchanged. `http.ts` sets `ctx.actor`: `token:` for Bearer, `"user"` for sessions. Every write route passes it, now including lists and copies. + - Tests: + - core: history has actor `token:script` and `field_sources` has `"user"`; a later sync (plugin `flip`) leaves the status alone, files a pending `changed`, and still applies the source-owned rating; + - API: a write-scope token's PATCH gives history actor `token:script` and `fieldSources.rating` `"user"`. + - DECISIONS: a new entry supersedes the earlier bullets, because AGENTS.md makes DECISIONS append-only. +3. **CSRF and 415.** + - `X-Requested-With: sttracker` is required on every non-Bearer, non-GET request, login and register included. + - JSON bodies need `application/json`; anything else gets 415 `unsupported_media_type`. Empty bodies are still allowed for optional-body routes such as `sources/:id/run`. + - Tests cover login, register, Bearer exemption and 415. +4. **Malformed cookie.** `readCookie` catches the decode error. `stt_session=%` now gives 401 (tested). +5. **Production by default.** + - Dev mode is on only when `NODE_ENV === "development"` or `process.execArgv.includes("--hot")`. + - Deviation: `Bun.argv` does not contain `--hot`. I checked: under `bun --hot`, `Bun.argv` held only the bun path and the script, and `process.execArgv` held `["--hot"]`. + - TODO has a hand-off line for Task 19. +6. **`DELETE me/tokens/:id`.** `revokeAppToken` returns a boolean, and the route returns 404 for unknown, foreign or already-revoked ids (tested). +7. **`listSuggestions`.** It queries by the resolved canonical id. A test asks by an aliased id; it fails without the fix and passes with it. +8. **`admin/invites`.** Now `admin: true`. Tests: a member gets 403 on invites and on plugin reload; an admin can reload. +11. **OpenAPI multipart.** `RouteDef.formFields` is set on attachments. The multipart schema lists `file` (binary) plus `entity`, `entityId` and `caption`, with the required fields (tested). +12. **Tests.** + - Cookie plus a bogus Bearer on a write gives 401. + - A member calling reload gets 403. + - A multipart import over 50 MB gives 413 `validation`. + - A positive test with a write-scope token. + - The attachment test now uses bob's real entry (404), and checks that bob cannot read ana's attachment. + +## TDD notes +- The core items were RED first: + - `deleted_at` tests: 2 fail. + - Token actor: missing `authenticateAppToken` export. +- For items 3, 4 and 7 I wrote the fix before the test. I then confirmed each test catches its bug by reverting the fix temporarily: the readCookie try/catch, the `none`-route CSRF exemption, the 415 check and the canonical suggestions id each made their test fail. + +## Verification +- `bun run typecheck`: clean. +- `bun test`: 321 pass, 0 fail (34 files). +- `TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres bun test`: 520 pass, 0 fail. +- Real boot on a fresh SQLite DATA_DIR: + - migrations up to 0003 applied; + - `openapi.json` was served; + - a login POST without the header returned the 403 CSRF error; + - SIGTERM shut it down cleanly. +- `git status`: clean. + +## Still open +- The permission files and AGENTS.md Commands are still not updated; this needs a human (see Concerns 1 above). +- `restoreVersion`, `acceptSuggestion` and `resolveChange` take no actor, so token calls to them record `"user"` in history. Logged in `docs/TODO.md`. diff --git a/docs/plans/001-phase1-sdd/reports/task-17-report.md b/docs/plans/001-phase1-sdd/reports/task-17-report.md new file mode 100644 index 0000000..6bcc401 --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-17-report.md @@ -0,0 +1,222 @@ +# Task 17 report: web app shell, auth, library, quick add, entry detail + +**Status:** DONE_WITH_CONCERNS +**Branch:** `task/17` (worktree `/home/user/STTracker/.worktrees/t17`, based on task/16 at 0a54020) + +## Commits +- `0bb871c` feat(web): api client and display formatters +- `70bc6c3` feat(web): theme tokens, self-hosted Inter, HTML entry +- `439d138` feat(web): app shell, library, entry detail + +## What was built +- **`index.html`** replaces the Task 16 placeholder: a Bun HTML import with `src/theme.css` and `src/main.tsx`. The server's import path (`../../web/index.html` in `apps/server/src/main.ts`) is unchanged, and the server was not touched. +- **`src/api.ts`**: `request()` and `ApiError(status, code, message)`. Cookie auth uses `credentials: "same-origin"`. **Every** non-GET request sends `X-Requested-With: sttracker`, including `auth/login` and `auth/register` (per the coordinator's heads-up). Non-multipart non-GET requests always send `content-type: application/json`, even with no body. Multipart requests send only the CSRF header. Non-JSON errors become `http_`; fetch failures become `network` (status 0). Wire types are mirrored locally because apps/web has no `@sttracker/core` dependency, and `package.json` must not change. +- **`src/format.ts`**: `formatRating(v, "5half"|"5"|"10"|"100")`, `formatPartialDate`, `formatDateRange` (adds "(no exact date)" when either end is coarser than a day), `parsePartialDate` (string-only, never through `Date`), `rollupLabel` ("season 3 of 5", "5 seasons", "all 200 episodes"), plus status, media-type, ordinal and time-ago helpers. +- **Routes (wouter):** `/login`, `/register` ("Create the admin account"), `/` and `/entries/:id` (the same `Library` component, so the list keeps its state), and a `NotYet` placeholder for `/log`, `/lists`, `/owned`, `/changes`, `/sources`, `/settings` and `/about`, which Task 18 replaces. +- **Shell:** sidebar with Library, Log, Lists, Owned, Changes (pending count badge in `--warn`), the Sources list (connections with a status dot, then "All sources" or "Connect a source"), Settings, and the user name with "Sign out". On mobile (≤860px) it becomes a top bar. +- **List pane:** search (debounced), the Quick add input with the exact placeholder (Enter creates the entry, selects it and clears the box), status chips plus a "Draft" chip, media-type chips, and rows showing cover or striped draft cover, title, `type · meta` (child roll-up, else status, plus year), and the Draft badge, stars, or an accent dot when in progress. An orange dot marks entries with pending changes. Also: "Show more" (cursor paging), skeleton loading, empty and no-match states. +- **Detail pane:** + - Header: cover; title with Draft badge; meta line ("Book · Read twice · last finished 14 Sep 2026"); a star rating slider in the user's scale; status select; History button; tag pills (remove with ×; add with Enter or a comma). + - The orange change alert appears when pending changes target the entry or its engagements. "See what changed" goes to `/changes?entry=:id`. + - "Link to catalogue" (drafts only) shows match suggestions with "Link" / "Not this", and a catalogue search whose "Link" calls `POST entries/:id/move`. + - Boxes: + - Reads/Plays/Watches/Listens: newest first, numbered "2nd read", with partial dates; add form, and remove. + - Your copy: list, plus an add form for bought at, format, when, price and currency. + - Attachments: upload with CSS background-image thumbnails. + - Linked sources: derived from `plugin::` actors in `fieldSources`. + - Details: custom fields for every field type, saved on blur or change. The box is hidden when no fields are defined. + - Children: each with its own rating, linking to the child entry. + - Your review: Gelasio, a Markdown textarea that saves on blur or Cmd/Ctrl+Enter. + - History drawer: a non-modal side panel that takes focus, closes on Esc and returns focus to the History button. It shows field-level before → after diffs and "Restore". +- **Rating control:** a `role="slider"` with `aria-valuetext` ("4½ stars"). Arrows step by half a star, Home/End jump, and Delete/Backspace clears. Pointer clicks pick a half or full star. Keyboard steps are saved once, 500 ms after the last key, on blur, or on unmount, so stepping does not write a history row per keypress. The 10- and 100-point scales use a labelled number field. +- **Rating scale setting:** `src/settings.ts` (`useRatingScale`, `setRatingScale`) defaults to `5half`. The API has no preferences endpoint, so the value is stored in localStorage for now. Task 18's Settings should call `setRatingScale`. + +## Files +`apps/web/index.html`, `apps/web/src/{main.tsx,api.ts,format.ts,hooks.ts,settings.ts,theme.css}`, `apps/web/src/routes/{login,register,library,not-yet}.tsx`, `apps/web/src/components/{Shell,EntryDetail,DetailBoxes,HistoryDrawer,LinkToCatalogue,StarRating,Cover,Icon}.tsx`, `apps/web/src/fonts/{inter-latin-wght-normal.woff2,Inter-OFL.txt}`, `apps/web/test/{api,format}.test.ts`. + +## TDD evidence +- RED: `bun test apps/web` failed with `Cannot find module '../src/api.ts'` (and the same for `format.ts`): 0 pass, 2 fail. +- GREEN: `bun test apps/web` gave **15 pass, 0 fail, 58 expect() calls**. The tests include the brief's exact values: `formatRating(90,"5half")="★★★★½"`, null gives `""`, `{2025-03,month}` gives `"Mar 2025"`, `{2019,year}` gives `"2019"`, and a non-2xx JSON error throws an `ApiError` carrying `code`. Additional tests cover CSRF and content-type headers on login and on body-less POSTs, multipart without content-type, query building, 204, non-JSON errors and network errors. +- No DB code was touched, so the Postgres run is not applicable. + +## Typecheck +`bun run typecheck` (`tsc --noEmit -p tsconfig.json`) is clean, with no output. + +## Manual check (headless Chrome via CDP; nothing installed) +`/usr/bin/google-chrome --headless=new` was driven by a throwaway Bun CDP script in `/tmp/t17-cdp/` (outside the repo). It ran against a fresh `DATA_DIR`, first in dev mode and then with `NODE_ENV=production`. +- `/` redirects to `/login`. On `/register`, "Create the admin account" registers and auto-signs in, then the empty library shows. +- Quick add: typing "From the Library of Jurgen Leitner" and pressing Enter created the entry, navigated to `/entries/:id`, highlighted the row and opened the detail. +- Keyboard rating: 9 × ArrowRight gave `aria-valuetext` "4½ stars". The API then reported `rating: 90` with exactly one history row for it. +- Engagements, tags, copy and review render. The History drawer works. Mobile at 390px stacks correctly: the list alone on `/`, the detail alone with "‹ Library" on `/entries/:id`. +- Production mode: HTML, CSS and JS chunks return 200, Inter is inlined into the CSS, and the browser console shows no errors. +- Screenshots: `/tmp/t17-cdp/01-login.png` … `08-library-mobile.png` (05 is the desktop detail, 06 the history, 07 the mobile detail). + +**Compared with the mockup (section 1):** the layout matches: 190px sidebar, 360px list and a fluid detail pane, with the same tokens, dashed amber "+ Quick add" box, 40×56 row covers, 110×160 detail cover with shadow, stars in `--rating`, pills, the 2-column box grid with the same headings and row layout ("2nd read · 2 Sep – 14 Sep 2026", "1st read · Mar 2025 (no exact date)", Bought / When / price / Format) and a full-width Gelasio review box. Intended differences: +- Filter chips sit under Quick add, as the brief requires. +- A "Link to catalogue" box appears for drafts. +- "Search your library…" replaces "Search 1,284 entries…", because the API returns no total. +- The Changes badge and alert button use dark text on orange, because white on `#f97316` is 2.80:1 and fails AA. +- Sidebar group labels use `--text-faint` instead of the mockup's `#6f69a3` (3.59:1). + +Fixed during the check: +- The catalogue-search icon overlapped its placeholder. +- A visible scrollbar strip showed in the mobile top bar. +- Each arrow keypress on the rating saved a PATCH and a history row. +- The tag input animated its width, which the impeccable detector flagged. + +## Self-review +- Accessibility: + - Every input is labelled (sr-only labels where the mockup shows none). + - Focus is visible: a 2px `--focus` (#9d92ff) ring at 6.3:1. + - Chips use `aria-pressed`; rows use `aria-current`; the stars are a keyboard slider; the drawer manages focus; errors use `role="alert"`. + - Contrast was checked numerically: muted and faint text on all surfaces is ≥4.55:1, except `--text-faint` on `--surface-3`, so placeholders there use `--text-muted` (5.79:1). +- The impeccable detector also flagged "repeating stripes" and "Inter" (overused font). Both are pinned by DESIGN.md (the striped draft cover and Inter for the UI), so they were kept. +- No `TODO` comments in source. + +## Concerns +1. **No first-run endpoint.** The API cannot tell the client whether any user exists. `/register` assumes first run and switches to "Create your account" with an invite field when the server answers `invite_required` (or the link has `?invite=`). `/` → `/login` links "First time here? Create the admin account" either way. A `GET /api/v1/setup` (or a flag on `/about`) would let the client route straight to `/register` on first run. +2. **No route to list an entry's attachments.** Core has `listAttachments(db, userId, entity, entityId)`, but no route exposes it. The Attachments box uploads correctly but shows only files uploaded in the current session; after a reload they are not shown, though they remain stored. A `GET /attachments?entity=entry&entityId=` route is needed. +3. **No preferences endpoint.** `/me` does not return `users.preferences`, so the rating scale lives in localStorage (default 5 stars with halves) until an endpoint exists. +4. **Gelasio is not self-hosted.** No local copy was available and adding assets from the network was out of scope, so review text falls back to Georgia. Inter is vendored (OFL, licence included). +5. **Docs duties not done.** The dispatch limited me to `apps/web/**` because Task 19 is editing docs in parallel. These entries need adding by whoever merges: + - DECISIONS: localStorage rating scale; vendored Inter; dark text on `--warn` for AA; the register first-run heuristic; keyboard rating saves debounced. + - TODO: items 1–4 above. + - ARCHITECTURE: the web app's module layout (`api.ts`, `format.ts`, `hooks.ts`, `settings.ts`, routes and components). +6. Linked sources are inferred from `fieldSources` actors, so a source that matched an entry but never wrote a field will not appear. A per-entry source-links route would fix this. +7. In production the bundled HTML references `/../../chunk-*.css|js`, an artefact of Bun bundling `../../web/index.html` from `apps/server`. Browsers normalise it to `/chunk-*`, and those load (200), so it works. Worth knowing for the Task 19 Docker check. + +--- + +## Fix round 1 + +**Status:** DONE_WITH_CONCERNS (one: Gelasio ships as TTF, not woff2; see below). + +### Commits +- `d547355` Merge branch 'feat/phase1' into task/17 (clean, no conflicts; tests green after it) +- `d204775` feat(server): auth status, attachments list, user preferences +- `5fa5931` fix(web): review round 1: optimistic edits, friendly errors, preferences, Gelasio + +### Item 0: three API routes +- `GET auth/status` (auth none) → `{ hasUsers }`, via new `hasUsers(db)` in `core/auth.ts`. +- `GET attachments?entity=&entityId=` (any:read) → `requireLinkTarget` (404 when foreign or unknown), then `listAttachments`, each with its `links`. A bad `entity` gives 400. +- `GET/PATCH me/preferences` (any:read / any:write) → `{ ratingScale, defaultVisibility }` in the new `core/preferences.ts`. The keys match what `defaultVisibility()` reads; the test proves quick add then uses `"friends"`. The zod body is `.partial().strict()`. Unknown stored keys are kept. +- RED: `bun test apps/server/test/api.test.ts` → 3 fail (the new tests). GREEN: SQLite 32 pass / 0 fail; Postgres 59 pass / 0 fail. +- UI: + - `/login` redirects to `/register` on a fresh instance. + - `/register` shows "Create the admin account" or invite mode from `auth/status` (the `invite_required` fallback is kept). + - Attachments come from the API. + - The rating scale comes from `me/preferences`; the localStorage version is removed. + +### Items 1–3 +1. `useEntryPatch`: + - The cached `["entry", id]` updates optimistically in `onMutate` and rolls back in `onError` (then refetches). + - One entry's PATCHes run in order through a TanStack mutation `scope`; I checked in query-core 5.105 that `onMutate` runs immediately even while the mutation is queued. + - `edit(cur => patch)` builds from the latest cache. Tags and custom fields use it, through the `addTag`, `removeTag` and `setCustomValue` builders in the new `src/edits.ts`. + - Only the last queued edit writes the server's answer back. + - Test: "two quick tag adds built from the latest cache keep both". +2. The review textarea is controlled, with a `synced` ref. Incoming server changes (Restore) replace the text unless the user has typed since, so a later blur cannot undo a restore. +3. `friendlyError(err)` in `format.ts` maps `validation` field/reason pairs (core reasons and zod messages: not_url, not_date, bad_precision, out_of_range, required/too small, too_long/too big, not_number, unknown_option, duplicate, mime, password, username, currency/cents), and 413 or "larger than 25 MB" to "That file is too large. The limit is 25 MB.". Other API errors keep the server's message. It is used at every error display site (grep shows no raw `.message` left). It accepts `unknown`, which also covers `ApiError`. Tests cover the mapping. + +### Item 4: Gelasio +- No offline copy existed and there is no woff2 tooling (fonttools/brotli are not installed, and installing them would need approval). +- So I downloaded the official files, as the ruling allowed, from `raw.githubusercontent.com/google/fonts/main/ofl/gelasio`: `Gelasio[wght].ttf` (regular to bold, so 400 and 600) and `Gelasio-Italic[wght].ttf`, plus `OFL.txt`. They are vendored unmodified as `apps/web/src/fonts/gelasio-wght.ttf` (sha256 4daecea4…) and `gelasio-italic-wght.ttf` (sha256 52559e84…), with `Gelasio-OFL.txt`. Two variable `@font-face` rules cover weight 400–700, normal and italic. +- **Deviation:** they are TTF (~170 KB each), not latin-subset woff2. That is filed in TODO. + +### Minors (all done) +- Engagement delete has `onError`. +- Sign out catches errors and still clears the cache, resets the library filters and navigates. +- Ordinals use `orderEngagements` (`finishedAt ?? startedAt`, then `createdAt`; undated counts as oldest), with a test. +- The remove button is visible under `@media (hover: none)`. +- The upload error goes through `friendlyError`. +- The Changes count uses visually hidden " waiting" text; the source status dot has `role="img"`. +- Quick add uses `readOnly` + `aria-busy` and refocuses after success. +- `mediaTypeOverride ?? work.mediaType` is used for row covers, History status labels and child rows. +- `MONTHS`, `shortDate` and the past-tense verb are de-duplicated into `format.ts`. +- The caret uses `--text`. +- History shows `token:` actors as App token “name”. + +### Docs +- DESIGN.md: ratified dark text on `--warn`, `--text-faint` group labels, placeholders, self-hosted fonts. +- DECISIONS: one new entry at the top, newest first. +- TODO: Gelasio woff2, linked-sources inference, attachments of reads/copies, and a Known-issue note on the `/../../chunk-*` production paths. +- ARCHITECTURE: the apps/web row and `core/preferences`. + +### Verification +- `bun run typecheck`: clean. +- `bun test` (full): 347 pass / 0 fail. With `TEST_POSTGRES_URL=…55432…`: 549 pass / 0 fail. +- CDP re-check (headless Chrome, fresh instance, production mode): + - A fresh `/login` redirects to `/register` "Create the admin account". + - Quick add keeps focus on `#quick-add`. + - Two quick tag adds give `["cosmic horror","re-read"]` in the UI and on the server. + - Review: "First thoughts." → "Second thoughts." → Restore shows "First thoughts." in the UI, and after a blur the server still has "First thoughts.". + - An uploaded attachment is still listed after reload. + - After sign-out, `/login` stays on `/login`, and `/register` shows "Create your account" with the invite field. + - The browser console was empty. +- Screenshots: `/tmp/t17-cdp/fix1-detail.png`, `/tmp/t17-cdp/fix1-register-invite.png`. + +--- + +## Fix round 2 + +**Status:** DONE. **Commit:** `4608c97` fix(web): keep review text on a failed save; review round 2. +Files touched: `apps/web/src/{format.ts, components/DetailBoxes.tsx, components/EntryDetail.tsx, components/HistoryDrawer.tsx, routes/register.tsx}`, `apps/web/test/format.test.ts`, `packages/core/src/preferences.ts`. + +1. **Failed review save keeps the text.** + - Callback order is not reliable. The hook-level `onError` rollback, React's effect flush and the per-call `onError` can interleave, so I did not rely on "restore `synced` before the rollback lands". + - The sync logic is now pure helpers in `format.ts` (`Draft`, `draftIncoming`, `draftStartSave`, `draftSaved`, `draftFailedSave`): + - while a save is in flight, incoming values (optimistic, then rollback) are ignored; + - on failure, `synced` goes back to `was`, so the local text counts as unsaved. + - Overlapping saves are counted (`inFlight`). + - Tests: "a rollback after a failed save keeps the local text", in both orders (rollback first, error callback first), plus "Restore replaces untouched text but not edited text". + - **Second bug found by the CDP check:** + - The failed save's `onError` invalidates the entry, the refetch fails while the server is down, and `EntryDetail` then showed "Couldn't load this entry". That unmounted the review box and lost the text. + - `EntryDetail` now shows the error screen only when there is no cached data; TanStack keeps `data` after a failed refetch. +2. `was` is captured before any state update and used in the failure path. +3. History restore uses `scope: { id: "entry-patch:" }`, the same scope as `useEntryPatch`, so it queues behind a pending blur-save. +4. `updatePreferences` does its read-modify-write in one transaction, with `forUpdate()` on Postgres. This follows `connections.ts`; SQLite runs on a single connection. +5. `register` invalidates `["auth-status"]` after success. +6. `friendlyError(err, { ratingScale })`: out-of-range ratings read "Pick a rating between 0 and 5 stars." (10 → "between 0 and 10", 100 → "between 0 and 100"; custom fields: "Pick a rating for Mood between …"). It is passed the user's scale at the entry and custom-field error sites. Tests cover this. + +### Verification +- `bun run typecheck`: clean. +- `bun test apps/web apps/server packages/core`: 241 pass / 0 fail (SQLite). With `TEST_POSTGRES_URL`: 436 pass / 0 fail. +- CDP check (headless Chrome; the driver spawned and killed its own server, restarted on the same DATA_DIR): + 1. "Saved first." reached the server. + 2. With the server stopped, I typed "Written while the server was down." and blurred. The error shows ("Can't reach the server. Check your connection and try again.") and the text is kept. + 3. After the restart the text is still there, and blurring again saves it (the server has the new text). + 4. The browser console was empty. +- Screenshot: `/tmp/t17-cdp/fix2-failed-save.png`. + +### Noted, not changed (outside this round's files) +While the server is down, the library list pane replaces its rows with the error message on a failed refetch. Same pattern as `EntryDetail`; `routes/library.tsx` was not in this round's file list, so it is unchanged. + +--- + +## Fix round 3 + +**Status:** DONE. Touched only `apps/web/src/{components/DetailBoxes.tsx,hooks.ts,format.ts}` and `apps/web/test/format.test.ts`. + +**Fix (`saving` stuck on overlapping saves):** +- Bookkeeping no longer uses per-call `mutate(..., { onSuccess, onError })`; those fire only for the observer's latest call. +- `useEntryPatch(id, { onSettled })` gains a hook-level callback. It is held in a ref and fires once for **every** mutation, with `(patch, error)`. +- `ReviewBox` pushes `{ was, stored }` onto a FIFO ref per save. Each settle shifts the oldest; the entry-patch scope is serial, so they settle in order. It then calls `draftSettled(draft, save, ok)`. +- The draft helpers in `format.ts` changed: + - `Draft` is now `{ text, synced, pending, incoming, echoes }`, with `newDraft`, `draftIncoming`, `draftStartSave` and `draftSettled`. + - While saves are pending, incoming values are only remembered. + - When the last save settles, the remembered value is applied unless it is one of our own echoes (a save's optimistic `stored` or rollback `was`). +- That echo filter fixes a subtle case the new tests caught: in fail-then-success, the last remembered value is the first save's rollback ("old"). Re-applying it would replace text the server already has. A genuine outside change (a Restore, another tab) is still applied once saving ends. + +**Tests (format.test.ts, "review draft sync"):** +- A rollback after a failed save keeps the local text. +- Two overlapping saves: + - success/success: both settle, `pending` returns to 0 and the echoes clear; + - fail/success: ends synced; + - success/fail: the text stays unsaved, with `synced` at the first save. +- An incoming value that arrives while saving is applied once saving ends. +- An outside change replaces untouched text but not edited text. +- RED: `draftSettled` was not exported. GREEN: `bun test apps/web` gives 28 pass / 0 fail. + +**Verification:** +- `bun run typecheck`: clean. +- Full `bun test`: 352 pass / 0 fail (SQLite); with `TEST_POSTGRES_URL`, 554 pass / 0 fail. +- The browser failed-save check (`/tmp/t17-cdp/drive3.ts`) passed again: text typed during the outage is kept, survives the restart, and re-saves to the server. The console was empty. diff --git a/docs/plans/001-phase1-sdd/reports/task-2-report.md b/docs/plans/001-phase1-sdd/reports/task-2-report.md new file mode 100644 index 0000000..32c522f --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-2-report.md @@ -0,0 +1,25 @@ +# Task 2 report: schema migration and Database types + +Status: DONE_WITH_CONCERNS. Branch `worktree-agent-a36198c2788ae2f86`. Commits: da04adb `feat(db): initial schema`, then the fix-round commit `fix(db): ...`. + +## Original work (da04adb) +- Worktree was on main; fast-forwarded to feat/phase1 (27ec24e) before starting. +- `packages/db/src/schema.ts`: 26 table interfaces and `Database`. No `Generated<>`, since all IDs and values come from app code. `favorite` is a number (0/1), JSON columns are strings. +- `packages/db/src/migrations/0001_initial.ts`: schema builder only, with text/integer/real. NOT NULL unless the brief says null. FK columns have `references` and an index. All brief unique constraints and the (user_id, listened_at) and (status, run_at) indexes are present. Extra indexes: works.media_type, works.merged_into_id, attachments.sha256, (entity, entity_id) on attachment_links and user_history, (target_entity, target_id) on source_records, (user_id, status) on change_events, jobs.lock_key. +- `migrations/index.ts` registers `0001_initial`. `AGENTS.md` now says UUIDv7 (`newId()`). +- `packages/db/test/schema.test.ts`: unique constraint test from the brief. +- TDD: RED was `SQLiteError: no such table: users`. GREEN on SQLite and Postgres. +- Decisions: `works.merged_into_id` and `cover_attachment_id` have no FK. `attachments.size` is `integer`, which is int4 on Postgres. No column defaults. + +## Fix round 1 +1. `down` in 0001_initial.ts drops all 26 tables in the reverse order given. New test "migrating down removes the tables" uses `Migrator.migrateDown()` and asserts `select 1 from users` rejects, on both dialects. +2. `packages/db/src/testing.ts`: for Postgres, `beforeAll` creates and migrates a template `st_tpl_` and closes its connections. `beforeEach` runs `create database st_ template st_tpl_`. `afterEach` closes the connection and drops the test DB. `afterAll` drops the template. All hooks have a 30 000 ms timeout. SQLite is unchanged (`:memory:` plus migrate). +3. `EntryStatus` union added in schema.ts and used for `entries.status`. The test uses `"planned" as const`. + +## Verification +- `bun run typecheck`: clean. +- `bun test packages/db` (SQLite): 7 pass, 0 fail. +- `TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres bun test packages/db`, run 3 times in a row: 13 pass, 0 fail each time. No hook timeouts. + +## Concerns +- The shared path `/home/user/STTracker/.superpowers/...` is write-blocked for this worktree-isolated agent. This report is at `.superpowers/sdd/001-phase1-own-your-data/task-2-report.md` inside the worktree, uncommitted, and the controller needs to copy it over. diff --git a/docs/plans/001-phase1-sdd/reports/task-3-report.md b/docs/plans/001-phase1-sdd/reports/task-3-report.md new file mode 100644 index 0000000..3f98e43 --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-3-report.md @@ -0,0 +1,13 @@ +# Task 3 report: packages/plugin-sdk + +Built: types.ts (contract + definePlugin validating sdkVersion===1 and non-empty id), http.ts (createHttpClient, HttpError, token bucket, retry/backoff, Retry-After seconds/date, X-RateLimit-Reset-In, optional `now`), testing.ts (createFixtureHttp, createTestContext, collect), partial-date.ts (parsePartialDate, comparePartialDates; string-only, no Date conversion for values). +Files: packages/plugin-sdk/{package.json (copied from main checkout, where it was untracked), src/*.ts, test/http.test.ts, test/partial-date.test.ts}. +TDD: RED = `bun test packages/plugin-sdk` -> "Cannot find module '../src/partial-date.ts'", 0 pass 2 fail. GREEN = 24 pass, 0 fail. No DB, so no Postgres run. `bun run typecheck` clean. +Decisions: default maxRetries 3, backoff 500*2^attempt ms; retries also consume rate tokens; json throws HttpError on any non-ok, raw returns non-retryable responses as-is and throws HttpError only when retries on 429/5xx are exhausted; fixture lookup tries full URL then URL without query; datetime partial dates keep the full string with precision "day"; invalid calendar dates return null; compare is lexicographic on value. +Concerns: worktree was branched before packages/ existed, so plugin-sdk/package.json was copied in from /home/user/STTracker; bun.lock untouched. + +## Fix round 1 +- testing.ts: `createTestContext` defaults to a fetch that throws `Error("createTestContext: no fetch provided")`. +- http.ts: `HttpError` constructor builds its message from origin + pathname only (covers both throw sites). +- Tests added: no-fetch context rejects with that message; 404 on `?api_key=SECRET` gives a message without "SECRET". +- Command: `bun test packages/plugin-sdk` -> 26 pass, 0 fail. `bun run typecheck` clean. diff --git a/docs/plans/001-phase1-sdd/reports/task-4-report.md b/docs/plans/001-phase1-sdd/reports/task-4-report.md new file mode 100644 index 0000000..5de6d72 --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-4-report.md @@ -0,0 +1,10 @@ +# Task 4 report +Built packages/core/src/auth.ts (registerUser, login, getSessionUser, logout, createInvite, createAppToken, verifyAppToken, revokeAppToken, User) and packages/core/test/auth.test.ts (8 tests per dialect). +TDD: RED = module not found (bun test packages/core/test/auth.test.ts). GREEN: SQLite 8 pass; Postgres 16 pass (both dialects); typecheck clean. +Notes: invite is claimed after user insert inside the tx (used_by FK to users); claim is an atomic UPDATE ... where used_by is null and unexpired. Unknown/expired/used invite -> invite_required. Hashes are sha256 hex. Invite code = 16 random bytes base64url. Uses Uint8Array.toBase64 (Bun 1.3). +Concerns: none. Login does no timing equalisation for unknown users. + +## Fix round 1 +- registerUser takes pg_advisory_xact_lock(727401) on Postgres at tx start (SQLite already serialised); concurrent first sign-ups give one admin, other gets invite_required. +- Invite check now precedes username_taken. login verifies against a lazily computed dummy argon2id hash for unknown users. +- New tests: concurrent registration, no-invite+taken username, hashes differ from plaintext, cross-user revoke. SQLite 12 pass; Postgres 24 pass (both dialects); typecheck clean. diff --git a/docs/plans/001-phase1-sdd/reports/task-5-report.md b/docs/plans/001-phase1-sdd/reports/task-5-report.md new file mode 100644 index 0000000..9af87d3 --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-5-report.md @@ -0,0 +1,57 @@ +# Task 5 report +Built packages/core/src/works.ts (createWork, getWork, updateWorkFromHint, findWorkByExternalIds, searchWorks, resolveAlias, adminMergeWorks, Work, plus *In(k) variants usable inside transactions) and matching.ts (fuzzyScore, matchWork). Tests in packages/core/test/{works,matching}.test.ts. +acceptSuggestion/rejectSuggestion omitted per controller ruling (Task 6). +TDD: first run of the suggestions test failed on both DBs (0 suggestions); fixed; final: sqlite 11 pass, sqlite+postgres 21 pass, typecheck clean. +Decisions / deviations: +- fuzzyScore: a component missing on either side (year, creators) is dropped and weights renormalised; otherwise "The Expanse: Leviathan Wakes" vs "Leviathan Wakes" scores 0.467, not >0.6, and Pokemon=1 with absent year/creators would be 0.7. +- Candidate LIKE bounding uses OR over up to 5 significant tokens (>=3 chars), not only the first token: first-token-only misses "Expanse" vs "Leviathan Wakes". Still limit 200, scored in TS. +- Parent without ids/position: reuse shared work with identical media type, lower(title), year, parent; else create shared. +- Position child lookup: parent_id + normalized position + shared + unmerged. +- adminMergeWorks does not repoint children, list_items, copies or listens (getWork follows tombstones). Role read from users.role. Logs work_history for both works; nothing in user_history. +- Work type keeps snake_case column names with creators/metadata/position parsed. +- createWork writes a work_history "created" row. + +## Fix round 1 +Commit 437ca14 on `task/5`: fix(core): race-safe matching, visibility-checked ID matches, child-preserving merges. + +Changes: +- 1a: `packages/db/src/migrations/0002_works_position_unique.ts` (unique index `works_parent_id_position_uq` on `works(parent_id, position)`, with `down`), registered in `migrations/index.ts`. `schema.test.ts` "migrating down" now uses `migrateTo(NO_MIGRATIONS)` (one `migrateDown` only undoes 0002). +- 1b: `matchWork` re-runs the whole resolve in a fresh transaction on a unique violation, max 3 attempts. `isUniqueViolation(err)` (exported from matching.ts) checks `code`/`errno` "23505", or a message with "UNIQUE constraint failed" / "duplicate key", walking up to 5 levels of `cause`. +- 1c: `addMissingExternalIds` inserts with `onConflict(columns namespace,value).doNothing()`. It records an id in history only if `numInsertedOrUpdatedRows > 0`. +- 2: `adminMergeWorks` repoints `from`'s children to `to`, except a child whose non-null position `to` already has. Moved child ids are logged in `to`'s work_history (`children`). +- 3: `findWorkByExternalIds(db, ids, userId = null)` / `findWorkByExternalIdsIn(k, ids, userId)` follow merges and accept a work only if it is shared or owned by userId (null = shared only). An invisible match is skipped and the next namespace is tried. `matchWork` passes userId; parent resolution passes null. `createWork` with `ownerUserId` + external ids throws `ValidationError("externalIds", "private works cannot carry external ids")`. +- 4: `draft` reads the owner of the aliased workId. `adminMergeWorks` throws `ValidationError` unless both works are shared. +- Docs: DECISIONS.md entry (race safety, visibility, merge children). TODO.md known issue: a shared parent resolved without IDs or position (title/year reuse) has no unique key, so concurrent matches can still duplicate it. + +New tests: +- schema: works (parent_id, position) uniqueness; NULL parents and NULL positions are not constrained. +- works: createWork refuses ext ids on private; findWorkByExternalIds visibility (user B gets the shared work, never A's private one, even when A's private work owns one of the hint's IDs; A gets its own); getWork merged_into cycle → null; literal `_` in search; adminMergeWorks refuses private; merge show A into B, then matching A's episodes creates no works (s1 moved, s2 left under A because B had s2). +- matching: concurrent (Promise.all ×3) same new external id → 1 work and 1 external_id; concurrent same new episode chain → 3 works, same id; concurrent new season/episode under an existing show → same id, 3 works; concurrent addition of the same new ext id → no failure, 1 row; draft follows the alias. + +RED (before the fix): sqlite 17 pass / 5 fail. Postgres run: 29 pass / 14 fail; the concurrency tests failed with `PostgresError: duplicate key value violates unique constraint "external_ids_namespace_value_uq"`, errno "23505". Before the migration, the schema test failed on sqlite. + +GREEN: +- `bun test packages/core packages/db`: 30 pass, 0 fail (92 expect calls). +- `TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres bun test packages/core packages/db`: 58 pass, 0 fail (175 expect calls). +- Postgres matching.test.ts repeated 5×: 21 pass, 0 fail each time. +- `bun run typecheck`: `tsc --noEmit -p tsconfig.json`, exit 0, no output. + +Note: on SQLite the concurrency tests pass trivially, because the single connection serialises transactions. Only Postgres exercises the retry. + +## Fix round 2 +Commit on `task/5`: fix(core): follow merged tombstones in the position lookup. + +Changes: +- `matching.ts` step 2: the position lookup no longer filters `merged_into_id is null`. A found child is followed through `finalWorkId`, and that surviving work is the one updated and returned. Before, a merged tombstone still held its (parent_id, position) slot under `works_parent_id_position_uq`, so `createWorkIn` collided on every retry. +- docs/TODO.md known issue: "Admin merge does not merge colliding children recursively; colliding children stay under the tombstoned parent." + +Test (matching.test.ts): "position lookup follows a merged tombstone instead of colliding with it". It creates episodes E1 (s1e1) and E2 (s1e2) under one season and admin-merges E1 into E2. Matching an ID-less hint for that season at E1's position then returns `{ workId: E2, created: false, draft: false }` and creates no works. + +RED: `bun test packages/core/test/matching.test.ts`: 11 pass, 1 fail, with `SQLiteError: UNIQUE constraint failed: works.parent_id, works.position`. + +GREEN: +- `bun test packages/core packages/db`: 31 pass, 0 fail. +- `TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres bun test packages/core packages/db`: 60 pass, 0 fail. +- `bun run typecheck`: exit 0. + +Note: following the tombstone applies the hint's display fields to the surviving work (E2), the same as an external-ID match through a merge in step 1. diff --git a/docs/plans/001-phase1-sdd/reports/task-6-report.md b/docs/plans/001-phase1-sdd/reports/task-6-report.md new file mode 100644 index 0000000..79871d3 --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-6-report.md @@ -0,0 +1,221 @@ +# Task 6 report: user layer (entries, engagements, history, lists, copies, custom fields) + +**Status:** DONE_WITH_CONCERNS +**Branch:** `task/6` (worktree `/home/user/STTracker/.worktrees/t6`) + +## Takeover note +I took this task over from a previous implementer who was stopped by a usage-limit pause. +- Already committed by them: `56b86e2 feat(core): custom field definitions and value validation`. I kept it unchanged. +- Uncommitted WIP from them: `history.ts`, `entries.test.ts`, `history.test.ts`. I reviewed all three and kept them, with these changes: + - `history.ts`: `restoreVersion` now dispatches to engagement, copy and list restores as well as entries. + - `entries.test.ts`: added `!` to two array indexes to satisfy typecheck, and added one quickAdd media-type assertion. + - `history.test.ts`: added a test for restoring a move. +- `entries.ts` did not exist yet. I wrote it, along with engagements, lists, copies and their tests. + +## Commits +- `13b81b1 feat(core): entries, user history and match suggestion accept/reject` +- `c7471f7 feat(core): engagements, lists and copies with history restore` + +Each commit was verified green on its own, on both databases, with typecheck clean. I split the work into two commits instead of the brief's single Step 5 message, because the controller asked for logical steps. + +## What I built +- **history.ts** + - `Actor` type and `recordHistory(tx, …)`, which stores only the changed keys and writes nothing if nothing changed. + - `getHistory`, returning newest first. + - `restoreVersion`, which re-applies `before` as actor `"user"` for entry, engagement, copy and list rows. + - `inTx` helper: use the caller's `tx` if given, otherwise open a new transaction. +- **entries.ts** + - Writes: `upsertEntry`, `softDeleteEntry`, `moveEntryToWork`, `quickAdd`. `quickAdd` uses a local `insertPrivateWork` and does not depend on Task 5. + - Reads: `getEntry` and `listEntries`, both returning `EntryView`. + - Suggestions: `acceptSuggestion` and `rejectSuggestion`. + - Shared validators used by the other modules: rating, text, keys, partial date, default visibility, `resolveVisibleWork`, `requireEntry`, `MEDIA_TYPES`. + - Data rules: + - `field_sources` is set per patched key. + - `draft` and `childSummary` are computed with correlated subqueries, not N+1 queries. + - `overrides` (`title`, `coverUrl`, `year` only) are overlaid onto `EntryView.work`. + - `listEntries` behaviour: + - Keyset cursor is base64 JSON `{sort, sortValue, id}`. Default limit 50, capped at 200. + - Sorts: `updated` desc, `title` (case-insensitive asc), `rating` (desc, nulls last). + - Filters: `mediaType` (override-aware), `status`, `tag` (case-insensitive), `draft`, `q` (title or original title, LIKE-escaped), `parentId` (default null = top level). +- **engagements.ts** + - `addEngagement`, `updateEngagement`, `softDeleteEngagement`, `listEngagements`. + - Takes `PartialDate`. Value and precision are stored separately, and null is stored as null with precision `"unknown"`. + - A date's value must parse to exactly the stated precision; a full ISO datetime is accepted with precision `day`. + - Custom fields are checked against engagement-level definitions. + - `listEngagements` sorts by start date, with undated engagements last. +- **lists.ts** + - `createList`, `updateList`, `softDeleteList` (an extra), `addListItem` (optional `position`), `moveListItem` (index is clamped), `removeListItem`, `getList`, `listLists({kind?})`. + - Only a `grouping` may have `rating` or `review`. + - Item positions stay dense, from 0 to n-1. + - The list's history snapshot includes its items, so removing an item can be undone with `restoreVersion`. +- **copies.ts** + - `createCopy`, `updateCopy`, `softDeleteCopy`, `listCopies({workId?})`. Actor defaults to `"user"`, and there is an optional `tx`. + - A copy needs either `workId` or `titleHint`. + - `priceCents` must be a non-negative integer, and `currency` must be a three-letter ISO code. + - `acquiredAt` is a partial date. + - Custom fields are checked against copy-level definitions. +- **Optional `tx`:** every function the ruling lists takes `tx?` as its last parameter. +- **Other users' rows:** every function throws `NotFoundError`. Another user's private works also return `NotFoundError`. + +## Files +- `packages/core/src/`: `history.ts`, `entries.ts`, `engagements.ts`, `lists.ts`, `copies.ts`. `custom-fields.ts` came from the earlier commit. +- `packages/core/test/`: `history.test.ts`, `entries.test.ts`, `engagements.test.ts`, `lists.test.ts`, `copies.test.ts`, `suggestions.test.ts`. `custom-fields.test.ts` and `user-layer-fixtures.ts` came from the earlier commit. + +All nine brief tests exist under their exact names. "custom select field rejects unknown option" is in `custom-fields.test.ts`, from the earlier commit. + +## TDD evidence +- **RED** (`bun test packages/core/test`, after writing all tests and before any implementation): + ``` + error: Cannot find module '../src/entries.ts' … + error: Cannot find module '../src/engagements.ts' … + error: Cannot find module '../src/copies.ts' … + error: Cannot find module '../src/lists.ts' … + 4 pass + 6 fail + ``` +- **RED** for the move-restore test (`bun test packages/core/test/history.test.ts`): + ``` + (fail) history [sqlite] > restoreVersion undoes a move back to the draft work + ValidationError: targetWorkId: same_work + ``` + Fixed by deleting the user's alias before moving back. +- **GREEN**: + - `bun test packages/core` (SQLite): 40 pass, 0 fail. + - `TEST_POSTGRES_URL=… bun test packages/core` (both dialects): 79 pass, 0 fail. + - Whole repo: `bun test` gives 73 pass; with Postgres, 118 pass, 0 fail. + - Output is clean. +- **Typecheck:** `bun run typecheck` printed `tsc --noEmit -p tsconfig.json` and no errors. +- **Not seen RED first:** the quickAdd built-in media type check was added together with its assertion. + +## Decisions made (candidates for docs/DECISIONS.md) +- **History snapshot keys:** + - Entry rows use the EntryPatch keys plus `workId` and `deletedAt`. + - Engagement and copy rows use their patch keys plus `deletedAt`. + - List rows use the list fields plus `deletedAt` and `items`. + - The first history row of each entity is a creation row with an empty `before`; restoring it is a no-op. +- **Works are resolved on every write:** `upsertEntry` (and copy and list item writes) follow the user's `work_aliases` and global `merged_into_id` before writing, so writing to an old work id lands on the moved entry. +- **Moving onto a work the user already has an entry for** (the WIP test required this): + - If the target entry is live, it is kept. Its empty fields are filled from the moved entry. Tags, custom fields and overrides are combined. Engagements are moved to the target entry, and the source entry is soft-deleted. + - If the target entry is soft-deleted, the moved entry's values overwrite it and it is revived. +- **Alias bookkeeping on a move:** + - Aliases that pointed at the old work are re-pointed to the target. + - Any alias starting from the target is dropped, so no cycles form. + - The user's own private draft work is tombstoned (`merged_into_id`). +- **Restoring a move** clears that tombstone and the alias, then moves the entry back. +- **`customFields` patches replace the whole map,** so restore is exact. `field_sources` key: `customFields`. +- **Cursor format:** the cursor also carries `sort`, so a cursor from one sort used with another sort throws `ValidationError("cursor")`. +- **`quickAdd` mediaType** must be built-in (spec §6.1). User-defined types belong in `mediaTypeOverride`. +- **`acceptSuggestion`** throws `ValidationError("status","not_pending")` for a suggestion that is not pending. If the user has no live entry on the work, it only updates suggestion statuses. + +## Concerns +1. **Hidden entries in the default list.** The ruling's top-level filter is `parent_id IS NULL` and `childSummary` counts direct children only. As a result: + - An episode entry whose show has no entry for this user does not appear in the default list. + - A show → season → episode tree with no season entries rolls up 0 children on the show. + + Trakt imports create show and episode entries, so this will happen. Suggest a follow-up: either count grandchildren, or treat "parent has no live entry" as top level. +2. **Docs duties not done.** I did not edit `docs/DECISIONS.md` or `docs/ARCHITECTURE.md`, because the controller said to touch only my files while Tasks 5 and 7 run in parallel. The decisions above should go into DECISIONS.md at merge. +3. **Search and title sort ignore overrides.** `q` and the `title` sort use the work's title, not the user's override title, because overrides are a JSON text column. The `tag` filter matches the JSON text with LIKE. +4. **Restore gaps after a merge-move.** Engagements moved to the surviving entry are not written to engagement history. Restoring the source entry's soft-delete revives it on its old work, but the alias from that work stays in place. +5. **Concurrent creates can race.** Two simultaneous `upsertEntry` calls creating the same (user, work) entry fail on the unique constraint rather than retrying. + +--- + +# Fix round 1 + +**Status:** DONE +**Branch:** `task/6` + +## Commits +- `4c65939 merge: feat/phase1 into task/6`: no conflicts. Post-merge: 99 pass, typecheck clean. +- `512e6e9 fix(core): roll up descendants and surface orphaned entries; SQL-derived title cursor` (items 1–3). +- `7154ca0 fix(core): race-safe entry upsert and lossless per-user moves` (items 4–8 and the minors). +- `00fc018 docs: record user-layer decisions, core module map and known gaps` (item 9). + +Each commit was verified green on both dialects, with typecheck clean. + +## Findings addressed +1. **`childSummary`** now counts the user's live entries on works whose parent, grandparent or great-grandparent is the entry's work, through a portable 3-way OR over self-joined `works`. The rollup test gained the show → season (no entry) → episodes case. +2. **The default list** (`parentId: null`) now uses `NOT EXISTS` an ancestor, up to three levels, carrying a live entry of this user. New test "orphaned child entries are top level until an ancestor has an entry". It also checks that another user's entry and a soft-deleted entry on the show do not hide the episode. +3. **The title cursor** now carries the SQL `lower(w.title)` value, selected as `title_key`. New test "title pagination does not skip non-ASCII titles" pages one item at a time through seven mixed titles. +4. **`upsertEntry`** inserts defaults with `onConflict(user_id, work_id).doNothing()`, re-reads the row, and applies the patch through `writeEntry`. On a fresh create it writes a single history row with `before = null`. It never catches and retries. + - New test "concurrent upserts…" uses `Promise.all`. It passed 5 of 5 repeated runs on Postgres. +5. **Undoing a move** (`restoreEntryFromHistory`) deletes the forward alias D→S, un-tombstones the draft, and calls `moveEntry` with `{ undo: true }`. That mode skips all alias, copy and list bookkeeping. + - The history test now asserts that no aliases remain, and that `upsertEntry(S)` afterwards creates a separate entry on S while the draft entry stays untouched. +6. **`field_sources` on merge:** + - Combined fields (tags, customFields, overrides, concatenated text) get `"user"` if either side had `"user"`, otherwise the destination's value. + - A gap-filled field carries the source entry's value. + - A revived soft-deleted destination gets the source's `field_sources` as-is; keys absent there are removed rather than stamped with the actor. + - `writeEntry` now takes `opts.sources`, where `undefined` deletes the key. Two tests added. +7. **Merge history:** + - Each re-parented engagement (live or deleted) gets `updated_at` bumped and a `user_history` row with `{entryId}` before and after. + - The engagement snapshot now includes `entryId`, and engagement restore re-parents back after checking ownership. + - Differing non-empty `review`/`privateNotes` are joined with `\n\n---\n\n`. + - A conflicting rating keeps the destination's, and the destination's history row records `before.rating = ` and `after.rating = `. Restoring that row therefore applies the source rating. + - One test covers all of this. +8. **Copies and list items follow a move:** `repointCopiesAndListItems` moves this user's copies (with copy history `{workId}`) and the `list_items` in this user's lists. New test in `suggestions.test.ts` via `acceptSuggestion`. +9. **Docs:** merged `feat/phase1` first. + - One entry added at the top of `docs/DECISIONS.md` covering every bullet asked for: merge-on-move, alias bookkeeping with undo, cursor format, whole-map customFields, list_items hard-delete, hierarchy visibility, plus the concurrent-create approach. + - The `packages/core` row in `docs/ARCHITECTURE.md` now lists the existing modules and is marked "exists (partial)". + - `docs/TODO.md` gained three known gaps: search ignores overrides, rollup stops at three levels, and undoing a move does not move copies or list items back. + +Minors: +- `rejectSuggestion` throws `ValidationError("status","not_pending")` for a suggestion that is not pending. +- Engagement update and soft-delete throw `NotFoundError` when the parent entry is soft-deleted. +- Entry restore skips custom-field validation and only checks that the value is an object. + +Each has a test. + +## TDD evidence +- **RED:** ran the new and updated tests against the pre-fix source by stashing `packages/core/src`. + - `bun test packages/core` gave 10 failures: the move undo, restore custom fields, deleted-entry engagement, rejectSuggestion when not pending, copies and list items after accept, rollup grandchildren, orphans, non-ASCII pagination, merge into deleted, and merge text/rating/engagements. + - With `TEST_POSTGRES_URL=…`, the concurrent test failed on Postgres. + - The combined-sources test passed by coincidence at first, so I strengthened it (plugin source vs user destination) and re-ran it: 1 fail. +- **GREEN** (final): + - `bun test packages/core packages/db` (SQLite): 108 pass, 0 fail. + - The same with `TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres`: 207 pass, 0 fail. + - `bun run typecheck`: `tsc --noEmit -p tsconfig.json` with no errors. + - `git status` is clean. + +## Remaining concerns +- **Undoing a move is partial.** Copies and list items are not moved back, and aliases that the move re-pointed (X→D became X→S) are not restored. Both are recorded in TODO. +- **Restoring a merge's rating row** applies the source rating, by design of the "record in `before`" ruling. +- **Inconsistent DECISIONS.md order.** The new entry is at the top, as instructed ("newest first"), but Task 5 appended its entry at the bottom, so the file's order is mixed. + +--- + +# Fix round 2 + +**Status:** DONE +**Commit:** `c6865b2 fix(core): drill-down lists descendants under untracked intermediates` + +## What changed +- **Drill-down rule.** `listEntries({ parentId: X })` now uses `underParent(X)`, the top-level rule applied relative to X. It returns this user's live entries whose work is: + - a direct child of X; or + - a grandchild of X, when the parent between them (`p1`) carries no live entry of this user; or + - a great-grandchild of X, when neither `p1` nor `p2` carries a live entry of this user. + + It is portable SQL using `exists` / `not exists`. The other filters (`q`, `tag`, and so on) still apply. The top-level ancestor filter is used only when `parentId` is null. +- **Restore onto a deleted entry.** Restoring an engagement's `{entryId}` history row onto a soft-deleted entry now throws `ValidationError("entryId", "target entry is deleted")`. +- **Docs.** The hierarchy bullet in this branch's (still unmerged) DECISIONS entry now states the drill-down rule. + +## Tests +New assertions in `entries.test.ts` "listEntries rolls up children", using the show2 fixture: +- Drilling into the show lists both episodes. +- Drilling with a non-matching `q` returns nothing. +- After a season entry exists, drilling into the show lists only the season. +- Drilling into the season lists the episodes. + +New test in `engagements.test.ts`: "restoring a re-parenting onto a soft-deleted entry is rejected". + +## TDD evidence +- **RED** (`bun test packages/core`, before the fix): 99 pass, 2 fail. + - `(fail) entries [sqlite] > listEntries rolls up children` + - `(fail) engagements [sqlite] > restoring a re-parenting onto a soft-deleted entry is rejected` +- **GREEN:** + - `bun test packages/core packages/db`: 109 pass, 0 fail. + - With `TEST_POSTGRES_URL=…`: 209 pass, 0 fail. +- **Typecheck:** `bun run typecheck` reports no errors. +- **Worktree:** `git status` is clean. + +## Note +`childSummary` counts all descendant entries up to three levels. Once the season has an entry, the show's summary is 3 (the season plus 2 episodes), while drilling into the show lists only the season. That matches the ruling: the rollup counts everything under the show, and the drill-down shows the next tracked level. diff --git a/docs/plans/001-phase1-sdd/reports/task-7-report.md b/docs/plans/001-phase1-sdd/reports/task-7-report.md new file mode 100644 index 0000000..ea2a386 --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-7-report.md @@ -0,0 +1,10 @@ +# Task 7 report +Built packages/core/src/{crypto,attachments}.ts with tests in packages/core/test; TODO line added under "Next". +RED: `bun test packages/core/test` failed (cannot find ../src/crypto.ts). GREEN: SQLite 8 pass; Postgres 12 pass. Typecheck clean. +Decisions: each storeAttachment call creates a new attachment row + link (file deduped on disk); unlinkAttachment(db, dataDir, userId, attachmentId, entity, entityId) deletes the row when no links remain and the file when no row shares the sha; readAttachment returns null for non-owner/missing; HKDF salt "sttracker-v1", info "sttracker-credentials", 12-byte IV; malformed blob -> CryptoError. +Concerns: none. + +## Fix round 1 +Changes: per-sha in-process keyed mutex (withLock) held across store's exists/write/insert and unlink's delete-row/shared-check/rm; atomic write via `.tmp-` + rename (tmp removed on failure); image/svg+xml rejected, mime normalized (lowercase, params stripped) and stored normalized; CryptoError now gets `{ cause }`. +Tests added: tampered ciphertext -> CryptoError; IV is 12 bytes; svg/normalization; cross-user shared bytes survive other's unlink; foreign unlink no-op; listAttachments user-scoped; concurrent store/unlink; no temp files left. +Commands: `bun test packages/core/test/attachments.test.ts packages/core/test/crypto.test.ts` -> 16 pass 0 fail (SQLite); with TEST_POSTGRES_URL -> 26 pass 0 fail; `bun run typecheck` clean. diff --git a/docs/plans/001-phase1-sdd/reports/task-8-report.md b/docs/plans/001-phase1-sdd/reports/task-8-report.md new file mode 100644 index 0000000..f4c480f --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-8-report.md @@ -0,0 +1,136 @@ +# 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::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 `); 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 = `. 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. diff --git a/docs/plans/001-phase1-sdd/reports/task-9-report.md b/docs/plans/001-phase1-sdd/reports/task-9-report.md new file mode 100644 index 0000000..7baf9c2 --- /dev/null +++ b/docs/plans/001-phase1-sdd/reports/task-9-report.md @@ -0,0 +1,16 @@ +# Task 9 report +Status: DONE_WITH_CONCERNS. Branch task/9: 207c92b (merge task/8, docs conflicts kept both sides), 166b8ec feat(core): jobs, scheduler, plugin registry, connections. + +Built: jobs.ts (enqueue with lockKey + pg advisory lock, atomic claim, worker w/ backoff 2^attempts min, 5 attempts, scheduler with tickNow()), registry.ts (loadPlugins, reload with ?v=), connections.ts (CRUD, secret split into credentials_enc, makePluginContext, syncHandler). SDK `rateLimit` field; rateLimit set on trakt 100, hardcover 50, mangabaka 150, lastfm 240, listenbrainz 60; RATE_PER_MINUTE constants removed. Echo + broken fixtures. Docs: ARCHITECTURE, DECISIONS, TODO. + +Tests: tests 13 pass on SQLite, 24 on SQLite+Postgres (new files); full suite 395 pass with Postgres, 255 SQLite only. `bun run typecheck` clean. +TDD caveat: implementation was written before the tests were run, so no RED output was captured. + +Deviations: createConnection/updateConnection take manifest as trailing arg; makePluginContext is async and takes manifest (then optional http). syncHandler rethrows failed runs so the worker retries. ctx.log is a no-op (TODO). Stale `running` job reaper not built (TODO). Worktree had no node_modules; ran bun install --frozen-lockfile. + +## Fix round 1 +All findings addressed: scheduler picks non-paused connections; syncHandler drops missing/paused connections (DropJobError) and records failed run + connection error + NonRetryableError for missing plugin/CryptoError; runJob bookkeeping wrapped and logged, stop() never rejects, hourly stale-lock reaper each tick; reload test for edited entry file (needed `delete require.cache[file]` in addition to `?v=`, since Bun ignores the query for cached .ts); ctx.log persisted to source_runs.log after the run; credentials.save/updateConnection in one transaction; signal aborted on worker stop and threaded to context; rate-limit wiring test; mangabaka orphan comment removed; docs updated. Tests added for each. +Merged feat/phase1 (839d969), docs conflicts resolved keeping both sides. Results after merge: typecheck clean; `bun test` 275 pass (SQLite); with TEST_POSTGRES_URL 434 pass, 0 fail. + +## Fix round 2 +reapStale now skips this worker's in-flight job ids and takes `staleMs` (option, default 1h); a heartbeat (`heartbeatMs`, default 5 min) refreshes `locked_at` while a job runs; a job aborted by stop() is re-queued with attempts and run_at untouched. Tests: slow job runs once, heartbeat refresh, abort re-queue. diff --git a/docs/plans/001-phase1-sdd/task-15-fix1-findings.md b/docs/plans/001-phase1-sdd/task-15-fix1-findings.md new file mode 100644 index 0000000..ac7a2ce --- /dev/null +++ b/docs/plans/001-phase1-sdd/task-15-fix1-findings.md @@ -0,0 +1,51 @@ +# Task 15: fix round 1 findings (binding) + +File:line references are to `.worktrees/t15` at a047020. + +**I1. Aliases and tombstones are dropped (export.ts:260, 427-438).** +- Export `mergedIntoId` on works, plus the user's `work_aliases` rows, in a new `work_aliases.jsonl`. +- On import, after id remapping, restore `merged_into_id` (pointing at the remapped survivor) and the user's aliases. +- A private tombstone comes back as a tombstone, not a live duplicate. +- Bump nothing in the format version: these are additive files and keys. Document them in the DECISIONS entry. +- Test: per-user move D→S, then export/import into a fresh user. The alias exists, the draft is a tombstone, and a later `upsertEntry` on the old work resolves to the moved entry. + +**I2. Attachments are restored after the commit (export.ts:507-527).** +- Pre-validate every attachment (mime per the allowlist, size ≤ 25 MB, sha of the bytes equals the claimed sha) BEFORE opening the transaction. Any failure → `ImportError("invalid_attachment")`, with nothing written. +- Write the content-addressed files before the transaction (idempotent). +- Insert the attachment and attachment_links rows INSIDE the transaction with `tx`. +- Reuse attachments.ts helpers if they can take a handle. Otherwise add a small exported helper there, e.g. `writeAttachmentFile(dataDir, bytes)` returning the sha. You may touch attachments.ts for this. + +**I3. Export must read inside one transaction** (wrap all reads in `db.k.transaction().execute(async tx => …)` and use `tx`). On Postgres, use `isolation level repeatable read` if Kysely allows it (`setIsolationLevel("repeatable read")`). SQLite's transaction is already a snapshot. + +**I4. Round-trip test coverage.** Extend the seed and assertions to cover: +- copies with price and currency; +- custom field definitions and values (remapped); +- listens; +- a grouping list with rating and review; +- entry overrides and visibility; +- an engagement with year precision; +- an attachment with a caption, linked to two entities; +- a history row whose before/after holds a remapped UUID; +- alias and tombstone (I1); +- a SECOND user with a private work, an entry, and an attachment with the same bytes, none of which appear in user 1's bundle. + +Replace the `grep token` check with a check that no JSONL row contains keys named `password_hash`, `token_hash`, `credentials_enc` or `code_hash`, in any case form. + +**I5. Docs.** +- `docs/ARCHITECTURE.md`: add export.ts and backup.ts to the `packages/core` row and remove "export planned". +- `docs/TODO.md`: add the known gaps that remain (M1 sibling-position collision; zip-bomb size cap if not done). + +**Cheap minors (include):** +- **M3:** validate imported `rating` (an integer 0–100 or null), `status` (the Status enum) and `visibility` (private|friends|public). Otherwise `ImportError("corrupt_bundle")`. Cap the total uncompressed size at 2 GB by summing the entry sizes from `unzipSync`; over the cap → `ImportError("bundle_too_large")`. +- **M5:** + - backup files are created with mode 0600 and directories with 0700; + - write each backup into `.tmp`, then rename it to the final timestamp directory, so a failed run leaves no counted partial directory (remove the tmp on failure); + - chunk large `IN (…)` lists in batches of 500. +- **M1:** map a unique violation during import to `ImportError("conflict", …)` instead of a raw DB error. + +**Run** on both dialects: +- `bun test packages/core`; +- the same with `TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres`; +- `bun run typecheck`. + +Commit `fix(core): …`, append "Fix round 1" to task-15-report.md, and reply with the short contract. diff --git a/docs/plans/001-phase1-sdd/task-16-fix1-findings.md b/docs/plans/001-phase1-sdd/task-16-fix1-findings.md new file mode 100644 index 0000000..292d4ef --- /dev/null +++ b/docs/plans/001-phase1-sdd/task-16-fix1-findings.md @@ -0,0 +1,41 @@ +# Task 16: fix round 1 (binding) + +File:line references are to `.worktrees/t16` at 0a54020. Tasks 17 (apps/web) and 19 (Docker/docs) branch from your head and run in parallel, so keep the changes to apps/server, packages/core and packages/db. + +1. **Paused credential-less connections become unreachable.** + - Ruling: add migration `packages/db/src/migrations/0003_source_connections_deleted_at.ts`, a nullable `deleted_at` text column, with `down`. Register it and add it to `schema.ts`. + - `deleteConnection` sets `deleted_at` (and still wipes `credentials_enc` and pauses). + - `ownedRow`, `listConnections` (remove `isDeletedConnection` and the route-side filter), the scheduler due-query in jobs.ts and `syncHandler` (drop the job when `deleted_at` is set) all filter `deleted_at is null`. + - Test: pause a credential-less connection; it is still listed and can be unpaused. A deleted one is gone (404). + - (The schema.test.ts `migrateTo(NO_MIGRATIONS)` down-test must still pass.) + +2. **Token writes lose history provenance** (spec §6.9: the history actor is `user | plugin: | api token:`). + - Ruling: in core, the `field_sources` owner is derived from the actor: any actor not starting with `plugin:` is stored as `"user"` in `field_sources`, while history keeps the full actor. + - Apply this where `field_sources[key] = actor` is written (entries, engagements, copies, lists, and anywhere else; grep for it). + - `verifyAppToken`/`authenticate` expose the token name. API writes pass `token:` for Bearer requests and `"user"` for sessions. + - Tests: + - a token write shows `actor "token:"` in history and `"user"` in field_sources; + - a later sync doesn't overwrite it (pending changed). + - Update the DECISIONS entry that settled "all API writes as user". + +**Minors (include):** +3. **Login CSRF:** require `X-Requested-With: sttracker` on every non-Bearer, non-GET request, including `auth: "none"` routes such as login and register. Also require `content-type: application/json` on JSON-body routes (415 otherwise). Test both. +4. **Malformed cookie → 500:** wrap `decodeURIComponent` in `readCookie` and treat failure as no cookie. Test `stt_session=%` gives 401, not 500. +5. **Production by default:** `development` only when `NODE_ENV === "development"` or `Bun.argv` includes `--hot`. Document it in the TODO or README hand-off for Task 19. +6. **`DELETE me/tokens/:id`:** return 404 for an unknown or foreign id (check the affected rows). +7. **`listSuggestions`:** query by the resolved (canonical) work id. +8. **`admin/invites`:** add `admin: true` to the route. +11. **OpenAPI:** multipart routes document their field schema where a zod schema exists. +12. **Tests:** + - cookie plus bogus Bearer on a write gives 401; + - a non-admin calling `admin/plugins/reload` gets 403; + - a 50 MB+ multipart import gives 413; + - a positive test for a write-scope token; + - fix the attachment test to use a real other user's entry. + +**Run** both dialects: +- `bun test` (full suite); +- the same with `TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres`; +- `bun run typecheck`. + +Commit `fix(server): …` / `fix(core): …`, append "Fix round 1" to task-16-report.md, and reply with the short contract. diff --git a/docs/plans/001-phase1-sdd/task-16-rulings.md b/docs/plans/001-phase1-sdd/task-16-rulings.md new file mode 100644 index 0000000..c8b4007 --- /dev/null +++ b/docs/plans/001-phase1-sdd/task-16-rulings.md @@ -0,0 +1,42 @@ +# Task 16: controller rulings (binding, in addition to the brief) + +The core API already exists. Read the real signatures in `packages/core/src/**` (auth, works, matching, entries, engagements, lists, copies, custom-fields, attachments, crypto, export, backup, jobs, registry, connections, sync/{run,changes}) and in `packages/plugin-sdk/src/types.ts`. Several signatures differ from the original plan text. The code is authoritative. + +1. **Placeholder web.** If `apps/web/index.html` doesn't exist, create a minimal placeholder (`STTracker
STTracker is starting…
`). Task 17 replaces it. Serve it at `/*` via Bun HTML imports, with `/api/*` and `/ingest/*` handled first. + +2. **Secrets and connections in API responses:** + - Never return secrets, `credentials_enc`, token hashes or password hashes. + - `GET sources` returns `listConnections` output and must hide deleted connections, i.e. those with `status "paused"` and no credentials (Task 9 note). + - `POST sources` and `PATCH sources/:id` pass the plugin manifest from the registry into `createConnection`/`updateConnection`. + +3. **Running and importing:** + - `POST sources/:id/run` → `enqueue(db, "sync", { connectionId, mode? }, { lockKey: "sync:" })`. A null result → 409 `conflict` with the message "A sync for this source is already queued or running." + - `POST sources/:id/import` takes a multipart file and enqueues a sync job whose payload carries `{ connectionId, file: { name, base64 } }`. + - Size limit: 50 MB → 413 with error code `validation`. + - Make sure the sync handler (Task 9's `syncHandler`) supports file mode. If it doesn't, extend it minimally in `packages/core/src/connections.ts` (allowed). + +4. **Ownership of everything by id.** Every `:id` route verifies ownership through the core function, which throws `NotFoundError` → 404. For attachments, before `storeAttachment` verify that `entity`/`entityId` belongs to the user (Task 7 deferred this to the API layer). `GET attachments/:id` serves with `Content-Type` = the stored mime, plus `X-Content-Type-Options: nosniff` and `Content-Disposition: inline; filename="…"` (sanitized). + +5. **Error mapping:** + - `ValidationError` → 400 `validation`, with field and reason in the message. + - `AuthError` → 401 for `invalid_credentials`/`unauthenticated`, 403 for `forbidden`, 400 for `invite_required`/`username_taken`/`invalid_username`. + - `NotFoundError` → 404 `not_found`. + - `ImportError` → 400 with its code (409 for `account_not_empty` and `conflict`). + - Anything else → 500 `internal`, with a generic message; log the detail server-side, never return it. + +6. **Security basics:** + - Session cookie per the brief. + - CSRF: for cookie-authenticated non-GET requests, require the header `X-Requested-With: sttracker` (the web app sends it; Bearer-token requests are exempt). + - Login and register are rate-limited in-process: 10 per minute per IP, then 429. + - All JSON bodies are capped at 1 MB. + +7. **Boot (`main.ts`):** + - `createDb(DATABASE_URL)` → `migrateToLatest` → `loadPlugins(PLUGIN_DIRS)`; + - `startWorker(db, { sync: syncHandler(...), backup: backupHandler(...) })` and `startScheduler`; + - enqueue a daily `backup` job (lockKey "backup", runAt the next 03:00 local) and re-enqueue it after each run; + - SIGTERM/SIGINT → stop the worker and scheduler, then close the DB. + - The `DATA_DIR` default `./data`, created if missing. + +8. **OpenAPI** is generated from the route table with zod v4 `z.toJSONSchema`. Every route appears with method, path and summary, plus the request schema where there is one. + +9. **AGENTS.md Commands:** after verifying, set Dev to `bun --hot apps/server/src/main.ts` (needs `STTRACKER_SECRET`) and a Start command. Update `docs/PERMISSIONS.md` and the tool permission files (`.claude/settings.json`, `.codex/rules/project.rules`, `.gemini/settings.json`) only with commands you actually ran successfully. Update ARCHITECTURE (apps/server real) and DECISIONS (top: CSRF header, error mapping). diff --git a/docs/plans/001-phase1-sdd/task-17-fix1-findings.md b/docs/plans/001-phase1-sdd/task-17-fix1-findings.md new file mode 100644 index 0000000..30aa10d --- /dev/null +++ b/docs/plans/001-phase1-sdd/task-17-fix1-findings.md @@ -0,0 +1,47 @@ +# Task 17: fix round 1 (binding) + +File:line references are to `.worktrees/t17` at 439d138. **First** run `git merge feat/phase1`, which now holds Task 16's final server, including the fix round: the CSRF header on all non-Bearer writes, a 415 for non-JSON bodies, `deleted_at` on connections, and `token:` actors. Resolve the conflicts and re-run the tests. + +**Scope widened for this round:** you may edit `apps/server/src/routes/**` (the routes in item 0), `packages/core/src/**` only where needed for item 0, and the docs. + +0. **Ruling: three small API routes.** Each comes with API tests on both dialects. + - `GET /api/v1/auth/status` (auth none) → `{ hasUsers: boolean }`. `/register` uses it to show "Create the admin account" vs invite mode. + - `GET /api/v1/attachments?entity=&entityId=` (any:read) → owner-scoped `listAttachments`. Verify that the entity belongs to the user (404 otherwise). The detail Attachments box uses it instead of the session-only map. + - `GET/PATCH /api/v1/me/preferences` (session or any:write for PATCH) → `{ ratingScale: "5half"|"5"|"10"|"100", defaultVisibility: "private"|"friends"|"public" }`, stored in `users.preferences`, zod-validated. The UI reads the rating scale from it, not localStorage. `upsertEntry` already reads `defaultVisibility` from preferences; check the key name matches. + +1. **Lost updates on tags and custom fields** (EntryDetail.tsx:155,163; DetailBoxes.tsx:357). Update `["entry", id]` optimistically in `onMutate` (rollback in `onError`), and build each next value from the latest cache, not from the render's props. Add a logic test of the next-value builder: two quick tag adds keep both. + +2. **The review textarea goes stale after Restore** (DetailBoxes.tsx:509). Key it on `entry.review` (or make it controlled), so a restore refreshes it and a blur can't undo it. + +3. **Raw validation text in errors.** Add `friendlyError(err: ApiError): string` in `src/format.ts`: + - map `validation` field/reason pairs (`not_url`, `not_date`, `out_of_range`, `required`, `too_long`, password length, etc.) to plain sentence-case copy in the PRODUCT.md voice: what happened and how to fix it, no apology; + - map 413 to "That file is too large. The limit is 25 MB."; + - use it at every error display site. + + Tests for the mapping. + +4. **Gelasio.** Vendor the Gelasio woff2 (regular, italic and 600; latin subset is fine) plus its OFL text, next to Inter, and use it for the review. If you cannot obtain the font files offline, STOP and report NEEDS_CONTEXT rather than downloading from an unknown source. Fetching from Google Fonts' official GitHub (github.com/google/fonts, `ofl/gelasio`) is acceptable. + +**Minors (include):** +- Engagement delete: add `onError`. Sign out: catch, and still clear the cache and navigate. +- Engagement ordinals sort by `finishedAt ?? startedAt` (partial-date compare), then by `createdAt`. +- The engagement remove button is visible on touch: `@media (hover: none)` → opacity 1. +- The upload error uses `friendlyError` (413 → too large). +- `aria-label` on the Changes count and the status dot: use `role="img"`, or visually hidden text. +- Quick add keeps focus while pending: use `readOnly` + `aria-busy` instead of `disabled`, and refocus after success. +- Respect `mediaTypeOverride ?? work.mediaType` in row covers and History labels. +- De-duplicate `MONTHS` and the verb chain into format.ts. +- Drop the amber `caret-color` (use `--text`). +- Clear the module-level `savedFilters` and upload state on sign-out. +- History shows token actors by token name, not uuid (the API now emits `token:`). + +**Ratified:** dark text on `--warn` (AA contrast) and `--text-faint` group labels. Record both in DESIGN.md and DECISIONS. + +**Docs:** land the DECISIONS, TODO and ARCHITECTURE entries listed in your report §5 (DECISIONS newest-first) and update `docs/ARCHITECTURE.md`'s apps/web row. + +**Run:** +- `bun test` (full suite), with and without `TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres`; +- `bun run typecheck`; +- a quick CDP re-check: the restore → review flow, two quick tag adds, the Attachments list after reload, and /register on an instance that has users. + +Commit, append "Fix round 1" to task-17-report.md, and reply with the short contract. diff --git a/docs/plans/001-phase1-sdd/task-6-fix1-findings.md b/docs/plans/001-phase1-sdd/task-6-fix1-findings.md new file mode 100644 index 0000000..ef74684 --- /dev/null +++ b/docs/plans/001-phase1-sdd/task-6-fix1-findings.md @@ -0,0 +1,56 @@ +# Task 6: fix round 1 findings (binding) + +File:line references are to `.worktrees/t6` at c7471f7. + +1. **childSummary counts direct children only** (entries.ts:612-616). + - Count this user's live entries whose work has the entry's work as parent, grandparent or great-grandparent: a 3-way OR over joined `works` aliases, portable on both dialects. + - Add a grandchild case to the "listEntries rolls up children" test: show → season (no entry) → episode entries. + +2. **Default listEntries (`parentId: null`) hides orphaned child entries** (entries.ts:693). + - List entries whose work has no ancestor, up to depth 3, on which this user has a live entry. + - Test: an episode entry whose show has no entry appears at top level. Once the show has an entry, the episode disappears from the top level. + +3. **The title keyset cursor skips rows on SQLite for non-ASCII titles.** SQL orders by `lower(w.title)`, but the cursor uses JS `toLowerCase()` (entries.ts:711 vs :746). + - Select the SQL `lower(w.title)` as a column and put that exact value in the cursor. + - Add a non-ASCII pagination test, e.g. "Éclair", "Ölund", "Zeta", "alpha", "日本" with limit 1, paging through all. + +4. **Upsert race on unique (user_id, work_id) poisons the caller's transaction on Postgres** (entries.ts:384-404). + - Insert with `onConflict(oc => oc.columns(["user_id","work_id"]).doNothing())`, then re-read and apply the patch through the update path. Never catch and retry inside a transaction. + - Test: `Promise.all` of two `upsertEntry` calls for the same new (user, work) on both dialects yields one entry, with both patches applied. + +5. **Undoing a move leaves a reverse alias** (entries.ts:435-449 → moveEntry writes `src.workId → target`). Later syncs onto the shared work S silently land on the draft. + - The restore/undo path must move without writing an alias, and must delete the forward alias D→S it is undoing. + - Update history.test.ts:85: no alias remains after the undo. + - Test: after the undo, `upsertEntry(S)` creates or updates an entry on S, not on the draft. + +6. **The moveEntry merge path corrupts field_sources** (entries.ts:492-509). + - For combined fields (tags union, merged customFields/overrides), the merged `field_sources[key]` is `"user"` if either side's source for that key was `"user"`. Otherwise keep the destination's. + - For a revived soft-deleted destination, carry over the source's `field_sources` as-is. Do not stamp untouched keys with the actor. + - Tests for both. + +7. **The merge writes no history for moved engagements.** Conflicting non-empty source fields are dropped invisibly. + - Write a `user_history` row per moved engagement (`entry_id` before/after) and bump `updated_at`. + - When source and destination both have a non-empty `review` or `private_notes` that differ, append the source text to the destination's, separated by `\n\n---\n\n`, rather than dropping it. + - When ratings conflict, keep the destination's and record the source rating in the history row's `before`. + - Test. + +8. **Copies and list items on a moved draft work are detached.** + - Inside the move, re-point this user's `copies.work_id` and the `list_items.work_id` in this user's lists from the draft to the target. Write history for the copies. + - Test: after `acceptSuggestion`, `listCopies({workId: target})` includes the copy, and the list item shows the target's title. + +9. **Docs duties:** + - Append to `docs/DECISIONS.md` (newest first, existing format): merge-on-move semantics; alias bookkeeping, including that an undo removes the alias; the cursor format (carries the sort, uses SQL lower); whole-map customFields patches; list_items hard-delete with list history as the record; hierarchy visibility (rulings 1–2). + - Update `docs/ARCHITECTURE.md`'s Map row for `packages/core` from "planned" to list the modules that now exist (auth, works, matching, entries, engagements, history, lists, copies, custom-fields, attachments, crypto). + - First merge `feat/phase1` into your branch (`git merge feat/phase1`) so you see the current DECISIONS, ARCHITECTURE and TODO and resolve any conflicts. feat/phase1 now has Tasks 4, 5 and 7 merged. + +**Cheap minors (include):** +- `rejectSuggestion` only acts on a `pending` suggestion (otherwise `ValidationError`). +- `updateEngagement`/`softDeleteEngagement` throw `NotFoundError` when the parent entry is soft-deleted. +- Restoring an entry does not re-validate `customFields` against current definitions (restore is the user's own past data). + +**Run** on both dialects: +- `bun test packages/core packages/db`; +- the same with `TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres`; +- `bun run typecheck`. + +Commit in logical `fix(core): …` commits, append a "Fix round 1" section to task-6-report.md, and reply with the short contract. diff --git a/docs/plans/001-phase1-sdd/task-8-fix1-findings.md b/docs/plans/001-phase1-sdd/task-8-fix1-findings.md new file mode 100644 index 0000000..dddfa14 --- /dev/null +++ b/docs/plans/001-phase1-sdd/task-8-fix1-findings.md @@ -0,0 +1,53 @@ +# Task 8: fix round 1 (binding) + +File:line references are to `.worktrees/t8` at 1975e97. First run `git merge feat/phase1` in your worktree (Task 15 export and backup are now merged; there should be no conflicts in sync/**). + +## Review findings (Important) + +1. **Dedupe silently drops a second source's conflicting values** (apply.ts:460-471). + - After filling empty, unowned fields, run the remaining differing fields through `decideFields` against the duplicate engagement, so a disagreeing source files a `conflict` or pending `changed`, consistent with `addEntryRecord`. + - Test: A has rating 80, B dedupes with 90 → one pending conflict. +2. **A merged-away record files `removed` against the merge survivor** (apply.ts:565-586, run.ts:141-144). + - `markRemoved` must skip records with `merged_into_key` set: they are expected to vanish. No removed event for them, ever. + - Also don't file `removed` for a record whose pending `merged` event exists. + - Test: merge x→y is accepted, the next full pull lacks x, and no removed event is filed; the survivor is untouched. +3. **ID-less records leak a private draft on every retry** (apply.ts:386-388). + - Persist the `source_records` row with its matched work id (`target_entity "work"` or a dedicated field you already have, e.g. store the matched work id) in its own small committed step right after `matchWork`, before the apply transaction. + - On retry, reuse the stored work id instead of calling `matchWork` again. + - Test: a record whose apply throws once (inject a failure) → after two runs there is exactly one draft work. +4. **A user's per-user move-merge silently kills syncing** (apply.ts:220-245). + - In `moveEntryToWork` (packages/core/src/entries.ts) re-point `source_records` rows targeting the moved entry (and its engagements, if they move) to the surviving entry or engagements, inside the same transaction. You may edit entries.ts for this. + - Test: the user merges entry E1 into E2 via `moveEntryToWork`, then the source sends a change for the record that mapped to E1 → it applies to (or files an event for) E2, and is not dropped. +5. **The keep_mine test asserts nothing** (run.test.ts:315-318). After keep_mine, have B send a NEW value and assert a pending `changed` (user-owned field), not a `conflict`, with the rating still 80. +6. **Listens don't scale** (listens.ts:47-79). Per batch of 500: + - one `SELECT … source_key IN (…)`; + - resolve each distinct `musicbrainz_recording` once, outside the transaction (`createWork` has its own); + - then one transaction with a multi-row insert using `onConflict doNothing`; + - remove the per-row pre-check. + - Test: 1200 listens ingest correctly and idempotently. + +## Controller rulings on your open concerns (implement) + +7. **`list` records ARE applied in Phase 1.** + - A new record → a user list: kind `"list"`, name = the record's list name, description `"Imported from "`. Items are matched via `matchWork` (outside the transaction, as for entries). Map the record to the list via `source_records` (`target_entity "list"`). + - A changed record → name and description go through the per-field rules. Field ownership for lists: if the list was user-edited (any user_history row for it with actor "user"), file a pending `changed` with the whole incoming item set instead of applying. Otherwise replace the items. + - Removed → pending `removed`; accept soft-deletes the list. + - Tests: create, update, user-edited → pending, removed. +8. **Removing a multi-source engagement.** Accepting `removed` for an engagement that has other `engagement_sources` rows only deletes this connection's `engagement_sources` row. Soft-delete only when no sources remain. Test. +9. **Stale pending events are superseded.** When a newer incoming value arrives for the same (record, field) while an older event is still pending, set the old one's status to `"ignored"` with `payload.supersededBy = `, then create the new pending event. Test. + +## Minors (include, cheap) + +- `Object.hasOwn` for `action in STATUS_FOR` (changes.ts:79). +- Accept or keep_mine on an event whose target is gone (deleted, or merged away with no resolvable survivor) resolves it as `"ignored"` with `payload.reason = "target_gone"` instead of throwing NotFound. Follow merges to the survivor when possible. +- Run counts reflect only committed records: increment after commit. +- Add a test that a record-level error (not a pull error) skips removal. + +**Docs:** update the DECISIONS entry (top) and remove the TODO lines you've now fixed (lists, multi-source removal, superseding, listens batching). + +**Run** both dialects: +- `bun test packages/core`; +- the same with `TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres`; +- `bun run typecheck`. + +Commit in logical `fix(core): …` commits, append "Fix round 1" to task-8-report.md, and reply with the short contract. diff --git a/docs/plans/001-phase1-sdd/task-8-rulings.md b/docs/plans/001-phase1-sdd/task-8-rulings.md new file mode 100644 index 0000000..a04978b --- /dev/null +++ b/docs/plans/001-phase1-sdd/task-8-rulings.md @@ -0,0 +1,25 @@ +# Task 8: controller rulings (binding, in addition to the brief) + +These come from earlier tasks' reviews. Read the actual signatures in `packages/core/src/{works,matching,entries,engagements,history}.ts` before relying on the summaries below. + +1. **field_sources contract** (Task 6): `entries.field_sources` and `engagements.field_sources` are JSON maps from field name to the `Actor` that last set it. Plugin actor = `plugin::`. `upsertEntry`/`updateEngagement` re-stamp `field_sources` for EVERY key present in the patch. So the engine must send **only fields whose value actually changed** and that it is allowed to apply. Never echo unchanged values, or a plugin takes ownership of a user's field. + +2. **No reviving deletes.** If the target entry or engagement is soft-deleted (the user deleted it, or a removal was accepted), the engine must NOT revive it via `upsertEntry`/`updateEngagement` for a plugin actor. Record the record as seen. Changes to it produce nothing, or at most a pending `changed` event. Revival is a user action only. + +3. **Root ancestor entry** (hierarchy ruling): whenever the engine creates an entry on a child work (a work with `parent_id`), it also ensures the user has an entry on the ROOT ancestor work (follow `parent_id` to the top). If none exists, create it with `status "none"` and the plugin actor. Never modify an existing root entry for this purpose. + +4. **Map records to targets via `source_records`.** A record already mapped (`target_entity`/`target_id` set) must be updated through that mapping, not re-matched. ID-less hints (CSV) would otherwise create a new private draft on every sync. Only new records go through `matchWork`. + +5. **External-ID namespaces are type-specific** (`trakt_movie`, `tmdb_show`, `anilist_manga`, `isbn13`, …). Match generically. The only namespace the engine names itself is `musicbrainz_recording`, for listens. + +6. **Engagement dedupe** (brief rule 2) compares partial dates by precision. Day-precision values compare as dates. A datetime value's date part is its first 10 characters. Never convert through `Date` in a way that shifts timezone. + +7. **matchWork** already handles unique-violation retries in its own transaction, so do not call it inside an outer transaction that would be poisoned by a retry. Pattern per record: `matchWork` first (own transaction), then open the apply transaction and pass `tx` to the user-layer functions. + +8. **Inside any transaction use `tx`, never `db.k`.** SQLite runs on one connection behind a mutex, so `db.k` inside a transaction deadlocks. + +9. **Rate limits and secrets (Task 9's concern, not yours):** `runSync` receives a `makeContext` from the caller. Do not build http clients here. + +10. **Listens:** the `listens` unique key is (user_id, source, source_key). `source` = the plugin id, and `source_key` = the record key. + +11. **Docs duties:** update `docs/ARCHITECTURE.md` (the sync data flow is now real) and append to `docs/DECISIONS.md` any non-obvious choice: the hash composition, anti-flip-flop, no-revive, root-ancestor. Put new entries at the TOP, newest first. diff --git a/docs/plans/001-phase1-sdd/task-9-fix1-findings.md b/docs/plans/001-phase1-sdd/task-9-fix1-findings.md new file mode 100644 index 0000000..41c5163 --- /dev/null +++ b/docs/plans/001-phase1-sdd/task-9-fix1-findings.md @@ -0,0 +1,45 @@ +# Task 9: fix round 1 (binding) + +File:line references are to `.worktrees/t9` at 166b8ec. + +1. **A failed connection is never scheduled again** (jobs.ts:528). + - The scheduler selects connections with `status != 'paused'` (i.e. `active` or `error`), `schedule_minutes` set, and due. + - Test: an `error` connection that is due gets enqueued. + +2. **Failures before runSync loop invisibly forever** (connections.ts:338-371). Causes: a decrypt failure, a missing plugin, a deleted or paused connection. + - Ruling: `syncHandler` catches these, and the job does NOT retry: + - a missing or deleted connection: drop silently (the job is marked done with last_error); + - a missing plugin, or a CryptoError: insert a `source_runs` row (mode as requested, status `failed`, error message) and set the connection `status = 'error'`; + - advance `last_run_at` so the scheduler waits a full interval. + - Messages: + - CryptoError → `"Cannot decrypt this source's credentials. Was STTRACKER_SECRET changed? Reconnect the source to fix it."` + - missing plugin → `"Plugin '' is not installed or failed to load."` + - Signal non-retryable to the worker with a `NonRetryableError` class, exported from jobs.ts, that the worker marks `failed` immediately without backoff. + - Tests for each case. + +3. **An unhandled rejection can kill the process** (jobs.ts:445-465, 495). + - Wrap all bookkeeping in `runJob` in try/catch that logs via `console.error("[jobs]", …)`. + - Attach `.catch(log)` before adding to `inflight`. + - `stop()` never rejects. + - Also add a reaper: at `startWorker`, and on every tick, reset jobs with `status='running'` and `locked_at` older than 1 hour back to `queued` (attempts unchanged), so a crash never holds a lockKey forever. + - Tests: a handler whose bookkeeping update fails (inject one) doesn't reject `stop()`; a stale running job is re-queued. + +4. **Reload misses a plugin's non-index files** (plan-mandated limitation, verified on Bun 1.3.14). + - Ruling: keep `?v=` reload, but document it. Add a DECISIONS entry ("reload re-imports entry files only; changes in a plugin's other modules need a server restart") and a TODO line. + - Make the reload test assert that a change to the entry file IS picked up: write a temp plugin dir, reload, edit index.ts, reload, and see the new manifest version. + +**Minors (include):** +- Replace `.catch(() => {})` in the worker tick and scheduler with logging (`console.error("[jobs]", err)`). +- **Run-scoped context.** `syncHandler` passes a `makeContext(conn, runId)` that builds the context per run, so `ctx.log` appends to that run's `source_runs.log` (buffer, flushed by runSync's bookkeeping or appended with a cheap update; your call, but it must be persisted). +- `credentials.save` reads the CURRENT DB blob, merges and writes inside one transaction (`tx`). `updateConnection`'s read-modify-write also runs in one transaction. +- Abort `ctx.signal` when `stop()` is called on the worker (pass an AbortController into job handlers via a second argument, `handler(payload, { signal })`, and thread it into `makePluginContext`). +- Add a test that asserts the rate-limit wiring: makePluginContext with manifest `rateLimit {perMinute: 60, burst: 1}` and a fake clock or sleep spacing two calls. +- Remove the orphaned comment in plugins/mangabaka/src/api.ts:4. +- `docs/ARCHITECTURE.md` data flow: mark steps 1–2 as real. TODO: rate limiting is per run, not per plugin (spec §11 says per plugin; Phase 2). + +**Run** both dialects: +- `bun test packages/core plugins`; +- the same with `TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:55432/postgres`; +- `bun run typecheck`. + +Commit `fix(core): …`, append "Fix round 1" to task-9-report.md, and reply with the short contract. diff --git a/docs/plans/001-phase1-sdd/task-9-rulings.md b/docs/plans/001-phase1-sdd/task-9-rulings.md new file mode 100644 index 0000000..9ac71b2 --- /dev/null +++ b/docs/plans/001-phase1-sdd/task-9-rulings.md @@ -0,0 +1,29 @@ +# Task 9: controller rulings (binding, in addition to the brief) + +0. **Base.** Your branch task/9 starts at feat/phase1. Task 8 (the sync engine, which provides `runSync` in `packages/core/src/sync/run.ts`) is on branch `task/8`, still in a fix round. First run `git merge task/8`. Resolve the conflicts in docs/ARCHITECTURE.md, DECISIONS.md and TODO.md by keeping BOTH sides' content: DECISIONS newest-first, ARCHITECTURE combining both module lists. Commit the merge. Task 8 will change internals later, but `runSync(deps, plugin, connectionId, mode)` and `RunSummary` are stable. + +1. **Rate limits.** Add an optional `rateLimit?: { perMinute: number; burst?: number }` to `PluginManifest` in packages/plugin-sdk/src/types.ts. `makePluginContext` builds `ctx.http` with `createHttpClient({ ratePerMinute: manifest.rateLimit?.perMinute ?? 60, burst: manifest.rateLimit?.burst ?? 5, userAgent: "STTracker/0.1 (+self-hosted)" })`. Then set `rateLimit` on each built-in plugin manifest (plugins/*/index.ts): + - trakt: perMinute 100 + - hardcover: perMinute 50 + - mangabaka: perMinute 150 + - lastfm: perMinute 240 + - listenbrainz: perMinute 60 + - csv: none + + Remove the now-unused local `RATE_PER_MINUTE` constants if nothing uses them. + +2. **Secrets.** Config fields whose manifest type is `"secret"` are persisted ONLY inside the encrypted `credentials_enc` blob (`encryptJson` from `@sttracker/core/crypto`, with the instance secret), never in plain `config`. + - `createConnection`/`updateConnection` split the incoming config accordingly. + - `makePluginContext` merges the decrypted secrets back into `ctx.config`, and `ctx.credentials.get()` returns the decrypted blob (secrets plus any OAuth tokens a plugin saved). + - `credentials.save(c)` merges `c` into the blob and persists it encrypted IMMEDIATELY (Review focus 5). + - `listConnections` never returns secrets: config only, plus a `hasCredentials: boolean`. + +3. **Lock.** Sync jobs use `lockKey = "sync:"`. `enqueue` returns null if a queued or running job holds the same lockKey. The lock check and insert must be race-safe on Postgres: wrap them in a transaction with `pg_advisory_xact_lock(hashtext(lockKey))` on Postgres (SQLite is already serialized). + +4. **Job claim.** `update jobs set status='running', locked_at=… where id=? and status='queued'`, checking `numAffectedRows === 1n`. The dialect maps `numAffectedRows` for both databases. + +5. **Weekly full.** The sync handler runs `full` if the plugin lacks `pullChanges`, if the connection's last successful full run is older than 7 days (look at `source_runs` with mode `full` and status `ok`), or if the payload says `mode: "full"`. Otherwise `changes`. + +6. **Plugin loading** uses `import(pathToFileURL(file).href + "?v=" + Date.now())` for reload. The echo fixture plugin lives under packages/core/test/fixtures/plugins/echo/index.ts. A broken plugin fixture (a default export without a manifest) sits next to it. + +7. **Docs:** ARCHITECTURE (the job worker, registry and connections are real) and DECISIONS (top) for the rate-limit manifest field, the secret split and the lock.