John O'Keefe
de9e66636a
2026-10-17 lessons from the hardening session: most test/bruno failures were harness/schema/environment drift (drifted body keys, truncated payloads, container-only paths), found by triaging the test before the server. Codified as: triage order (test schema -> environment -> cheap probe), with the guard against "writing tests to work" — test/contract edits are asymmetric with code fixes: only harness plumbing fixes, additive pin widenings with a changelog entry, or deletions of tests of removed behavior. Pins were green at a known commit; git history arbitrates stale-vs-changed. A right-looking test that fails is prima facie a server bug: stop and flag. contract_pins.md rule 4: narrowing/removing a pinned key requires the user's explicit sign-off — a pin freezing buggy behavior is a ledger, not a license.
147 lines
7.9 KiB
Markdown
147 lines
7.9 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).
|
||
|
||
## 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).
|
||
|
||
## 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 + "')" }`. Note
|
||
`class="theme-{ user.Theme }"` has always been literal (harmlessly
|
||
masked by the JS theme engine).
|
||
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.
|