Conversation
Move db/client.ts to db/internal/client.ts and add a typed barrel db/index.ts that re-exports only the DB type. After this commit, the db value is only importable from db/internal/client.js — every other file imports the type from db/index.js. The compiler now rejects any future raw db.query outside db/ or repositories/. Add roleExists(db, roleId) to db/role-queries.ts and extend createUser with an optional roleId. Migrate the four raw db.query sites in routes/users.ts (lines 158, 170, 237, 321) to call the new query functions instead. routes/users.test.ts mocks the new functions; behavior assertions survive. - 14 db/*-queries.ts files now import only type DB from ./index.js - db/schema.ts is the only file in db/ that imports the db value (from ./internal/client.js, for bootstrap-time migration runs) - index.ts (server bootstrap) imports the db value from ./db/internal/client.js to set up DI; tests outside db/ import from db/internal/client.js to mock - TDD: roleExists and createUser(roleId) added RED→GREEN with unit tests in db/role-queries.test.ts and db/user-queries.test.ts Refs #1192 Co-Authored-By: Claude <noreply@anthropic.com>
… PUT role roundtrips - createUser now returns UserPublic (no password_hash, no is_system_role) in its RETURNING clause, eliminating the field-level leak at the source. The TypeScript cast in the route is now truthful instead of hiding a runtime leak that the mock-based test could not catch. - Add getRoleNameById helper and use it in PUT /api/users/:id/role to drop the redundant roleExists + getRoleById pair (which also fetched permissions just to read the role name). Single cheap query serves as both existence check and demotion-guard comparison. - Regression test for POST asserts password_hash / is_system_role / scrypt: do not appear in the response body.
Establishes the repositories/ directory pattern from #1192. getRoleInUseCount(db, roleId) queries users.role_id counts and parses the result, replacing the raw db.query that previously lived in routes/roles.ts:215. - repositories/role-repository.ts (new): exports getRoleInUseCount with the // fallow-ignore-file security-sink annotation, matching the convention of every db/*-queries.ts module. - repositories/role-repository.test.ts (new): 3 unit tests (happy path, zero count, empty rows) using the mockDb pattern from db/role-queries.test.ts. Refs #1192 Refs #1194
…eleteRole Replaces the two raw db.query sites in routes/roles.ts:215,224 with the new repositories/role-repository.ts.getRoleInUseCount and the existing db/role-queries.ts.deleteRole. The route's 204/403/404/409 behavior is preserved. - routes/roles.ts DELETE handler now uses getRoleInUseCount + deleteRole; uses deleteRole's boolean return to surface a TOCTOU race as a 404 (the original raw DELETE was fire-and-forget). - routes/roles.test.ts mocks the new repository module via vi.mock; the 4 existing DELETE test cases are updated to the new mock shape, plus 1 new test for the deleteRole-returns-false 404 branch and 1 positive-assertion test confirming the route calls getRoleInUseCount(db, roleId) and deleteRole(db, roleId) instead of touching db.query directly. Closes #1194 Refs #1192 Follow-ups filed as separate issues (out of scope for this slice): - #1200 deleteRole inner SELECT is redundant when caller already fetched - #1201 getRoleInUseCount silently collapses malformed count to 0
parseStrictInt is documented for user-supplied IDs and pagination inputs. COUNT(*) is a server-returned aggregate — always a valid numeric string — so parseInt(..., 10) is the correct decoder. Also removes the unused parseStrictInt import and the overly broad fallow-ignore-file security-sink suppression (a parameterised SELECT COUNT has no security-sink surface). Refs #1194
…1204) ## Summary Closes #1194 (slice 2 of #1192). The DELETE /api/roles/:id route previously held two raw `db.query` sites: a cross-table `SELECT COUNT(*) FROM users WHERE role_id = \$1` and a `DELETE FROM roles WHERE id = \$1`. This slice moves both behind the new `repositories/` seam while keeping the route's HTTP semantics intact. ## Why this PR targets `feat/1192-extend-db-seam-repos-errors`, not `develop` Per issue #1192's "Single PR, full scope" implementation decision, all slices of #1192 accumulate into one PR that ultimately merges the feature branch into `develop`. Slice PRs therefore target the feature branch, not `develop` directly. Slice 1 (PR #1199) was the special exception — it had to land first to seed the typed barrel that every later slice depends on. This PR (slice 2) targets the feature branch so slice 2's work joins the cumulative PR that will eventually close #1192. ## Changes (2 commits) ### `feat(repos): add role-repository.getRoleInUseCount` (`d3ba2bc`) - New module `server/src/repositories/role-repository.ts` exporting `getRoleInUseCount(db, roleId): Promise<number>` - `fallow-ignore-file security-sink` on the file — the SQL is parameterized via `$1` - New `server/src/repositories/role-repository.test.ts` with 3 cases: - returns the count when users are assigned - returns 0 when no users are assigned - returns 0 when the query returns no rows ### `refactor(server/routes): DELETE /api/roles/:id uses repositories/ + deleteRole` (`7d48f68`) - `routes/roles.ts` DELETE handler: - `db.query<{count:string}>(...)` + `parseStrictInt(...)` replaced with `getRoleInUseCount(db, roleId)` - `db.query('DELETE FROM roles WHERE id = \$1', [roleId])` replaced with `deleteRole(db, roleId)` (existing `db/role-queries.ts` function) - Route now `return next(new NotFoundError('Role not found'))` when `deleteRole` reports the row vanished between `getRoleById` and the DELETE (race window) - `routes/roles.test.ts` mocks `../repositories/role-repository.js`; behavior assertions for 204/403/404/409 survive verbatim - Two new tests added: - "should return 404 when deleteRole reports the role no longer exists" (race) - "should route the delete through deleteRole and getRoleInUseCount" (seam assertion) ## Acceptance criteria (per #1194) - [x] `repositories/role-repository.ts` exists and exports `getRoleInUseCount(db, roleId): Promise<number>` - [x] `routes/roles.ts:215,224` use `getRoleInUseCount` and `deleteRole` instead of raw SQL - [x] `repositories/role-repository.test.ts` covers the count (3 cases) - [x] `routes/roles.test.ts` mocks the new repository; behavior assertions survive - [x] `cd server && npx tsc --noEmit` clean - [x] `npm run test:run --workspace=allo-scrapper-server` passes (840/840) - [x] `cd server && npm run test:coverage` passes the coverage gate (98.87% statements) ## Notes - `repositories/` is a new directory. The architectural intent (per #1192): `db/` owns per-table SQL, `repositories/` owns business-shaped operations (cross-table queries, business policy). This PR establishes the `repositories/` pattern with a minimal slice; slice 5 (#1197) reuses the same shape for the larger refresh-token move. - `deleteRole` (existing in `db/role-queries.ts`) is unchanged. It still returns `false` for both "not found" and `is_system=true` — the route handles `is_system` via `getRoleById` before reaching `deleteRole`, so the dual false is harmless in practice. ## Related - Parent: #1192 - Slice 1: #1193 / PR #1199 (already merged — typed barrel + `routes/users.ts` migration; the typed barrel makes this PR's `repositories/` imports type-safe) - Sibling slices: #1195 (slice 3), #1196 (slice 4), #1197 (slice 5), #1198 (slice 6) - Follow-up: #1200 (deleteRole redundant inner SELECT — separate PR after slice 6 lands)
Add getUserWithRoleById(db, userId) to db/user-queries.ts, returning
{id, username, role_id, role_name, is_system_role} via a single users JOIN
roles query (r.is_system aliased as is_system_role, matching the existing
getUserByUsername pattern).
Rewire POST /api/auth/refresh to call getUserWithRoleById instead of the
raw inline SQL JOIN. The previous query referenced r.is_system_role, a
column that does not exist in the roles table; the function uses the
correct r.is_system alias.
Cover with unit tests for the new query (row shape, SQL shape, parameter
binding, undefined on miss, exclusion of password_hash/created_at) and
adapt routes/auth.test.ts to mock getUserWithRoleById. Adds a new test
for the 401 'User not found' branch the function enables.
Closes #1195, closes #1194 ## What Replaces the raw SQL JOIN in the token-refresh handler with the new `getUserWithRoleById` query from `db/user-queries.ts`. ## Changes - Added `getUserWithRoleById(db, userId)` to `db/user-queries.ts` returning `{ id, username, role_id, role_name, is_system_role }` - `routes/auth.ts` refresh handler uses `getUserWithRoleById` instead of raw JOIN - Added test coverage in `db/user-queries.test.ts` and `routes/auth.test.ts` ## Acceptance criteria - [x] `db/user-queries.ts` exports `getUserWithRoleById(db, userId)` returning the required row shape - [x] `routes/auth.ts` uses `getUserWithRoleById` instead of the raw JOIN - [x] `db/user-queries.test.ts` covers `getUserWithRoleById` - [x] `routes/auth.test.ts` mocks it; behavior assertions survive - [x] `cd server && npx tsc --noEmit` clean - [x] `npm run test:run --workspace=allo-scrapper-server` passes - [x] `cd server && npm run test:coverage` passes the coverage gate
- Add getActiveScrapeJobsCount to new db/system-stat-queries.ts - Add getLastCompletedScrapeAt to db/report-queries.ts - Add getTheaterCount to db/theater-queries.ts - Replace 3 raw db.query calls in services/system-info.ts - Run 3 queries concurrently via Promise.all - Update system-info.test.ts to mock query functions instead of raw db.query
…1207) Closes #1196 ## Summary Slice 4 of #1192: migrate the three remaining raw `db.query` calls in `services/system-info.ts` to named query functions, completing the `db/` seam for the system-info service. ## What was done ### New query functions | Function | Module | SQL it replaces | |---|---|---| | `getActiveScrapeJobsCount(db)` → `Promise<number>` | `db/system-stat-queries.ts` (new) | `SELECT COUNT(*) FROM pg_stat_activity WHERE state = 'active' AND query LIKE '%scrape%'` | | `getLastCompletedScrapeAt(db)` → `Promise<Date | null>` | `db/report-queries.ts` | `SELECT MAX(completed_at) FROM scrape_reports WHERE status = 'completed'` | | `getTheaterCount(db)` → `Promise<number>` | `db/theater-queries.ts` | `SELECT COUNT(*) FROM theaters` | ### Refactored service `services/system-info.ts` — `getScraperStatus` now delegates to the three named functions instead of executing raw inline SQL. The three independent queries run concurrently via `Promise.all` (semantically safe — they read unrelated tables). ### Tests (TDD: RED → GREEN per function) | Test file | New tests | Coverage | |---|---|---| | `db/system-stat-queries.test.ts` (new) | 3 | Positive count, zero count, missing row fallback | | `db/theater-queries.test.ts` | 3 | Positive count, zero count, missing row fallback | | `db/report-queries.test.ts` | 3 | Valid timestamp, null timestamp, empty rows | | `services/system-info.test.ts` | 5 (updated) | Existing behavior assertions preserved with `vi.mock()` on the new query functions instead of raw `db.query` mocks | ### Acceptance criteria - [x] `db/system-stat-queries.ts` exists and exports `getActiveScrapeJobsCount` - [x] `db/theater-queries.ts` exports `getTheaterCount` - [x] `db/report-queries.ts` exports `getLastCompletedScrapeAt` - [x] `services/system-info.ts` uses the new functions instead of raw SQL (lines 109-124 replaced) - [x] `db/system-stat-queries.test.ts` covers `getActiveScrapeJobsCount` - [x] `db/theater-queries.test.ts` covers `getTheaterCount` - [x] `db/report-queries.test.ts` covers `getLastCompletedScrapeAt` - [x] `services/system-info.test.ts` mocks the new query functions; behavior assertions survive - [x] `npx tsc --noEmit` clean (server + scraper) - [x] `npm run test:run --workspace=allo-scrapper-server` passes (854 tests, 57 files) - [x] `npm run test:coverage` passes coverage gate (server) ### Verification ``` Server: 57 test files | 854 tests passed | coverage 98.86% statements / 92.89% branches Scraper: 19 test files | 223 tests passed ``` No behavioral changes. No regressions. ## Parent #1192 — extend DB seam / repositories layer ## Sibling slices | Slice | Issue | Status | Description | |-------|-------|--------|-------------| | 1 | #1193 | merged | Typed barrel + migrate `routes/users.ts` | | 2 | #1194 | merged | Role-deletion route uses `repositories/` | | 3 | #1195 | merged | Token-refresh uses `getUserWithRoleById` | | **4** | **#1196** | **→ this PR** | **system-info service uses named queries** | | 5 | #1197 | pending | Larger refresh-token move | | 6 | #1198 | pending | TBD |
…s/ seam (#1197) Create db/refresh-token-queries.ts with 6 raw SQL operations (insertRefreshToken, findByTokenHash, revokeByTokenHash, revokeByUserId, cleanupExpired, rotateRefreshToken). Create repositories/refresh-token-repository.ts with 6 business functions (generateRefreshToken, validateRefreshToken, revokeRefreshToken, rotateRefreshToken, revokeAllUserTokens, cleanupExpiredTokens) plus parseRefreshTokenExpiry. Delete the old services/refresh-token-service.ts class and migrate routes/auth.ts to import from the new repository. Add 30 tests (9 db-queries + 21 repository).
…s/ seam (#1197) (#1208) ### Summary Moves the refresh-token feature from `services/refresh-token-service.ts` (a class) into the two-tier seam established in the parent PR `#1192`: | Tier | Module | Role | |---|---|---| | **db/** | `refresh-token-queries.ts` | Raw SQL — 6 single-table operations | | **repositories/** | `refresh-token-repository.ts` | Business logic — hashing, expiry, logging, atomic rotate | ### What changed **New — `db/refresh-token-queries.ts`** (123 lines, 9 tests) - `insertRefreshToken` — INSERT a token hash - `findByTokenHash` — SELECT by SHA-256 hash - `revokeByTokenHash` — UPDATE revoke a single token - `revokeByUserId` — UPDATE revoke all tokens for a user - `cleanupExpired` — DELETE expired/old-revoked tokens, returns count - `rotateRefreshToken` — atomic UPDATE+INSERT inside `db.transaction`, throws on consumed token **New — `repositories/refresh-token-repository.ts`** (150 lines, 21 tests) - `parseRefreshTokenExpiry()` — reads `REFRESH_TOKEN_EXPIRY` env var (byte-identical to old impl) - `generateRefreshToken(db, userId, expiryMs?)` — generates 384-bit random token, stores SHA-256 hash - `validateRefreshToken(db, rawToken)` — looks up by hash, checks revoked/expired, returns userId or null - `revokeRefreshToken(db, rawToken)` — revokes by hash - `rotateRefreshToken(db, userId, oldToken, expiryMs?)` — atomic revoke-old + insert-new - `revokeAllUserTokens(db, userId)` — revokes all active tokens for a user - `cleanupExpiredTokens(db, retentionDays?)` — delegates to db layer, logs count **Modified — `routes/auth.ts`** (+28/-23) - Replaced `new RefreshTokenService(db)` class instantiation with direct function calls - All four endpoints updated: login, change-password, refresh, logout - Import path: `../repositories/refresh-token-repository.js` **Modified — `routes/auth.test.ts`** (+28/-35) - Mock updated from class constructor pattern to named function exports - `rotateRefreshToken` assertion updated to include `db` as first argument **Deleted** - `services/refresh-token-service.ts` (191 lines) - `services/refresh-token-service.test.ts` (306 lines) ### Architecture ``` routes/auth.ts │ ├── generateRefreshToken() ├── validateRefreshToken() ├── rotateRefreshToken() ├── revokeRefreshToken() └── revokeAllUserTokens() │ ▼ repositories/refresh-token-repository.ts ← business logic (hash, expiry, log) │ ├── insertRefreshToken() ├── findByTokenHash() ├── revokeByTokenHash() ├── revokeByUserId() ├── cleanupExpired() └── rotateRefreshToken() ← atomic transaction │ ▼ db/refresh-token-queries.ts ← raw SQL only ``` ### Behavioral preservation All runtime behavior is byte-for-byte identical: - SHA-256 hashing unchanged - Revoked/expired token checks identical - Atomic rotate with rowCount gate identical - Error message on consumed token: identical - `parseRefreshTokenExpiry` logic: identical - Logger calls at same levels with same messages ### Verification | Check | Result | |---|---| | `tsc --noEmit` (server) | ✅ clean | | `tsc --noEmit` (scraper) | ✅ clean | | Server tests (868) | ✅ all passing | | Scraper tests (223) | ✅ all passing | | Pre-push hook | ✅ passed | ### Related - Parent: `#1192` — refactor: extend db/*-queries seam, add repositories/ layer - Follow-up: `#1198` — AppError + error middleware + routes-throw cleanup - Follow-up: `#1200` — deleteRole redundant inner SELECT - Follow-up: `#1201` — getRoleInUseCount silently collapses malformed count
…throw typed errors
- Add TheaterNotFoundError extends AppError (404) in utils/errors.ts
- Replace new Error('Theater not found: ...') in scraper-service with typed throw
- Replace new Error(...) in auth-service with ValidationError/AuthError/NotFoundError
- Replace new Error(...) in theater-service with ValidationError/NotFoundError
- Remove string-sniff in routes/scraper.ts — pass through to error middleware
- Collapse auth.ts login/register/change-password catch blocks (no more string matching)
- Collapse theaters.ts mutation catch blocks (add/update/delete routes)
- Update error-handler.test.ts with TheaterNotFoundError 404 mapping test
- Update scraper.test.ts and theaters.test.ts mocks to use typed errors
Closes #1198
…w cleanup (#1209) ## Summary Part of #1192 (slice 6) — AppError + error middleware + routes-throw cleanup. Replaces the string-sniff pattern in `routes/scraper.ts:80` (`error.message.startsWith("Theater not found")`) with a typed `TheaterNotFoundError extends AppError`. Extends this pattern to `auth-service` and `theater-service`, collapsing every route `try/catch` that manually translated error messages into `AppError` subclasses. ## Acceptance criteria - [x] `utils/errors.ts` exports `TheaterNotFoundError extends AppError` - [x] `middleware/error-handler.ts` exists and is mounted as the last `app.use` in `app.ts` - [x] The middleware maps every `AppError` subclass to its HTTP status (instanceof chain) - [x] Unknown errors are logged with `logger.error` and return a static sanitized 500 (no `error.message` in the response) - [x] `routes/scraper.ts:80` string-sniff replaced with pass-through `next(error)` (scraper-service already throws `TheaterNotFoundError`) - [x] Every route's `try { ... } catch (error) { next(new XError(...)) }` collapsed to a `throw`: - `routes/auth.ts` — login, register, change-password - `routes/scraper.ts` — trigger route - `routes/theaters.ts` — addTheaterViaUrl, addTheaterManual, updateTheater, deleteTheater - [x] `app.ts:155` `SELECT 1` health probe remains inline - [x] `middleware/error-handler.test.ts` updated with `TheaterNotFoundError` → 404 mapping test - [x] Route tests updated to assert the new throw/middleware error path (`scraper.test.ts`, `theaters.test.ts`) - [x] `cd server && npx tsc --noEmit` clean - [x] `npm run test:run --workspace=allo-scrapper-server` passes (833 tests) - [x] `cd server && npm run test:coverage` passes the coverage gate ## Changes ### New error class - **`server/src/utils/errors.ts`** — Added `TheaterNotFoundError extends AppError` (constructor takes `theaterId: string`, status 404) ### Services updated to throw typed errors - **`server/src/services/scraper-service.ts`** — `throw new Error("Theater not found: ...")` → `throw new TheaterNotFoundError(theaterId)` - **`server/src/services/auth-service.ts`** — All `throw new Error(...)` replaced with `ValidationError`, `AuthError`, or `NotFoundError` - **`server/src/services/theater-service.ts`** — `throw new Error(...)` replaced with `ValidationError` or `NotFoundError` ### Route catch blocks collapsed (no more string-sniffing) - **`server/src/routes/scraper.ts`** — String-sniff removed; passes through to error middleware - **`server/src/routes/auth.ts`** — Login, register, change-password catch blocks collapsed to `next(error)` - **`server/src/routes/theaters.ts`** — All 4 mutation catch blocks collapsed ### Tests updated - **`server/src/middleware/error-handler.test.ts`** — Added test for `TheaterNotFoundError` → 404 - **`server/src/routes/scraper.test.ts`** — Mock uses `new TheaterNotFoundError("X")` - **`server/src/routes/theaters.test.ts`** — Mock uses `new ValidationError(...)` ### Already in place (no changes needed) - `middleware/error-handler.ts` — Already maps `instanceof AppError` → status code, sanitizes 5xx in production, handles JWT errors - `app.ts:256` — Error middleware already mounted as last `app.use` - `app.ts:155` — `SELECT 1` health probe remains inline ## Verification - `tsc --noEmit` — clean - 833 server tests pass (55 files) - 223 scraper tests pass (19 files) - Coverage gate passes ## Linked issues - Sub-issue: closes #1198 - Parent: #1192
Rename the SQL transaction primitive in db/refresh-token-queries.ts from rotateRefreshToken to rotateRefreshTokenTx so the repository's rotateRefreshToken is the sole public entry point. This closes the seam leak flagged by fallow: consumers can no longer import the SQL function directly and bypass the hashing/expiry policy in the repository.
…rror middleware (#1199) ## Summary Closes #1192 — extend the per-table SQL seam (`db/*-queries.ts`) to absorb every raw `db.query()` outside it, add a `repositories/` layer for business-shaped operations, and replace string-sniffing with typed `AppError`s. ## Sub-issues landed in this umbrella PR | Slice | Issue | Title | |---|---|---| | 1 | #1193 | typed barrel + migrate `routes/users.ts` | | 2 | #1194 | role-deletion route uses `repositories/` | | 3 | #1195 | refresh route uses `getUserWithRoleById` | | 4 | #1196 | system-info service uses named query functions | | 5 | #1197 | refresh-token moved from `services/` to `db/repositories/` | | 6 | #1198 | `AppError` + error middleware + routes-throw cleanup | ## Final fix in this PR `fix(server): close rotateRefreshToken dual-export seam leak` — renames the SQL transaction primitive in `db/refresh-token-queries.ts` to `rotateRefreshTokenTx` so the repository's `rotateRefreshToken` is the sole public entry point. The repository adds hashing/expiry policy; the SQL function is internal. ## Architecture - **Two-tier seam**: `db/` owns per-table SQL; `repositories/` owns business-shaped operations (multi-table queries, business policy). - **Type-level enforcement**: `db/internal/client.ts` holds the `pg.Pool` wrapper; `db/index.ts` re-exports only the `DB` *type*. Any future `import { db }` outside `db/` and `repositories/` is a compile error. - **Error model**: `AppError` subclasses (`TheaterNotFoundError`, etc.); a top-level `error-handler` middleware maps each subclass to an HTTP status. Routes `throw` — no more `try { ... } catch (e) { next(new XError(...)) }`. ## Verification | Check | Result | |---|---| | `tsc --noEmit` (server, scraper, client) | clean | | server tests | 869/869 | | server coverage (80/65/80/80 threshold) | 98.84 / 92.82 / 98.64 / 98.78 | | scraper tests | 223/223 | | fallow (vs develop) | no new dead code, no new complexity | | pre-push hook | passed |
- AGENTS.md: add '## Agent skills' section pointing at docs/agents/
- docs/agents/{issue-tracker,triage-labels,domain}.md: config the
triage / to-issues / qa / domain-modeling skills consume
- CONTEXT.md: seed glossary with Theater, Showtime, WeeklyProgram, Source
- docs/adr/0001-closed-theaters-are-hard-deleted.md: closed Theaters
are hard-deleted; history loss accepted in exchange for schema simplicity
- docs/adr/0002-drop-screen-count.md: screen_count is not a domain
attribute; remove column + scraper parse + UI rendering
🚨 **Severity:** HIGH 💡 **Vulnerability:** The API allowed the modification of core system roles (e.g., the 'admin' role) via `PUT /api/roles/:id` and `PUT /api/roles/:id/permissions`. The `is_system` flag was only being checked on deletion, leaving updates vulnerable. 🎯 **Impact:** An attacker with `roles:update` permissions could rename system roles, alter their descriptions, or, crucially, modify their assigned permissions, potentially leading to privilege escalation or denial of service by stripping admins of necessary rights. 🔧 **Fix:** Added checks in the `PUT /api/roles/:id` and `PUT /api/roles/:id/permissions` route handlers in `server/src/routes/roles.ts` to throw a 403 `AuthError` if the target role has `is_system` set to `true`. ✅ **Verification:** Added dedicated unit tests in `server/src/routes/roles.test.ts` to verify that attempts to update system role properties or permissions correctly result in a 403 `AuthError`. Evaluated and passed by `npm run test:run -w server`. --- *PR created automatically by Jules for task [5526760787351845684](https://jules.google.com/task/5526760787351845684) started by @PhBassin* Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Adds a thin DELETE-only function that issues a single `DELETE FROM roles WHERE id = $1` and returns whether the row was removed (rowCount > 0). Callers MUST already have verified existence and is_system; the function performs no pre-checks. This mirrors the slice-1 pattern (3caf921) of narrow single-purpose repository helpers (`roleExists`, `getRoleNameById`) and provides the seam for a follow-up commit that removes the redundant inner SELECT from the DELETE role handler. Refs #1200
The DELETE /api/roles/:id handler previously issued two SELECTs against
`roles` on the happy path:
1. `getRoleById` — fetches the role + its permissions for the
existence and `is_system` guards.
2. `deleteRole` — re-ran the same `SELECT id, name, description,
is_system, created_at FROM roles WHERE id = $1` internally before
issuing the `DELETE`.
This commit switches the handler to `deleteRoleById` (added in the
previous commit), which performs only the `DELETE` and reports whether
the row was removed via `rowCount > 0`. Net effect: 1 SELECT on
`roles` in the happy path (was 2), no behavior change.
Behavior preserved:
- 204 on success
- 404 when the role does not exist (getRoleById OR TOCTOU race via
deleteRoleById returning false)
- 403 when `is_system` is true
- 409 when users are still assigned to the role
A new route test ("should fetch the role exactly once") pins the
acceptance criterion at the route seam: `getRoleById` and
`deleteRoleById` each called exactly once, and the old `deleteRole`
never called.
Closes #1200
`deleteRole` had exactly one caller — the DELETE /api/roles/:id handler, which switched to `deleteRoleById` in the previous commit. With zero callers, the function and its four tests are dead code; keeping them would leave fallow's unused-export check pointing at a removed feature. The four scenarios previously covered by `deleteRole` tests are now pinned at two seams: - `deleteRoleById` tests cover the rowCount==1 / rowCount==0 contract and the DELETE-only SQL shape. - Route tests pin the 204 / 404 / 403 / 409 outcomes that the old function's behavior fed into. The "fetch the role exactly once" route test drops its now-meaningless `expect(deleteRole).not.toHaveBeenCalled()` assertion (the function no longer exists) and relies on the positive call-count assertions on `getRoleById` and `deleteRoleById`. Closes #1200
…1218) ## Summary The DELETE /api/roles/:id handler issued **two** SELECTs against `roles` on the happy path: 1. `getRoleById` — fetches the role + its permissions for the existence and `is_system` guards. 2. `deleteRole` — re-ran the same `SELECT id, name, description, is_system, created_at FROM roles WHERE id = $1` internally before issuing the `DELETE`. The route had already done all the work needed to decide whether the delete was safe (`getRoleById` returned the role with its `is_system` flag, `getRoleInUseCount` confirmed no users were assigned), making the inner SELECT dead weight. ## Fix Add `deleteRoleById(db, roleId): Promise<boolean>` — a thin DELETE-only repository function that issues a single `DELETE FROM roles WHERE id = $1` and returns whether the row was removed via `rowCount > 0`. Switch the route handler to use it. Net effect: **1 SELECT on `roles` in the happy path (was 2)**, no behavior change. Mirrors the slice-1 pattern (commit 3caf921) of narrow single-purpose repository helpers (`roleExists`, `getRoleNameById`). ## Commits Three atomic commits, each leaving the tree green: 1. **`feat(server/db): add deleteRoleById repository function`** — new function + its 3 unit tests, additive; old `deleteRole` untouched. 2. **`perf(server/routes): use deleteRoleById to skip redundant SELECT`** — route handler + route tests updated; pins the AC with a new "fetch the role exactly once" test. 3. **`chore(server/db): remove now-unused deleteRole function`** — dead-code cleanup after the route switched callers. ## Acceptance criteria - [x] DELETE /api/roles/:id happy path runs exactly 1 SELECT on `roles` (was 2) — pinned by the new "fetch the role exactly once" route test and the `deleteRoleById` "issues exactly one DELETE query and never a SELECT" repository test. - [x] Behavior preserved: 204 on success, 404 on missing role, 403 on system role, 409 on users still assigned — existing route tests still pass against the new wiring. - [x] `routes/roles.test.ts` mocks the new function shape. - [x] Coverage gate passes (98.78% lines / 92.82% branches, gates 80/65). ## Verification - `cd server && npx tsc --noEmit` — clean - `cd scraper && npx tsc --noEmit` — clean - `cd server && npm run test:run` — 871 tests pass - `cd server && npm run test:coverage` — gate passes - `cd scraper && npm run test:run` — 223 tests pass ## Out of scope Same `deleteRole`-style redundancy in `refresh-token-service.ts` (slice 5, #1197) — separate fix, separate PR. Closes #1200
Adds two failing tests pinning the unsafe-default behavior described in issue #1201: - count: null → must throw (not silently return 0) - count: 'not-a-number' → must throw (not propagate NaN) Both fail against the current implementation, which collapses them into 'role is unused' and lets DELETE /api/roles/:id proceed on a role that may be in use.
The `?? '0'` fallback silently collapsed two distinct cases into the
same value: a legitimate empty result and a malformed row (null or
non-numeric count). The latter must surface as an error so the role-
delete route doesn't proceed on a role that may be in use.
- Empty rows: return 0 via an explicit branch.
- Malformed count: throw `Error` → caught by the route's
`catch (error) { next(error); }` and rendered as 5xx.
Resolves #1201
…1219) ## Summary Fixes the unsafe default in `getRoleInUseCount` where the `?? '0'` fallback silently collapsed two distinct cases into the same value: 1. **Legitimate empty result** — no rows returned, role genuinely has 0 users. 2. **Malformed row** — `{ count: null }` or non-numeric `count` from a SQL bug, schema drift, or broken migration. The second case previously surfaced as `0`, letting `DELETE /api/roles/:id` proceed on a role that may be in use. ## Changes - **Empty rows**: returned 0 via an explicit branch (no fallback). - **Malformed count**: throws `Error: getRoleInUseCount: unexpected non-numeric count from database`, which the route's `catch (error) { next(error); }` block propagates to a 5xx response. - Per the `parseStrictInt` → `parseInt(..., 10)` decision in commit `0538c43`, the parser choice is unchanged. This fix is **only** about the silent fallback. ## Acceptance criteria (from #1201) - [x] Empty rows still return 0 (explicit branch, not a fallback) - [x] `null`/non-numeric `count` surfaces as a thrown error (→ 5xx), not 0 - [x] `repositories/role-repository.test.ts` covers the malformed-count case - [x] Behavior for the 204 / 409 / 404 paths unchanged (all 29 route tests pass) ## Out of scope - Generic `parseStrictInt` NaN policy (separate sentinel/security concern). - Reopening the `parseInt` vs `parseStrictInt` question — settled by `0538c43`. ## Verification - **873/873** server tests pass (`npm run test:run --workspace=allo-scrapper-server`). - **223/223** scraper tests pass. - Server coverage gate met: 98.86% stmt / 92.89% branch; `role-repository.ts` at 100% lines. - `tsc --noEmit` clean on both `server` and `scraper`. - All 29 role-route tests pass — 204/409/404 paths unchanged. ## Atomic commits (TDD red → green) 1. `test(server): cover malformed count case in getRoleInUseCount` — RED: 2 new tests, failing against original impl. 2. `fix(server): throw on malformed count from getRoleInUseCount` — GREEN: tests pass. Resolves #1201
- Seed CONTEXT.md with the recurring domain terms (Theater, Showtime, WeeklyProgram, Source) under Core entities and the scrape-run lifecycle (ScrapeAttempt, ScrapeSummary, Resume, ScrapeJob, ProgressEvent, ScheduleChangeEvent, RateLimitConfig) under Scraping. - ScrapeAttempt captures the per-(theater, date) FSM including the handleRateLimit cascade into not_attempted. - Resume collapses the three names (UI button, resumeMode flag, pendingAttempts payload) into one concept. - Document the ScrapeSummary / ScrapeJob / ProgressEvent / ScheduleChangeEvent duplicates ahead of the forthcoming protocol package. - Add CONTEXT.md reference to AGENTS.md critical conventions.
Closes #1210 ## Acceptance criteria (from #1210) - [x] `CONTEXT.md` exists at the repo root - [x] `ScrapeAttempt` (FSM: states, transitions, who triggers each) — `CONTEXT.md:63` - States: `pending`, `success`, `failed`, `rate_limited`, `not_attempted` - Transitions: `pending → success/failed/rate_limited`; `not_attempted` reached directly from `handleRateLimit` (`scraper/src/scraper/index.ts:462`) - [x] `ScrapeSummary` (one canonical definition; server-side shape noted as the post-convergence target) — `CONTEXT.md:84` - [x] `Resume` (maps the three names — UI "Reprendre le scraping", `resumeMode: true`, `pendingAttempts`) — `CONTEXT.md:90` - [x] `ScrapeJob` (discriminated union `{ type: 'scrape' | 'add_theater' }`, anchored to the forthcoming protocol package) — `CONTEXT.md:98` - [x] `ProgressEvent` (anchored to the forthcoming protocol package) — `CONTEXT.md:106` - [x] `ScheduleChangeEvent` — `CONTEXT.md:112` - [x] `RateLimitConfig` (one definition; `source` discriminator documented as audit-only) — `CONTEXT.md:120` - [x] Cross-references to source files inline (e.g. `server/src/db/scrape-attempt-queries.ts:4`, `server/src/services/progress-tracker.ts:19`, `scraper/src/types/scraper.ts:113`, `server/src/services/redis-client.ts:50`, `scraper/src/redis/client.ts:50`, `client/src/pages/ReportsPage/ReportRateLimitedNotice.tsx:38`, `server/src/routes/scraper.ts:85`, `scraper/src/scraper/index.ts:146-147`, `server/src/config/rate-limits.ts:10`, `server/src/db/rate-limit-queries.ts:28`) - [x] `AGENTS.md` mentions `CONTEXT.md` in Critical conventions — `AGENTS.md:12` ## What Introduce `CONTEXT.md` at the repo root as the project's domain glossary, per the architectural-review finding that domain terms were spread across JSDoc, scattered across services, and ambiguous between server and scraper. ## Entries - **Core entities:** `Theater`, `Showtime`, `WeeklyProgram`, `Source` - **Scraping:** `ScrapeAttempt` (with the pending → {success, failed, rate_limited} FSM plus the handleRateLimit cascade into `not_attempted`), `ScrapeSummary`, `Resume` (collapses three names — UI button / `resumeMode` flag / `pendingAttempts` payload — into one concept), `ScrapeJob`, `ProgressEvent`, `ScheduleChangeEvent`, `RateLimitConfig` Each entry uses inline file:line citations to the canonical code, and calls out the "What X is not" cases that future contributors most often trip on (Showtime vs. Screening/Session; Source vs. Strategy/Parser; ScheduleChangeEvent vs. Schedule; `source` discriminator as audit metadata only). ## Why The architecture-review process consumes `CONTEXT.md` to frame candidates and avoid re-deriving terms from JSDoc. The ScrapeAttempt FSM and the Resume concept were the most acute prior gaps; both are now in one place. ## Coordination - No auth-task terms (`UserService`, `mintAccessToken`) exist yet, so none are added here. They land together with whatever PR introduces them. - The `ScrapeSummary` / `ScrapeJob` / `ProgressEvent` / `ScheduleChangeEvent` type collisions are documented and deferred to the forthcoming protocol package, per the issue's note ("CONTEXT.md just documents the canonical one"). ## AGENTS.md Adds the "`CONTEXT.md` is the project's domain glossary — read first, extend in the same change" bullet to Critical conventions. ## Verification Documentation only — no code change, no schema migration. Acceptance criteria from #1210 checked off above.
…n (TDD red)
Add server/src/services/auth-service.mint.test.ts asserting the canonical
JWT payload shape ({ id, username, role_name, is_system_role, permissions })
against a real jwt.verify — no jsonwebtoken mock, independent source of
truth. Covers:
- payload claims match the canonical mint contract
- permission lookup uses user.role_id
- HS256 header
- JWT_EXPIRES_IN is honored when set
- throws when JWT_SECRET is missing
This test will fail until mintAccessToken is added to AuthService — the
next commit wires the implementation. Per issue #1211 acceptance criterion:
"Token payload shape is asserted once, in one test, against the canonical
function."
…TDD green)
Extract the duplicated token-minting assembly from auth-service.login
and routes/auth.ts refresh handler into a single canonical function on
AuthService: mintAccessToken(user, db).
Changes:
- AuthService.mintAccessToken(user, db): Promise<string> now owns the
canonical payload shape ({ id, username, role_name, is_system_role,
permissions }) and the HS256 / getCurrentSecret / parseJwtExpiration
wiring. Exported as AccessTokenSubject the minimum user shape needed.
- auth-service.login goes through this.mintAccessToken; the payload
assembly no longer lives in login.
- routes/auth.ts refresh handler constructs AuthService and calls
authService.mintAccessToken(user, db) instead of the in-handler
`await import('jsonwebtoken')` block plus the parallel
secret/expiry/permissions lookups. The dynamic-import pattern is gone
and the route no longer imports jsonwebtoken directly.
- Routes still import getPermissionNamesByRoleId so the response
payload can include permissions — the same lookup happens inside
mintAccessToken for the token claims; both are kept (one extra
round-trip per request) to honor the issue's `Promise<string>`
return-type spec. A future optimization could fold both lookups into
one.
Closes the implementation half of issue #1211. Pair with the previous
commit (the red-side canonical shape test).
Refs #1211
Promote "Session" from a reserved word to a defined concept in CONTEXT.md: a user-auth credential lifecycle implemented by SessionService (services/session-service.ts). Cross-link the two existing "not a Session" callouts (Showtime, ScrapeRun) to the new Authentication -> Session entry. Refs #1237
…1237) (#1239) Closes #1237 — candidate **C1** (top recommendation) of the architecture review #1232. ## What Extracts the whole credential-issuance lifecycle — access token, refresh token, cookies, CSRF, permission resolution — behind one **Session** seam. `routes/auth.ts` shrinks from 306 to ~40 lines; the six hand-written `set*`/`clear*` cookie helpers and the 73-line inline `/refresh` orchestration move behind `SessionService`. ## Latent bug fixed The refresh-token lifetime was declared twice (`auth.ts` cookie `maxAge` and `DEFAULT_EXPIRY_MS` in the repository). It now lives in **exactly one place** — `parseRefreshTokenExpiry`, driven by `REFRESH_TOKEN_EXPIRY` — and both the persisted token expiry and the refresh cookie `maxAge` read from it. ## Commits - \`e25beff\` introduce SessionService, own credential lifecycle - \`ee60e0c\` record Session domain entry in CONTEXT.md - \`cef57b1\` extract \`toSessionUser\` builder into AuthService (kill the duplicated \`SessionUser\` shape) - \`8a688c3\` unify SessionService response ownership — cookie-touching methods own the full HTTP response via \`sendSuccess\`/\`rejectUnauthorized\`; routes become uniform void shims; \`register\` is the sole return-data method ## Acceptance criteria (#1237) - [x] Session module owns \`login\`/\`refresh\`/\`logout\`/\`changePassword\` end-to-end (token mint, refresh issue/rotate/revoke, cookie + CSRF surface, permission resolution) - [x] \`routes/auth.ts\` handlers are thin (≤ 6 lines each); the six cookie helpers and inline \`/refresh\` orchestration are gone - [x] Refresh-token lifetime declared in exactly one place; cookie \`maxAge\` follows the same source - [x] All endpoints behave identically — existing auth route tests pass - [x] New tests exercise SessionService directly (plain calls + stub \`res\`, no supertest) - [x] CONTEXT.md \`Session\` entry points at the module - [x] \`tsc --noEmit\` clean; coverage gate met (**99.21% stmts / 94.01% branches**, 925 tests) ## Notes - Build-on: #1235 (refresh-token data-access collapse) already landed on the parent branch. - ESM \`.js\` extensions preserved; \`validatePasswordStrength\` chain preserved (delegated to \`AuthService\`); no \`error.message\` in 500s.
…able (#1233) Replace the seven hand-written createRefreshableLimiter blocks, the manual allMiddleware[] array, and the subscribe() loop with one declarative table keyed by limiter name + one factory walk that collects refreshers and wires the config-refresh subscription exactly once. The seven exported handlers keep identical names and behavior, so route consumers are unchanged. Adding a limiter is now one table row + one destructure name instead of three separate edits. Adds a parameterized characterization test pinning each limiter's config-key wiring through the public handler (2nd request is 429 when its own max is driven to 1), so a mis-wiring or dropped limiter fails fast.
…iterSpecs Add a `namedLimiters satisfies Record<LimiterName, RequestHandler>` manifest between the table-driven handlers loop and the named-export destructure. The export list is now compile-checked against limiterSpecs: a missing or stale export name fails tsc (pre-push + CI) instead of only surfacing at a consumer import or via the characterization test. ESM forbids dynamic named exports, so the public surface is still written out once by hand; the manifest makes that hand-written list impossible to drift from the spec table silently. Also drop the redundant afterEach(vi.resetModules) (beforeEach already resets per test) and hoist the duplicated route-registration responder in the characterization test.
…able (#1233) (#1240) Closes #1233 (candidate **C3** of epic #1232). ## What `server/src/middleware/rate-limit.ts` declared **seven** near-identical `createRefreshableLimiter` blocks plus a hand-maintained `allMiddleware[]` and a `subscribe()` loop. Each block differed only in `{ windowKey, maxKey, options }`. Adding an 8th limiter meant editing three places. Replaced with: - one declarative `limiterSpecs` table keyed by limiter name, - one `buildRefreshable` factory, - one loop that collects handlers **and** refreshers, then wires `subscribe` a single time. Adding a limiter is now **one table row + one destructure name** (was three edits). ## Behavior preserved (no route changes) The seven exported handlers keep identical names: \`generalLimiter\`, \`authLimiter\`, \`registerLimiter\`, \`protectedLimiter\`, \`scraperLimiter\`, \`publicLimiter\`, \`healthCheckLimiter\` (+ \`authenticatedKeyGenerator\`). Per-limiter options preserved: - \`skipSuccessfulRequests\` — auth - \`authenticatedKeyGenerator\` — protected, scraper - \`standardHeaders\` — general, protected, health - custom \`message\` + distinct windows — register, health Config refresh still rebuilds every delegate when the source changes (single \`subscribe\` iterating the collected refreshers). ## Test Pure refactor — existing \`rate-limit.test.ts\` is the spec. Added one parameterized **characterization** tracer bullet that drives each limiter's own config key to \`1\` and asserts the 2nd request is \`429\`, pinning the table→key wiring through the public handler. Green before and after the refactor. ## Verification - \`cd server && npx tsc --noEmit\` — clean - \`npm run test:coverage --workspace=allo-scrapper-server\` — 932 tests pass; coverage 99.21% stmts / 94.01% branches, gate exit 0 ## Notes - Based on \`refactor/1232-architecture-deepening\` per the epic's branching note; targets the same branch. - No new dependencies.
ScraperService bundled ScrapeJob dispatch (triggerScrape / triggerResume /
getStatus) with SSE transport (subscribeToProgress: headers, listener
lifecycle, disconnect cleanup). Extract the transport into a standalone
services/sse-bridge.ts whose attachProgressStream(res, sink, onClose?) owns the
text/event-stream + X-Accel-Buffering + Cache-Control + Connection headers and
the add/remove-listener lifecycle, returning a disconnect cleanup.
The bridge depends on a ProgressListenerSink interface (implemented by
ProgressTracker), so it is unit-testable with a stub sink rather than a fake
Express res or the tracker singleton. ScraperService retains dispatch + status
only; routes/scraper.ts /progress now wires the bridge directly.
Behavior preserved: identical headers, connect/disconnect log lines, listener
count, and cleanup wired to req.on('close'). CONTEXT.md SSE cross-reference
updated to the new module. C4 of epic #1232.
#1241) Closes #1236 (candidate **C4** of epic #1232). ## What `ScraperService` bundled two unrelated concerns behind one class: - **ScrapeJob dispatch** — `triggerScrape` / `triggerResume` / `getStatus` (validate theater → create ScrapeReport → reset tracker → publish job) - **SSE transport** — `subscribeToProgress`: `text/event-stream` + `Cache-Control` + `Connection` + `X-Accel-Buffering` headers and the add/remove-listener lifecycle against `progressTracker` They shared a module by name, not by concept. Extracted the transport into a standalone **`services/sse-bridge.ts`** whose `attachProgressStream(res, sink, onClose?)` owns the transport detail and returns a disconnect cleanup. `ScraperService` retains dispatch + status only. ## Why a sink interface The bridge depends on a `ProgressListenerSink` interface (`addListener` / `removeListener` / `getListenerCount`), implemented by `ProgressTracker`. That makes the bridge unit-testable with a stub sink rather than a fake Express `res` or the tracker singleton (AC #4). Per `CONTEXT.md`, the API does not scrape — it fans `ProgressEvent`s from the Redis pub/sub bridge to SSE clients through this seam. ## Behavior preserved (no wire-format or route changes) The `/api/scraper/progress` endpoint behaves identically: - same four headers, same order - same `📡 SSE client connected/disconnected (N total|remaining)` log lines - cleanup still wired to `req.on('close')` - route consumers unchanged (the route now calls the bridge directly with the `progressTracker` singleton) `CONTEXT.md` SSE cross-reference updated from `ScraperService.subscribeToProgress` to `attachProgressStream` in `services/sse-bridge.ts`. ## Test - New `sse-bridge.test.ts` (7 tests): headers, listener registration, connect/disconnect logging, cleanup removes listener, optional `onClose`. - New `/progress` route test asserting the route wires `attachProgressStream` with the real `progressTracker`. - Removed the now-obsolete `subscribeToProgress` unit test from `scraper-service.test.ts`. ## Verification - `cd server && npx tsc --noEmit` — clean - `cd scraper && npx tsc --noEmit` — clean - `npm run test:coverage --workspace=allo-scrapper-server` — 939 tests pass; coverage 99.21% stmts / 94.01% branches, gate exit 0 - `npm run test:run --workspace=allo-scrapper-scraper` — 254 tests pass ## Notes - Based on `refactor/1232-architecture-deepening` per the epic's branching note (same as #1239 / #1240); targets the same branch. - No new dependencies. Did NOT deepen the remaining dispatcher into a single `dispatch(intent)` entry — explicitly marked speculative in #1232.
Lands ADR 0002 (Drop screen_count — it's not a domain attribute). The column was historically scraped from AlloCiné's HTML and displayed in the API/UI but never used in any WHERE / ORDER BY / aggregation; the historical values are intentionally discarded per the ADR. ## What - New migration `025_drop_screen_count.sql` (next free number after 024; idempotent via `DROP COLUMN IF EXISTS`). 017/018 are historically duplicated but do not consume 025. - `docker/init.sql`: column dropped from the CREATE TABLE so fresh DBs match. - `server/src/db/system-queries.test.ts`: the pending-migrations test now asserts 025 is in the expected file list (the migration runner auto-computes and stores the SHA-256 checksum). ## Verification - `cd server && npx tsc --noEmit` — clean - `npm run test:run --workspace=allo-scrapper-server` — system-queries test passes with the new entry
Closes #1234 part A (candidate C5 of epic #1232): the validator extraction was 'extracted-for-testability' that created a test web pinning dead behavior in place. The Theater module's public interface — addTheaterViaUrl / addTheaterManual / updateTheater — is now the only test surface, and the combination logic that lived inline and untested is finally exercised through it. Also drops screen_count from the server layer as part of the same fold (validateScreenCount was one of the extracted validators; removing the column while the validator file existed would have meant deleting a file and untangling its test web — this commit removes both together). ## What - Deleted `services/theater-validator.ts` (+ its dedicated 197-line test file). The ten exported validators were a private implementation detail presented as a public seam. - Folded all field rules into `services/theater-service.ts` as non-exported module functions. The combination policy — 'at-least-one-field required', per-field length/charset rules, short-circuit order — now lives behind the TheaterService interface. - Exported `TheaterUpdateInput` as the typed input to `updateTheater` (replacing the inline inline type). The speculative-generality `validateAtLeastOneField(data, [...])` is replaced with a hardcoded `UPDATE_FIELDS` constant (single call site). - Removed `screen_count` from: `TheaterUpdateInput`, the `updateTheater` body, `Theater` in `types/scraper.ts`, and all three query modules (`theater-queries` INSERT/UPDATE/SELECT + TheaterRow; `showtime-queries` ShowtimeWithTheaterRow + 3 SELECT projections + mapper; `movie-queries` WeeklyMovieRow + 2 SELECT projections + mapper). ## Test `services/theater-service.test.ts` expanded from 12 → 36 tests. The interface is now the test surface, including combination cases the issue called out as previously untested: - 'accepts and forwards a combination of all valid fields together' - 'forwards an empty string as undefined (reset) alongside a real update' (documents that the at-least-one-field check fires before the reset semantics) - 'short-circuits on the first invalid field in a combination' - 'does not call the DB when validation fails' Removed the 4 `screen_count`-specific cases from `routes/theaters.validation.test.ts` (negative, zero, >50, non-integer); the route still has end-to-end coverage of the remaining location fields. ## Verification - `cd server && npx tsc --noEmit` — clean - `npm run test:run --workspace=allo-scrapper-server` — 921/921 pass - `npm run test:coverage --workspace=allo-scrapper-server` — 99.41% stmts / 95.89% branches / 99.09% funcs (theater-service.ts: 100% stmts, 88.88% branches)
Part of landing ADR 0002 on the scraper side. AlloCiné's data-theater JSON still publishes a `screenCount` field; the parser now ignores it (along with any other unknown keys) instead of carrying it through to the Theater type and the upsert. ## What - `scraper/src/types/scraper.ts`: `screen_count` removed from the `Theater` interface. - `scraper/src/scraper/theater-parser.ts`: `parseTheaterSection` no longer reads `data.screenCount`; the fallback Theater object drops the field. - `scraper/src/db/theater-queries.ts`: `upsertTheater` INSERT and the `getTheaters` mapper drop the column; TheaterRow updated. - Test: removed the screen_count type/count assertion from the theater-parser fixture test and the inline HTML fixture's `screenCount` key. ## Verification - `cd scraper && npx tsc --noEmit` — clean - `npm run test:run --workspace=allo-scrapper-scraper` — 253/253 pass
Part of landing ADR 0002 on the client side. Drops the field from the Theater type, the edit form, the admin table, and the public detail page (the '🎬 X salles' affordance is gone per the ADR). ## What - `client/src/types/index.ts` + `client/src/api/theaters.ts`: `screen_count` removed from `Theater` and `TheaterUpdate`. - `client/src/utils/theaterValidators.ts`: `validateScreenCount` deleted (+ its dedicated test block). - `client/src/hooks/useEditTheaterForm.ts`: the screenCount field is gone from FormState, FORM_FIELDS, buildUpdates, and the returned API. - `client/src/components/admin/EditTheaterModal.tsx`: the 'Number of Screens' form group is removed. - `client/src/components/admin/TheatersTable.tsx`: the 'Screens' column is removed. - `client/src/components/theater/TheaterHeader.tsx`: the '🎬 X salle(s)' paragraph is removed. - Test fixtures scrubbed across TheaterPage, TheatersPage, TheatersQuickLinks, and useEditTheaterForm. ## Verification - `cd client && npx tsc -b` — clean - `npm run test:run --workspace=client` — 584/584 pass
Updates the domain docs to reflect that screen_count has been dropped. ## What - `docs/adr/0002-drop-screen-count.md`: status flips Accepted → Implemented, with the implementing issue (#1234) and migration (025_drop_screen_count.sql) noted. - `CONTEXT.md`: the Theater entry's screen_count note moves from present tense ('is being removed') to past ('has been removed'). - `docs/reference/api/{theaters,movies}.md`: `screen_count` removed from response-body examples (list, create, update). - `docs/reference/database/{README,schema}.md`: column dropped from the theaters table reference. - `docs/reference/architecture/scraper-system.md`: dropped from the parsed-theater interface sketch. - `docs/project/agents.md`: the 'Zero Values — use != null not ||' teaching snippet was built on `theater.screen_count`; rewrote it to use `movie.press_rating` (a real numeric-or-null field that still exists).
#1234) (#1242) Closes #1234 (candidate **C5** of epic #1232). ## What Two coupled changes in one vertical slice: ### A. Fold `theater-validator.ts` back inside the Theater module `server/src/services/theater-validator.ts` was ten pure validators extracted for testability, but the extraction created a test web that pinned dead behavior in place. The question that actually matters — *"is this whole payload valid, and which fields combine legally?"* — lived inline and untested in `TheaterService.updateTheater`. This is the "testing past the interface" smell: micro-tests passed while the real combination logic was uncovered. The field rules now live inside `services/theater-service.ts` as **non-exported module functions**. The Theater module's public interface — `addTheaterViaUrl` / `addTheaterManual` / `updateTheater` / `deleteTheater` — is the only test seam. The speculative-generality `validateAtLeastOneField(data, [...])` is replaced with a hardcoded `UPDATE_FIELDS` constant (single call site), and a typed `TheaterUpdateInput` is exported for route consumers. ### B. Land ADR 0002 (drop `screen_count`) ADR 0002 (*Drop screen_count — it's not a domain attribute*) was **Accepted** since 2026-06-23 but stuck behind the validator extraction — the test web made removal expensive. Doing A + B together removes that resistance. The `screen_count` column is dropped (idempotent migration `025`), and the field is removed from every layer: server queries/types/service, scraper parser/types/upsert, client type/API/form/table/header, `docker/init.sql`, and all test fixtures. ADR 0002 flips to **Implemented**. ## Why both at once Per #1234: *"the validator extraction is precisely what has made ADR 0002 expensive to land — the test web pins the dead field in place. Doing A + B together removes that resistance."* `validateScreenCount` was one of the ten extracted validators; folding it back and deleting it is the same edit as removing the column from the service signature. ## Test - `services/theater-validator.test.ts` (197 lines) **deleted** — the dedicated unit-test web for the extracted validators is gone. - `services/theater-service.test.ts` expanded from **12 → 36 tests**. The interface is now the test surface, including the combination cases the issue called out as previously untested: - "accepts and forwards a combination of all valid fields together" - "forwards an empty string as undefined (reset) alongside a real update" (documents that the at-least-one-field check fires before the reset semantics) - "short-circuits on the first invalid field in a combination" (documents field order) - "does not call the DB when validation fails" - Removed the 4 `screen_count`-specific cases from `routes/theaters.validation.test.ts`; the route still has end-to-end coverage of the remaining location fields. ## Verification - `cd server && npx tsc --noEmit` — clean - `cd scraper && npx tsc --noEmit` — clean - `cd client && npx tsc -b` — clean - `npm run test:run --workspace=allo-scrapper-server` — **921/921 pass** - `npm run test:coverage --workspace=allo-scrapper-server` — 99.41% stmts / 95.89% branches / 99.09% funcs (theater-service.ts: 100% stmts, 88.88% branches); gate exit 0 - `npm run test:run --workspace=allo-scrapper-scraper` — **253/253 pass** - `npm run test:run --workspace=client` — **584/584 pass** ## Commits 5 atomic commits (schema → server → scraper → client → docs): 1. `feat(db): drop screen_count column from theaters` — migration `025` + `docker/init.sql` + system-queries test 2. `refactor(server): fold theater-validator into Theater module` — incl. validator file deletion, `TheaterUpdateInput`, expanded service tests, screen_count scrubbed from queries/types/routes 3. `refactor(scraper): stop parsing screenCount from source HTML` 4. `refactor(client): remove screen_count from theater UI` — type/API/form/table/header/validators + tests 5. `docs: mark ADR 0002 implemented and scrub reference docs` ## Notes - Based on `refactor/1232-architecture-deepening` per the epic's branching note (same as #1239 / #1240 / #1241); targets the same branch. - The migration is destructive (`DROP COLUMN`) — historical `screen_count` values are discarded per ADR 0002; no preservation step. - Real-world HTML fixtures (`theater-c0089-page.html` etc.) still contain `screenCount` in the source JSON; the parser now ignores it (alongside any other unknown keys), which those fixture tests continue to exercise. - No new dependencies. Did NOT deepen the TheaterService into a single `dispatch(intent)` entry — out of scope for C5. - Code-review skill run: **0 hard standards violations, 0 spec gaps.**
…imiting The loop-based factory + record indirection + re-export destructuring landed in #1233 broke CodeQL's js/missing-rate-limiting data flow, producing a false-positive alert on /api/auth/login (which is limited). Return to direct createRefreshableLimiter(...).handler exports while keeping the limiterSpecs table as the single source of truth.
…en seam, rate-limit table, SSE transport, theater-validator (#1232) (#1243) Closes #1232. Epic merge: lands the five deepening candidates from the architecture review (*Session module · refresh-token seam · rate-limit table · SSE transport · theater-validator*) into `develop`. Each candidate was reviewed and merged individually against this branch (see *Sub-PRs* below); this PR is the final consolidation. The branch fast-forwards cleanly into `develop` (0 commits behind). ## Candidates landed | # | Candidate | Issue | PR | Outcome | |---|---|---|---|---| | **C1** | Deepen the **Session module** | #1237 | #1239 | New `SessionService` owns the credential-issuance lifecycle (access token, refresh token, cookies, CSRF, expiry). Route handlers under `routes/auth.ts` shrink to thin shims. Refresh-token lifetime now declared in **exactly one place** (`parseRefreshTokenExpiry`), closing the latent cookie/DB drift bug. | | **C2** | Collapse the **refresh-token data-access seam** + fold `role-repository` | #1235 | (direct) | `db/refresh-token-queries.ts` collapsed into `repositories/refresh-token-repository.ts` — hashing policy stops leaking into SQL; the interface deals in raw tokens. `role-repository.ts` (23 lines, one function) folded into `db/role-queries.ts`. One clear data-access convention repo-wide. | | **C3** | Drive the **rate-limit middlewares** from one declarative table | #1233 | #1240 | The seven near-identical `createRefreshableLimiter` blocks replaced by one table keyed by limiter name + one factory that wires `subscribe` once. Adding an 8th limiter is now one table row. | | **C4** | Extract the **SSE transport** from `ScraperService` | #1236 | #1241 | New `services/sse-bridge.ts` owns the `text/event-stream` transport detail; `ScraperService` retains ScrapeJob dispatch only. Bridge unit-testable with a stub sink instead of a fake Express `res`. | | **C5** | Fold **`theater-validator`** back into the Theater module + land ADR 0002 | #1234 | #1242 | The 10 extracted validators (and their 197-line test web) gone; field rules live behind `TheaterService.add/update`; the previously-untested combination logic is now exercised through the interface. ADR 0002 (drop `screen_count`) landed in the same slice — the extraction was what had made it expensive. | ## Diff at a glance ``` 61 files changed, 1583 insertions(+), 1439 deletions(-) ``` - **5 new files**: `services/session-service.{ts,test.ts}`, `services/sse-bridge.{ts,test.ts}`, `migrations/025_drop_screen_count.sql` - **3 deletions**: `services/theater-validator.{ts,test.ts}`, `repositories/role-repository.ts` - **18 commits** across 4 sub-PRs + direct C2 commits ## Breaking change: `screen_count` removed (ADR 0002) Lands ADR 0002 (*Drop screen_count — it's not a domain attribute*), Accepted since 2026-06-23. The column is dropped (idempotent migration `025`), and the field is removed from every layer: server queries/types/service, scraper parser/types/upsert, client type/API/form/table/header, `docker/init.sql`, and all test fixtures. ADR 0002 status flips to **Implemented**. Per the ADR: the field was never used in any `WHERE` / `ORDER BY` / aggregation in the server, scraper, or client — only displayed. Historical values are intentionally discarded (no preservation step). The 🎬 X salles affordance on the theater detail page is gone. ## Domain glossary updates (`CONTEXT.md`) - **Session** entry added — records the new `SessionService` as the canonical credential-issuance lifecycle, with explicit "what a Session is *not*" callouts (access token, SSE subscriber session, ScrapeRun, Showtime). - **Theater** entry's `screen_count` note moves from present ("is being removed") to past ("has been removed"). ## Verification Each sub-PR ran the full pre-push gate (`tsc --noEmit` × 3 workspaces + server tests + server coverage + scraper tests). At the final consolidation point: - `cd server && npx tsc --noEmit` — clean - `cd scraper && npx tsc --noEmit` — clean - `cd client && npx tsc -b` — clean - Server tests: **921 pass**; coverage **99.41% stmts / 95.89% branches / 99.09% funcs** (gate exit 0) - Scraper tests: **253 pass** - Client tests: **584 pass** ## Sub-PRs (traceability) 1. **#1239** — `refactor(server): introduce SessionService, own credential lifecycle (#1237)` — C1 2. *(direct commits on epic)* — `collapse refresh-token data-access seam` + `fold role-repository into role-queries` — C2 (#1235) 3. **#1240** — `refactor(server): drive rate-limit middlewares from one declarative table (#1233)` — C3 4. **#1241** — `refactor(server): extract SSE transport into standalone bridge (#1236)` — C4 5. **#1242** — `refactor: fold theater-validator into Theater module and land ADR 0002 (#1234)` — C5 ## Lineage Builds on four earlier refactors referenced in #1232: - #1210 — introduced `CONTEXT.md` (C1 instantiates the *Session* concept it reserved) - #1211 — centralized `mintAccessToken` in `AuthService` (C1 is the unfinished second half) - #1213 — converged rate-limit config to one `RateLimitSource` (C3 is the middleware-side follow-up) - #1216 — deepened the scraper orchestrator (C4 applies the same deepening lens server-side) ## Notes - No new dependencies added across any of the five candidates. - Merge strategy: this branch is 0 commits behind `develop` — a fast-forward or merge commit will both work. - Visual report (Tailwind + Mermaid before/after diagrams for each candidate): https://gist.github.com/PhBassin/f3320899ddef5c190c940a4a7478c329
Adds a self-contained production overlay for deploying allo-scrapper to a single VPS on top of the existing docker-compose.yaml. - deploy/docker-compose.prod.yml: Traefik v3 reverse proxy (auto Let's Encrypt via TLS-ALPN-01) + Watchtower (label-scoped auto-update on the :stable tag, 5 min poll). ics-web host port is masked; routing is declared via Docker labels. App containers get watchtower.enable=true; DB/Redis/Traefik/Watchtower are excluded. - deploy/.env.example: template with DOMAIN, ACME_EMAIL, JWT_SECRET, POSTGRES_PASSWORD, ALLOWED_ORIGINS. - deploy/README.md: full runbook — bootstrap (ufw, fail2ban, Docker install), first boot, verify CD, daily ops, rollback, troubleshooting, optional Traefik dashboard with BasicAuth. - scripts/deploy-rollback.sh: one-liner to roll the three app containers back to an arbitrary tag (vX.Y.Z or stable). DB/Redis/Traefik/Watchtower left running. No changes to CI, Dockerfiles, or application code.
Traefik v3.2's embedded Docker SDK defaults to API version 1.24, which Docker Engine 25+ rejects (minimum supported is 1.40). Auto-negotiation does not trigger reliably on newer daemon, causing the docker provider to loop with: client version 1.24 is too old. Minimum supported API version is 1.40 Pin DOCKER_API_VERSION=1.44 (available on Docker Engine 25+/26+/27+) so Traefik uses a version both sides accept.
Traefik v3.2 (late 2024) ships an older Docker SDK that defaults to API version 1.24, which Docker Engine 25+ rejects. The DOCKER_API_VERSION env var is not propagated by Traefik's client, so pinning it alone did not fix the issue. Bumping to Traefik v3.7 (latest stable, July 2026) brings a recent SDK that negotiates the API version correctly. We keep the DOCKER_API_VERSION pin as a belt-and-suspenders safety net.
Watchtower uses the same Docker SDK as Traefik and hits the same 'client version 1.25 is too old' error on Docker Engine 25+/29+. Apply the same DOCKER_API_VERSION=1.44 pin.
## Summary Adds a self-contained production overlay for deploying allo-scrapper to a single VPS (Docker Engine 29) on top of the existing `docker-compose.yaml`. Validated end-to-end on `cinemas.opelkad.com`. ### Files added - `deploy/docker-compose.prod.yml` — Traefik v3.7 reverse proxy (auto Let's Encrypt via TLS-ALPN-01) + Watchtower (label-scoped auto-update on the :stable tag, 5 min poll). `ics-web` host port is masked; routing is declared via Docker labels. App containers get `watchtower.enable=true`; DB/Redis/Traefik/Watchtower are excluded. - `deploy/.env.example` — template with `DOMAIN`, `ACME_EMAIL`, `JWT_SECRET`, `POSTGRES_PASSWORD`, `ALLOWED_ORIGINS`. - `deploy/README.md` — full runbook (bootstrap: ufw, fail2ban, Docker install, deploy user + CI SSH key) + first boot + verify CD + daily ops + rollback + troubleshooting + optional Traefik dashboard with BasicAuth. - `scripts/deploy-rollback.sh` — one-liner to roll the three app containers back to an arbitrary tag (`vX.Y.Z` or `stable`). DB/Redis/Traefik/Watchtower left running. ### Key decisions - **Traefik v3.7** (not Caddy). Docker provider via labels, ACME TLS-ALPN-01, dashboard off in prod. - **Watchtower with label scope** — only updates the three app containers. - **`:stable` tier** — follows the latest `vX.Y.Z` tag. `develop` pushes go to `:latest` and are NOT auto-deployed. - **`DOCKER_API_VERSION=1.44` pinned** on Traefik and Watchtower — their embedded Docker SDK defaults to 1.24/1.25 which Docker Engine 25+ rejects. Traefik v3.7`s newer SDK negotiates correctly but we keep the pin as belt-and-suspenders. ### Production validation (cinemas.opelkad.com, 2026-07-19) - All 7 containers healthy (traefik, ics-web, ics-scraper, ics-scraper-cron, ics-db, ics-redis, watchtower) - Let's Encrypt cert obtained via TLS-ALPN-01 in ~6s - `curl https://cinemas.opelkad.com/api/health` → `{"status":"healthy","database":"connected"}` ### No changes to - CI workflows (`.github/workflows/`) - Dockerfiles - Application code (`server/`, `client/`, `scraper/`, `packages/`) - Existing `docker-compose.yaml` ## Test plan - [x] `docker compose config --quiet` validates the merged compose - [x] Labels correctly applied: ics-web traefik=true + watchtower=true; DB/Redis/Traefik/Watchtower excluded - [x] Port 3000 not exposed on host (Traefik proxies via internal network) - [x] End-to-end on VPS: HTTPS up, cert obtained, health endpoint responds, admin login works - [ ] Watchtower CD test (push a new tag, observe auto-update) — to be done after merge
PhBassin
added
enhancement
PhBassin
added a commit
that referenced
this pull request
Jul 19, 2026
Manual release v4.8.0 - version-tag.yml was skipped because the merge commit body of PR #1245 contained the literal string [skip ci].
PhBassin
added a commit
that referenced
this pull request
Jul 19, 2026
## Problem
The `version-tag.yml` workflow was skipping pushes when the merge commit
**body** contained `[skip ci]` as literal text — even when it was inside
a PR template example.
**Root cause**: `if: ${{ !contains(github.event.head_commit.message,
[skip ci]) }}` at the job level performs a naive substring match against
the **full** commit message (subject + body). GitHub convention reserves
`[skip ci]` for the **subject line** only.
**Consequence**: PR #1245 (`chore(release): 4.7.4 → 4.8.0 …`) was merged
to main. Its PR template body contained the literal string:
```
5. Commit `chore(release): bump version to v4.8.0 [skip ci]`
```
The workflow matched this and silently skipped — no tag, no release, no
Docker build. This PRs version bump was applied manually as a
workaround.
## Solution
Move the skip-guard from a job-level `if:` expression to a **first-step
shell guard** that only inspects the **subject line** (first line) of
`head_commit.message`:
```yaml
steps:
- name: Skip on version-bump commits (check subject only, not body)
run: |
SUBJECT="$(echo ${{ github.event.head_commit.message }} | head -n 1)"
if echo "$SUBJECT" | grep -qE \[(skip|no) ci\]; then
echo "Skipping — version-bump commit detected (subject contains [skip ci])"
exit 78
fi
```
The automatic bump commits still have `[skip ci]` in their subject
(`chore(release): bump version to vX.Y.Z [skip ci]`) and will be
correctly skipped.
Uses `exit 78` for a **neutral** skip (job shows as cancelled/cancelled,
workflow stays green).
## Verification
- `[skip ci]` in subject → skipped ✓ (automatic bump commits)
- `[skip ci]` in body only → runs ✓ (PR merge commits with template
examples)
- No `[skip ci]` anywhere → runs ✓ (normal pushes)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release PR — develop → main
Base tag:
v4.7.4(released 2026-06-19, 30 days ago)Proposed version:
v4.8.0(label:minor)Commits merged: 97
Diff: 172 files, +7,676 / −5,363 lines
Version bump reasoning
Label:
minor— recommended per Conventional Commits.Commits breakdown (97 total)
Breaking changes
BREAKING CHANGE:in body: 0!suffix on type: 0 (3 commits match--grep "!"but as false positives — the!is invalidateExternalUrl/screen_countstrings, not a CC suffix)Decision: 17
featcommits, 0 explicit breaking → textbookminor.majorwas considered (argument: droppingscreen_countbreaks existing VPS databases) but rejected because (a) the migration is gated behindIF EXISTS, (b) no public API surface is affected, (c) the deployment footprint is a single VPS.Highlights
Architecture (server)
routes/users.tsfor admin user CRUD ([Task]: Extract UserService from routes/users.ts #1215, refactor(server): extract UserService from routes/users.ts (closes #1215) #1230)SseBridgemodule ([Task]: Extract the SSE transport out of ScraperService #1236, refactor(server): extract SSE transport into standalone bridge (#1236) #1241)repositories/introduced,role-repositoryfolded intorole-queriescatchblocks ([PRD #1192 / 6] AppError + error middleware + routes-throw cleanup #1198, feat(1198): [PRD #1192 / 6] AppError + error middleware + routes-throw cleanup #1209)Architecture (scraper)
FetchTransport/PuppeteerTransportadapters;AllocineScraperStrategydepends on Transport, not onhttp-clientdirectly ([Task]: Extract Transport adapter interface in scraper (deepening of #1214) #1225, refactor(scraper): extract validateExternalUrl SSRF utility (cycle 1 of #1225) #1227, refactor(scraper): extract Transport adapter interface (closes #1225) #1228)validateExternalUrlSSRF utility extracted (cycle 1 of [Task]: Extract Transport adapter interface in scraper (deepening of #1214) #1225)ScrapeRundeep module — encapsulates mutable run state (closes [Task]: Deepen scraper orchestrator (862 LOC, mutable ScrapeContext) #1216, refactor(scraper): introduce ScrapeRun deep module to encapsulate mutable run state (closes #1216) #1231)fetchTheaterPage(closes [Task]: Make the Puppeteer-vs-fetch seam in scraper http-client.ts explicit (and fix SSRF gap) #1214, fix(scraper): reject non-allocine URLs in fetchTheaterPage (closes #1214) #1226)Domain / DB
screen_countcolumn dropped fromtheaters(ADR 0002 landed). Migration025_drop_screen_count.sqlis idempotent (IF EXISTS).theater-validatorfolded in, ADR 0002 marked implementedDeployment / DevOps
deploy/docker-compose.prod.yml,deploy/.env.example,deploy/README.md,scripts/deploy-rollback.sh(feat: VPS production deployment overlay (Traefik + Watchtower) #1244)cinemas.opelkad.comDOCKER_API_VERSION=1.44pinned for Docker Engine 25+/29+ compatibilitypackages/scraper-protocolextracted as shared module ([PRD #1192 / 5] refresh-token feature moves from services/ to repositories/ #1197)Pre-merge checklist
mainminor)package.jsonon develop NOT manually bumped (the workflow will bump on main)vX.Y.Ztag pushed (workflow creates it post-merge)Post-merge — what
version-tag.ymlwill do.github/scripts/generate-changelog.shpackage.jsonversion to4.8.0## [4.8.0] - <date>block intoCHANGELOG.mdchore(release): bump version to v4.8.0 [skip ci]v4.8.0on maindocker-build-push.yml) viagh workflow run:stabletag to the new imagesync-main-to-develop.ymlrebases develop onto main (round-trip the bump back)Post-merge verification (read-only)
VPS impact
Once
v4.8.0is tagged and the image publishes toghcr.io/phbassin/allo-scrapper:stable, Watchtower oncinemas.opelkad.comwill roll forward automatically within ~5 min. This release fixes thescreen_count500 error currently affecting/api/movies.