John O'Keefe
ee98c0fb3b
Release / build-and-push (push) Successful in 3m29s
Integration tests require the containerized dev DB and bind-mounted paths — a bare host `go test ./cmd/server/tests` fails on database auth on every machine not running the compose stack (the old doc prescribed exactly that flow). development.md now documents the container runner and the iteration rule: run ONLY your own tests via a filtered container invocation while working, and reserve full-suite runs for pre-commit validation — several tests read live shared state (system_settings, real library folders, the periodic backup scan's websocket broadcasts) and flap on data or timing a change never touched. Binary media fixtures are never committed: internal/services/ testdata/ is gitignored by policy (the audio metadata tests read real-format binaries that ffmpeg cannot synthesize — Audible freeform atoms — so they live untracked on the machines that run the suite). The new "Media fixtures" section carries the ffmpeg recipes for every supported container, tag writing, embedded covers and ffmetadata chapters, plus the manual end-to-end recipe for the live system (one book per subfolder — a directory with ≥2 audio files ingests as one folder book).
195 lines
11 KiB
Markdown
195 lines
11 KiB
Markdown
# AGENTS.md — Bookhoard server
|
||
|
||
Standing rules for any agent or developer working in this repo.
|
||
Project-wide context lives in the sibling repos' AGENTS.md files
|
||
(../bookhoard-app/AGENTS.md is the Android client's).
|
||
|
||
## Rendering ownership (templ vs TypeScript) — ALWAYS apply
|
||
|
||
The point of templ + htmx + Alpine is: the server renders the UI, and
|
||
JavaScript is kept minimal. The rule that keeps it that way:
|
||
|
||
> **JavaScript decides what shows. templ decides what it looks like.**
|
||
|
||
- Content derived from **server data** (DB rows, search results, match
|
||
lists, conflict cards, any list/card/badge/table) is rendered by
|
||
templ. When it changes client-side, fetch a **templ partial** from an
|
||
SSR route and swap it in (`htmx.ajax(...)`), never rebuild it from a
|
||
JSON payload with `innerHTML` string templates.
|
||
- **Pure functions of client-local state** (open/closed panels, tabs,
|
||
password strength from a typed value, toasts) may render in
|
||
TS/Alpine — no server round trip needed.
|
||
- `web/src` is **TypeScript only** — no new `.js` source files.
|
||
Vendored third-party and vite build output under `web/static` are
|
||
exempt.
|
||
- Inline `<script>` in a `.templ` file needs a reason a bundled module
|
||
cannot do the job (pre-paint execution). Currently sanctioned:
|
||
`theme_flash_guard.templ` only.
|
||
|
||
Sanctioned JS islands (engines, not UI renderers — exempt from the
|
||
above): the reader (foliate-js + `web/src/reader/`), Chart.js graphs in
|
||
stats, toasts, password validation. Anything else that renders app UI
|
||
in TypeScript is a defect; if a templ twin of it exists, one of the two
|
||
must be deleted.
|
||
|
||
## Library visibility (family-lens rule) — ALWAYS apply
|
||
|
||
Per-user library visibility (`library_visibility` table, default-ALLOW:
|
||
every user sees every library unless an admin hides one) must be
|
||
enforced on **every endpoint that returns media metadata or content**.
|
||
It is NOT automatic — enforcement is per-handler.
|
||
|
||
- **New endpoints that list/filter media or series** MUST filter by the
|
||
requesting user's visible-library set, and MUST reject an explicit
|
||
`library_id` outside that set with 403 (never silently pass).
|
||
- Helpers (do not hand-roll):
|
||
- `handlers`: `(*MediaHandler).visibleLibrarySet` /
|
||
`(*SeriesHandler).visibleLibrarySet` — membership map + id slice.
|
||
- `router`: `visibleLibraryIDsFor` — same for SSR routes.
|
||
- `resolveLibrary` (helpers.go) only honors requested library ids
|
||
inside the visible set.
|
||
- SQL pattern: `AND (sqlc.narg('visible_library_ids')::uuid[] IS NULL OR
|
||
library_id = ANY(sqlc.narg('visible_library_ids')::uuid[]))` — see
|
||
`ListMediaItems`, `GetDistinctSeries`, `SearchMediaItems`.
|
||
- Content endpoints (file bytes, page images) additionally gate per item
|
||
like `ReaderHandler.ensureItemVisible` / the download endpoint.
|
||
- Tests must pin visibility: hidden library → 403 on the endpoint, and
|
||
absent from unscoped aggregates.
|
||
|
||
History: 2026-10-13 (`431fec0`) the series/media listing surfaces
|
||
leaked hidden libraries to any authenticated account — series endpoints
|
||
were name-only across ALL libraries. Fixed; do not regress.
|
||
|
||
## Tests
|
||
|
||
- Use `setupTestServer()` from `cmd/server/tests/test_helpers_test.go`
|
||
(see the Conventions section below for the pool/cleanup rules).
|
||
- Integration tests run against the SHARED dev database: never write
|
||
destructive SQL without preserving the dev rows (libraries Manga/
|
||
Comics/Ebooks, testuser's annotations and progress are real data).
|
||
- Test-created users must use `@tests.bookhoard.internal` or
|
||
`@example.com` emails and "test"-named collections/libraries so the
|
||
harness cleanup can find them.
|
||
- **NEVER rename or edit the shared admin (`testuser`) in tests** — a
|
||
renamed admin escapes the cleanup preserve-list and cascades real
|
||
data (this wiped the dev annotations once; see user_test.go
|
||
TestUpdateProfileSelfEdit for the dedicated-user pattern).
|
||
- **`testuser` is a LIVE reader**: the dev account genuinely reads in
|
||
the web reader while the suite runs, so `reading_history` /
|
||
`reading_progress` rows accrue on it mid-test. Exact-total
|
||
assertions against that account flap (three stats/analytics tests
|
||
broke on 2026-10-14 within hours of session capture shipping).
|
||
Tests that need deterministic data use `setup.RegularToken` +
|
||
`getRegularUserID` (the fresh per-test user), or assert ranges
|
||
instead of equals. Related: **no table-wide DELETEs in test
|
||
cleanups** — sync_integration's `DELETE FROM reading_history WHERE
|
||
true` destroyed the live reader's stats data on every suite run
|
||
until scoped to the test-sync users (cascade via user delete).
|
||
- **RUN ONLY YOUR OWN TESTS while iterating**: integration tests
|
||
require the containerized dev DB — a bare host `go test
|
||
./cmd/server/tests` fails on auth on every machine not running the
|
||
compose stack. Use the filtered container runner
|
||
(`docker compose -f docker-compose.yml -f
|
||
docker-compose.dev.yml --profile tests run --rm tests go test
|
||
./cmd/server/tests -run 'TestYourPrefix' -v`) and save full-suite
|
||
runs for pre-commit validation; several tests read live shared
|
||
state (system_settings, real library folders, the periodic backup
|
||
scan's websocket broadcasts) and flap on data/timing your change
|
||
never touched. Full recipes: docs/contributing/development.md
|
||
→ Testing.
|
||
- **NEVER COMMIT BINARY MEDIA** (mp3/m4b/flac/... fixtures):
|
||
`internal/services/testdata/` is gitignored by policy; generate
|
||
media at test runtime with ffmpeg instead (test images ship it).
|
||
ffmpeg cannot write Audible freeform atoms — those paths are unit-
|
||
tested with synthetic ffprobeReport values. Recipes:
|
||
docs/contributing/development.md → Media fixtures.
|
||
|
||
## Timezones
|
||
|
||
- The server owns display conversion: clients send instants (RFC3339),
|
||
the server renders in the requesting user's timezone.
|
||
- Use `utils.FormatTimeInTimezone` / `utils.UserLocation` — never
|
||
`time.Now().Format(...)` bare (that is server-UTC).
|
||
- Analytics day-buckets must bucket in the user's timezone
|
||
(`CreatedAt.In(loc)`), and date-range end bounds must be end-of-day.
|
||
|
||
## Debug discipline (test-failure triage) — 2026-10-17
|
||
|
||
When a test or bruno request fails, suspect the TEST FIRST — most
|
||
failures in this repo are harness/schema/environment drift, not server
|
||
bugs (2026-10-17 bruno sweep: drifted body keys, bodies pre-truncated
|
||
at their first `{{var}}`, missing `/api` prefixes; host runs hitting
|
||
container-only paths/hostnames). Triage order: (1) read the
|
||
test/fixture/payload schema and diff against a known-good sibling
|
||
(bruno: the `NewDevDBSetup` files are the reference for `body.data`
|
||
form); (2) check environment assumptions (host vs container,
|
||
harness cleanup, live-DB traffic); (3) cheap probe (curl, one SQL
|
||
query) before any deep dive.
|
||
|
||
But test changes are ASYMMETRIC with code changes: server code may be
|
||
fixed freely when it is the bug; test/contract edits may only
|
||
(a) fix harness plumbing, (b) WIDEN pins additively with a
|
||
`docs/contract_pins.md` changelog entry, (c) delete tests of deleted
|
||
behavior — never narrow an assertion or flip an expectation to match
|
||
observed behavior. Contract pins were green at a known commit, so git
|
||
history arbitrates "test stale vs code changed." If a test looks right
|
||
but fails and no harness bug explains it, that is prima facie evidence
|
||
of a server bug: stop and flag the user.
|
||
|
||
## Known gotchas (do not relearn)
|
||
|
||
1. **templ does NOT interpolate `{ expr }` inside quoted attribute
|
||
values** — it emits the literal text. `x-init="f('{ col.ID }')"`
|
||
shipped a request to `/api/collections/{ collection.ID }` found by
|
||
the request logs; the whole attribute value must BE an expression:
|
||
`x-init={ "f('" + col.ID + "')" }`. (Every occurrence of this bug —
|
||
including `class="theme-{ user.Theme }"` on 22 page bodies and 26
|
||
attribute interpolations — was swept and fixed in 2026-10; when
|
||
adding attributes with dynamic values, the whole value is an
|
||
expression, and free text goes through `templates.JSONArg`.)
|
||
2. **SettingsRegistry caches**: writes must go through the settings
|
||
API (Reload on save). Direct SQL writes to system_settings leave
|
||
the cache stale until the next API-path write — never "preview"
|
||
rule changes via psql and expect them applied (2026-10-14: a live
|
||
curl test flip looked ignored until the API path was used).
|
||
3. **User collections are not dashboard sections**: nothing sets
|
||
collection show_on_dashboard for user rows, and GetDashboardSections
|
||
skips empty user sections. Collection detail (JSON + SSR) must NOT
|
||
route user rows through the dashboard-section path — a 7ce7321
|
||
regression did exactly that (static detail without library_id
|
||
returned zero books until 0a7fbd6). Only real system query types
|
||
(continue-reading etc.) route there; `query_type='filter'` is the
|
||
schema DEFAULT on every user row, not a query type.
|
||
4. **End-of-day arithmetic**: anchor the END bound to the START of the
|
||
day, never to NOW(). `now.AddDate(0,0,1) − 1ns` carries this
|
||
instant's clock time into tomorrow — a range "ending today"
|
||
resolves tomorrow's calendar date and today's rows land in
|
||
yesterday's bucket (found twice now: analytics 1601548 and stats
|
||
2026-10-14). Same trap, two deaths.
|
||
5. **Test binaries run against the LIVE dev DB**: hand-probes made in
|
||
psql during debugging (seed rows, fake sessions) will silently
|
||
pollute the next integration run — DELETE them when done (this
|
||
burned the first stats suite run: a stray 30-min probe row shipped
|
||
a phantom "185 minutes" failure).
|
||
6. **SaveProgressRequest.DeviceID is NOT always a device**: the
|
||
web/app progress paths have historically stuffed the USER id into
|
||
it (it only fed a broadcast label before stats). Anything that
|
||
dereferences it (reading_history device_id FK, device joins) must
|
||
probe DeviceExistsForUser first — stats capture does.
|
||
|
||
## Conventions (absorbed from the retired PROJECT_GUIDELINES.md —
|
||
## only the rules that still matched reality)
|
||
|
||
- **Runtime is Docker** (Makefile auto-detects docker, falls back to
|
||
podman). The old "podman only" rule is dead.
|
||
- **The server binary is only built inside the Docker build**
|
||
(`make rebuild-app`); host-side Go builds are for tests/vet only.
|
||
- **Schema changes go in place** — edit `database/schema/schema.sql`,
|
||
never create migration files, until a release splits them.
|
||
- **Business logic lives in services**, not handlers; handlers parse
|
||
requests and call services (all DB access goes through services).
|
||
- **Integration tests**: one `setupTestServer()` per test function;
|
||
test pools use `max_conns=1`; never create a `pgxpool` per subtest
|
||
(connection exhaustion); use `t.Cleanup()`-based helpers; table-driven
|
||
subtests over duplicate functions.
|