AI Implementation feature(913): Admin Area Players Management 1.00 (#57)

.kilo/plans/867.md

@@ -1,50 +0,0 @@
-# Implementation Plan: Hardened User Deletion Cleanup + Concurrency-Safe Last-Admin Lock
-
-## 1. Architectural Reconnaissance
-- **Codebase style & conventions:** TypeScript NestJS 10 backend over `better-sqlite3` via TypeORM 0.3. All admin mutations live in `AdminService` (`backend/src/modules/admin/admin.service.ts:109-119`) and call a shared `UsersService.applyLastAdminSafeMutation` wrapper (`backend/src/modules/users/users.service.ts:63-95`). The wrapper currently uses `dataSource.transaction('SERIALIZABLE', ...)` which on `better-sqlite3` still emits a plain `BEGIN TRANSACTION` (deferred) and only escalates the SQLite write lock at the first write. Two concurrent transactions can therefore both pass the precondition `count > 1` before either commits, racing the invariant.
-- **Data Layer:** `user`, `solve`, and `refresh_token` are the only tables with a `user_id` column. `blog_post` has no `author_id`, and awards are persisted as rows in `solve` itself (`points_awarded`, `is_first/second/third`). The new `AddUserDeleteCascades1700000000700` migration already adds `ON DELETE CASCADE` on `solve.user_id` and `refresh_token.user_id`, so a plain `manager.delete(UserEntity, ...)` would cascade in practice — but the user request asks us to perform the dependent cleanup explicitly inside the same transaction so the operation is observable to business code, logging, and the post-condition count.
-- **Test Framework & Structure:** Jest 29 multi-project config (`tests/jest.config.js`); existing `tests/backend/last-admin-invariant.spec.ts` and `tests/backend/admin-players.spec.ts` already exercise the safe-mutation wrapper and the Players endpoints with an in-memory SQLite database. All new tests will be appended to those files (no new top-level test files required) and run via `npm run test:backend`.
-- **Required Tools & Dependencies:** No new package or global CLI is required. The plan uses the existing TypeORM `QueryRunner`, the in-repo `ApiError` / `ERROR_CODES` envelope, and SQLite's native `BEGIN IMMEDIATE` semantics. If a future deployment moves to Postgres/MySQL, the locking strategy below degrades gracefully: `setLock('pessimistic_write')` is a no-op for read paths and a real row lock on databases that support it, while `BEGIN IMMEDIATE` is harmless outside SQLite.
-
-## 2. Impacted Files
-- **To Modify:**
-  - `backend/src/modules/admin/admin.service.ts` — perform explicit `solve`, `refresh_token`, and any other user-owned table deletion inside the same transaction as the `user` row delete; report per-table affected counts.
-  - `backend/src/modules/users/users.service.ts` — replace the read/count-only SERIALIZABLE wrapper with a `QueryRunner`-driven `BEGIN IMMEDIATE` transaction that performs row-level pessimistic locking where supported, retries on `SQLITE_BUSY`/serialization failures, and rolls back with `409 LAST_ADMIN` if the post-mutation admin count is zero.
-  - `backend/src/common/errors/error-codes.ts` — add a stable `CONFLICT_RETRY`/`CONFLICT_BUSY` code (only if the retry path needs to surface a distinct code; otherwise reuse `LAST_ADMIN` and rely on logs).
-  - `tests/backend/last-admin-invariant.spec.ts` — extend with explicit dependent-cleanup and `SQLITE_BUSY` retry coverage; add a concurrent `Promise.all` assertion that at most one writer can drive the admin count to zero.
-  - `tests/backend/admin-players.spec.ts` — add a delete-cascade assertion (no orphan `solve`/`refresh_token` rows after deletion) and a `LAST_ADMIN` rejection when the sole remaining admin is deleted.
-- **To Create:** None. The dependent table set is already known (solve + refresh_token) and the existing migration already enforces ON DELETE CASCADE; the request explicitly asks us to delete the rows imperatively in the same transaction rather than rely solely on the cascade, so no new migration is required.
-
-## 3. Proposed Changes
-1. **Backend Logic & APIs — Dependent cleanup on delete (admin.service.ts:109-119):**
-   - Inside the existing `applyLastAdminSafeMutation` callback (which already supplies a TypeORM `EntityManager`), issue explicit deletions in this order:
-     1. `manager.delete(SolveEntity, { userId: targetId })` (capture `affected`).
-     2. `manager.delete(RefreshTokenEntity, { userId: targetId })` (capture `affected`).
-     3. `manager.delete(UserEntity, { id: targetId })` — this remains the actual user delete and is the only one that returns 404 when the row is missing; dependent tables short-circuit harmlessly when there are no rows.
-   - Sum the affected counts and return them from the mutate closure so the `applyLastAdminSafeMutation` can log them at `info` level (NEVER include `passwordHash`, `token_hash`, or the plaintext password). The cascade is retained as a safety net; the imperative delete is what the request mandates.
-   - Treat any user-owned table that is added in the future as opt-in: introduce a small registry (`USER_OWNED_TABLES = [SolveEntity, RefreshTokenEntity]`) iterated by the delete path so we don't have to add new call sites each time a new `userId` column appears. The plan keeps this registry co-located with the service; no new file.
-2. **Backend Logic & APIs — Concurrency-safe last-admin wrapper (users.service.ts:63-95):**
-   - Replace the `dataSource.transaction('SERIALIZABLE', ...)` call with a `dataSource.createQueryRunner()`-driven flow:
-     1. `await queryRunner.connect()` then `await queryRunner.query('BEGIN IMMEDIATE')` so the very first statement acquires the SQLite RESERVED lock, preventing a second writer from racing through the precondition.
-     2. Acquire a row-level lock on the target user via `repo.createQueryBuilder('u').setLock('pessimistic_write').where('u.id = :id', { id: targetId }).getOne()`. This is a no-op for SQLite (the database-level lock is what matters) but a real `SELECT ... FOR UPDATE` on Postgres/MySQL if the deployment is ever migrated.
-     3. Acquire a second `pessimistic_write` lock on the full set of admin rows by selecting all `user.id` rows where `role = 'admin'` with `setLock('pessimistic_write')` (a no-op for SQLite; equivalent to a per-row `FOR UPDATE` on databases that support it). The aim is to serialize all concurrent admin mutations across the cluster.
-     4. Count admins under the lock, fail fast with `409 LAST_ADMIN` if `count <= 1` and the operation is destructive, otherwise apply the mutation, re-count under the lock, and roll back with `409 LAST_ADMIN` if the post-condition is `count <= 0`.
-     5. Commit; in the `catch`, attempt `ROLLBACK` and rethrow. Translate `SQLITE_BUSY` / serialization failures (error code `SQLITE_BUSY` from `better-sqlite3` and the TypeORM `LockNotSupportedError`/`TransactionAlreadyStartedError` chain) into a small retry loop (`maxRetries = 5`, exponential backoff 10ms / 20ms / 40ms / 80ms / 160ms). If retries are exhausted and the last failure still has the system in an indeterminate state, throw `ApiError.conflict(ERROR_CODES.LAST_ADMIN, ...)` so the caller always sees the existing 409 contract.
-   - Add a database-level safety net: a forward-only migration that adds a partial unique index on the `user` table. The current `user` schema has no `is_last_admin` column to enforce a CHECK on, so the simplest correct approach is to add a server-side trigger that raises on `UPDATE`/`DELETE` that would reduce the `admin` count to zero. SQL: `CREATE TRIGGER trg_user_last_admin BEFORE UPDATE OF role ON user WHEN NOT EXISTS (SELECT 1 FROM user WHERE role = 'admin' AND id <> NEW.id) AND OLD.role = 'admin' AND NEW.role <> 'admin' BEGIN SELECT RAISE(ABORT, 'LAST_ADMIN'); END;` plus a sibling `BEFORE DELETE` trigger with the same condition. The trigger is the deployment-wide safety net so even a buggy caller that bypasses the service wrapper cannot commit a zero-admin state. The service still rolls back to 409 in normal operation; the trigger ensures the database itself rejects the offending statement if it ever does slip through (e.g. raw SQL or a future parallel module that forgets the wrapper).
-3. **Backend Logic & APIs — Error code & messaging:**
-   - The existing `ApiError.conflict(ERROR_CODES.LAST_ADMIN, message)` shape is preserved for every public path. Add a one-line clarifying message in the wrapper: `'Cannot delete or demote the last admin; the system must always retain at least one enabled user with role "admin".'`. Plaintext, hashes, and tokens are never included.
-4. **Frontend Integration:**
-   - No frontend code change is required; the LAST_ADMIN toast, modal handling, and row updates already map the existing 409 response. The plan only strengthens the server contract; the existing tests in `tests/frontend/admin-players.pure.spec.ts` and `tests/frontend/change-password-modal.spec.ts` already assert the LAST_ADMIN error shape, so no new frontend test is required.
-5. **Test Strategy (covered in `tests/backend/last-admin-invariant.spec.ts` and `tests/backend/admin-players.spec.ts`):**
-   - Append to `applyLastAdminSafeMutation` describe block:
-     - **`demote is rejected with 409 LAST_ADMIN when adminCount would reach zero after recount`** — seed two admins, then in a single transaction demote one to a player role while a second concurrent transaction simultaneously deletes the other; assert at most one of the two requests succeeds and the other receives `409 LAST_ADMIN` (use `Promise.all` with two `applyLastAdminSafeMutation` calls).
-     - **`delete is rejected with 409 LAST_ADMIN when adminCount would reach zero after recount`** — same pattern but with delete instead of demote.
-     - **`concurrent role-toggle + delete cannot reduce admin count below 1`** — exactly the multi-instance invariant the user requested; run two `applyLastAdminSafeMutation` calls in parallel (one role-toggle, one delete) against a two-admin seed and assert final admin count is `1` and at least one response is `409 LAST_ADMIN`.
-   - Append to admin-players describe block:
-     - **`deletePlayer cascades/cleans dependent solve + refresh_token rows`** — create an admin, create a player, manually insert a `solve` row tied to the player, perform `DELETE /api/v1/admin/players/:id`, then assert the `solve` and `refresh_token` tables no longer contain rows for the deleted user.
-     - **`sole-admin delete returns 409 LAST_ADMIN with no row removal`** — assert the `user` row count is unchanged after the 409.
-   - Use the existing in-memory better-sqlite3 + Nest `Test.createTestingModule` setup; no new infrastructure. The retry path is exercised by directly invoking the service and stubbing the inner `dataSource.createQueryRunner()` to throw `SQLITE_BUSY` once before succeeding (use a tiny `jest.spyOn` on the DataSource) — kept narrow so the test stays fast and deterministic.
-
-## 4. Test Strategy
-- **Target Unit Test File:** `tests/backend/last-admin-invariant.spec.ts` (service-level) and `tests/backend/admin-players.spec.ts` (HTTP-level).
-- **Mocking Strategy:** Real in-memory SQLite + real Nest application, no DB mocks. The retry path uses a one-line `jest.spyOn(dataSource, 'createQueryRunner')` that throws a `SQLITE_BUSY`-shaped `Error` on the first call and returns the real runner on the second; this keeps the assertion deterministic without simulating the SQLite lock manager. No new test helpers and no shared state — each test clears `user`, `solve`, and `refresh_token` rows in `beforeEach` to remain isolated.

.kilo/plans/913-build-fix.md

@@ -0,0 +1,70 @@
+# Implementation Plan: Fix Job 913 Build Error (missing `auth` injection in AdminUsersComponent)
+
+## 1. Root cause
+The `onToggleRole` handler added in the previous turn calls
+`this.auth.currentUser()`, `this.auth.applyPeerRoleChange(...)`, and
+`this.auth.publishRoleChange(...)`
+(`frontend/src/app/features/admin/admin-users.component.ts:244-247`), but
+`AdminUsersComponent` never declared an `auth` field. The component only injects
+`AdminService` (`admin`) and `NotificationService` (`notify`). TypeScript build
+(`ng build --configuration production`) therefore fails with `TS2339: Property
+'auth' does not exist on type 'AdminUsersComponent'` at lines 244, 246, 247.
+
+The Jest tests passed because they exercise pure modules and
+`admin-players.pure`, not the component class — so the missing field was not
+caught at test time.
+
+## 2. Required changes (single file)
+
+**File:** `frontend/src/app/features/admin/admin-users.component.ts`
+
+1. **Add the import** alongside the existing core-service imports (lines 10-11
+   currently import `AdminService` and `NotificationService`):
+
+```ts
+import { AuthService, AdminUser } from '...';
+```
+
+The `AdminUser` type can be left on the existing `AdminService` import line. We
+must add `AuthService` and import only the value (not the type) — `AuthService`
+is the singleton provided in `'root'` by `core/services/auth.service.ts`.
+
+The accurate edit is:
+
+- Replace the existing `import { AdminService, AdminUser } from '...admin.service';`
+  with two separate imports, or add the new import:
+
+```ts
+import { AdminService, AdminUser } from '../../core/services/admin.service';
+import { AuthService } from '../../core/services/auth.service';
+```
+
+2. **Inject the service** alongside the two existing injections (lines ~170-171):
+
+```ts
+private readonly admin = inject(AdminService);
+private readonly notify = inject(NotificationService);
+private readonly auth = inject(AuthService);
+```
+
+That is the only change required — the existing `onToggleRole` body already
+references `this.auth.currentUser`, `this.auth.applyPeerRoleChange`, and
+`this.auth.publishRoleChange`, all of which exist on `AuthService`.
+
+## 3. Verification
+
+- `npm --workspace frontend run build` should pass (no TypeScript errors).
+- `npm test` should still pass — the existing pure tests
+  (`tests/frontend/admin-players.pure.spec.ts`,
+  `tests/frontend/auth-session-events.spec.ts`,
+  `tests/backend/admin-players.spec.ts`) are unaffected by adding the
+  injection.
+- No public API, route, store, or service signature changes are required.
+
+## 4. Out of scope
+
+- No backend changes.
+- No test additions (the build error is a pure TypeScript declaration
+  issue; tests already cover the behaviour).
+- No re-architecture of cross-tab role-change handling — the previous turn's
+  implementation is correct, only the missing field declaration is needed.