.kilo/plans/893.md

@@ -1,37 +0,0 @@
-# Implementation Plan: Admin Area General Settings and Categories 1.11
-
-## 1. Architectural Reconnaissance
-- **Codebase style & conventions:** Node.js npm-workspace monorepo with strict TypeScript. The backend is NestJS 10 with thin controllers and repository-backed services; the frontend is Angular 17 using standalone, OnPush components, signals, typed `HttpClient` calls, and embedded `AdminCategoriesComponent` rendering on both `/admin/categories` and `/admin/general`. Category API responses are mapped from entities into `CategoryView`, and list ordering is `LOWER(abbreviation)` plus abbreviation as a deterministic tie-breaker.
-- **Data Layer:** TypeORM 0.3 over `better-sqlite3`, `synchronize: false` in the application, explicit migrations registered in `backend/src/database/database.module.ts`, and startup migration execution through `DatabaseInitService.init()` before listening. The persistent production database defaults to `/data/hipctf/db.sqlite`. The current entity and list service require `created_at`/`updated_at`, but legacy databases can have the earlier six-column `category` table. Although migration `1700000000200` attempts to repair timestamps and `1700000000300` defines the canonical CRY/MSC/PWN/REV/WEB/HW rows, the observed persistent database proves a new forward migration is required: already-recorded migrations must never be edited or relied on to rerun.
-- **Test Framework & Structure:** Root Jest 29 multi-project configuration with `ts-jest`; backend tests live in `tests/backend/` and run via `npm test` (or focused `npm run test:backend`). Existing migration tests use an isolated in-memory SQLite database. Extend this dedicated test area with a focused legacy-schema migration regression rather than UI/visual tests; the Angular component and service wiring already render icon, abbreviation, description, edit, and delete actions from a successful response.
-- **Required Tools & Dependencies:** No new system tools, global CLIs, npm packages, or `/data` fixtures are required. Existing Node/npm, TypeScript, TypeORM, `better-sqlite3`, NestJS testing, and Jest dependencies are sufficient; `setup.sh` should remain unchanged. Verification should use the root single-command suite (`npm test`) and existing build command (`npm run build`).
-
-## 2. Impacted Files
-- **To Modify:**
-  - `backend/src/database/database.module.ts` — register the new forward repair migration after `UpdateSystemCategoryKeys1700000000300` so existing `/data` databases receive it on restart.
-  - `tests/backend/migrations.spec.ts` — register the migration in the fresh-schema migration list and strengthen canonical system-category assertions to require exactly CRY, HW, MSC, PWN, REV, WEB once each with populated metadata.
-  - `tests/backend/database-init.spec.ts` — assert startup initialization yields the exact canonical set and remains duplicate-free across repeated initialization/startup behavior.
-  - `backend/src/config/env.schema.ts` — replace the obsolete crypto/forensics/pwn/web/misc/osint seed constants with the canonical system-key metadata, keeping seed definitions consistent with the repaired database model for fresh installs and future reuse.
-- **To Create:**
-  - `backend/src/database/migrations/1700000000400-RepairCategorySchemaAndSystemCategories.ts` — forward-only, idempotent compatibility migration for legacy category schemas and stale system-category rows.
-  - `tests/backend/category-repair-migration.spec.ts` — focused in-memory regression starting from the exact legacy six-column schema and CRY/FOR/MSC/OSI/PWN/WEB data described by the Job.
-
-## 3. Proposed Changes
-1. **Database / Schema Migration:**
-   - Add a new migration with a timestamp/name greater than `1700000000300`; do not modify the semantics of migrations that may already be recorded in persistent databases.
-   - Inspect `PRAGMA table_info("category")`; add `created_at` and `updated_at` only when absent, then backfill blank/null values with an ISO UTC SQLite timestamp. This makes TypeORM `SELECT c.*` valid before any category rows are read through `CategoryEntity`.
-   - Reconcile system rows transactionally against one canonical definition ordered by the required sort key: CRY (Cryptography), HW (Hardware), MSC (Misc), PWN (Pwn), REV (Reverse Engineering), WEB (Web), each with non-empty description and icon path.
-   - Preserve user-created rows (`system_key IS NULL`). For system rows, remove obsolete FOR/OSI records, update existing canonical-key rows to canonical name/abbreviation/description/icon metadata, and insert only missing canonical keys. Handle abbreviation collisions deterministically: reuse an existing row with the canonical abbreviation only when it can safely become that system row; otherwise avoid destructive changes to unrelated user data and fail migration with a clear conflict rather than silently corrupting references.
-   - Ensure uniqueness indexes for non-null `system_key` and `abbreviation` exist after reconciliation. Make the SQL guarded/upsert-like so rerunning the migration logic in a test, or restarting after it has been recorded, leaves exactly one row per canonical system key and no duplicates.
-   - Keep rollback conservative: reverse only schema/index additions that can be safely removed, or explicitly make `down` non-destructive for canonical data so rollback cannot delete user/challenge relationships.
-2. **Backend Logic & APIs:**
-   - Keep `AdminCategoriesService.list()` and `CategoryEntity` timestamp fields intact; once startup repair aligns the physical schema, TypeORM can select all declared columns and map the existing `CategoryView` without the `c.created_at` 500.
-   - Keep `GET /api/v1/admin/categories` controller/service contracts unchanged. Confirm the service returns repaired rows alphabetically by lowercased abbreviation with deterministic tie-breaking and exposes `iconPath`, `abbreviation`, `description`, `isSystem`, and timestamps.
-   - Align `SYSTEM_CATEGORY_KEYS`/`SYSTEM_CATEGORY_META` with CRY/HW/MSC/PWN/REV/WEB so a fresh database and any future seed consumer cannot recreate FOR/OSI. Use one canonical metadata representation where practical to prevent the seed and repair migration from drifting.
-   - Tighten startup verification to validate the canonical six system keys rather than only logging a total category count; retain user-created categories in the total and report missing/duplicate canonical rows explicitly.
-3. **Frontend UI Integration:**
-   - No frontend code change is planned. `AdminGeneralComponent` already embeds `AdminCategoriesComponent`; `AdminService.listCategories()` already calls the correct endpoint; and the category template already renders icon, abbreviation, description, pencil, and trash controls for every returned row. Repairing the API restores both `/admin/general` and `/admin/categories` without introducing a competing Angular effect or touching the reported NG0600 symptom.
-
-## 4. Test Strategy
-- **Target Unit Test File:** `tests/backend/category-repair-migration.spec.ts`, plus minimal assertion updates in `tests/backend/migrations.spec.ts` and `tests/backend/database-init.spec.ts`.
-- **Mocking Strategy:** Use a real isolated `better-sqlite3` in-memory `DataSource`/`QueryRunner`; do not mock TypeORM or use the persistent `/data` database. Construct the exact legacy `category` table with only `id`, `system_key`, `name`, `abbreviation`, `description`, and `icon_path`, seed CRY/FOR/MSC/OSI/PWN/WEB, run the new migration, and assert: timestamp columns exist and are populated; a TypeORM repository/list query no longer throws; the system abbreviations are exactly `[CRY, HW, MSC, PWN, REV, WEB]` in API sort order; each row has icon and description metadata; and invoking the repair twice leaves six unique system rows. Keep the migration suite's fresh-database assertion equally exact, and close every DataSource after the tests. No browser, visual confirmation, network service, or heavyweight frontend fixture is needed.

.kilo/plans/894.md

@@ -0,0 +1,82 @@
+# Implementation Plan: Admin Area General Settings and Categories 1.12 (Job 894)
+
+## 1. Architectural Reconnaissance
+
+- **Codebase style & conventions:**
+  - Backend: NestJS 10 with TypeORM + better-sqlite3, TypeScript, async/await throughout, zod-validated DTOs.
+  - Frontend: Angular 17 standalone components, signals + reactive forms, OnPush change detection, jest-jsdom for tests.
+  - Tests live under `/repo/tests/backend` and `/repo/tests/frontend` (a single root-level `tests/` directory — never alongside source). They are executed with `npm test` from the repository root via `tests/jest.config.js` (multi-project Jest config, two projects: `backend` and `frontend`).
+- **Data Layer:** SQLite via TypeORM (`/data/hipctf/db.sqlite` by default). Theme catalog is file-based (`backend/themes/*.json`), loaded at module-init by `ThemeLoaderService` and filtered at request-time by `AdminGeneralService.listThemes()`.
+- **Test Framework & Structure:** Jest 29 with `ts-jest`. Two projects under `tests/jest.config.js`. Backend project uses `node` environment, frontend uses `jsdom` with `tests/frontend/jest.setup.ts`. New tests must be placed under `tests/backend/` (or `tests/frontend/`) and be discoverable by the existing globs (`<rootDir>/backend/**/*.spec.ts` / `<rootDir>/frontend/**/*.spec.ts`).
+- **Required Tools & Dependencies:** No new dependencies are required. The fix is a path-resolution change in the existing service. No `setup.sh` change is required (the canonical themes already ship in the repo at `backend/themes/`).
+
+## 2. Impacted Files
+
+- **To Modify:**
+  - `backend/src/modules/admin/general.service.ts` — change the `THEMES_DIR` resolution so the default `./themes` resolves to the backend project's `themes/` directory regardless of process CWD. This is the only behavioural change needed.
+- **To Create:**
+  - `tests/backend/admin-general-list-themes.spec.ts` — minimal unit test that asserts `listThemes()` returns all 10 canonical themes for the default configuration (the current `admin-general-service.spec.ts` already has a `listThemes` suite but it uses `process.env.THEMES_DIR` and a temp dir, so we add a focused new spec that exercises the **default** path-resolution without any env override, which is the regression case).
+
+## 3. Proposed Changes
+
+### Root-cause analysis
+
+`backend/src/modules/admin/general.service.ts:80-103` (`listThemes`) reads
+```ts
+const themesDir = path.resolve(this.config.get<string>('THEMES_DIR', './themes'));
+```
+`path.resolve('./themes', ...)` is resolved against `process.cwd()`. The backend is started with `node /repo/backend/dist/main.js` from `/repo` (per `package.json` `start` script), so `./themes` resolves to `/repo/themes`, which does not exist. The 10 canonical JSON files live at `/repo/backend/themes/`. Because the directory does not exist, `present` stays empty and every theme is filtered out, so `GET /api/v1/admin/general/themes` returns `[]` and the Admin → General "Global theme" `<select>` renders with zero options.
+
+### 1. Backend logic — fix default path resolution
+
+In `backend/src/modules/admin/general.service.ts`:
+
+- Replace the inline resolution with a small private helper, e.g.:
+  ```ts
+  private resolveThemesDir(raw: string): string {
+    if (path.isAbsolute(raw)) return raw;
+    // Resolve relative to the backend project root (one level up from
+    // backend/src/modules/admin at compile time, i.e. backend/), so the
+    // default './themes' always points at backend/themes regardless of
+    // the process CWD used to launch `node backend/dist/main.js`.
+    const backendRoot = path.resolve(__dirname, '..', '..', '..');
+    return path.resolve(backendRoot, raw);
+  }
+  ```
+  and use it in `listThemes()`:
+  ```ts
+  const themesDir = this.resolveThemesDir(this.config.get<string>('THEMES_DIR', './themes'));
+  ```
+- Rationale: from `backend/src/modules/admin/general.service.ts`, three `..` segments walk back to the backend package root (`backend/`). At runtime the compiled file lives at `backend/dist/modules/admin/general.service.js`, so `__dirname` is `backend/dist/modules/admin` and three `..` segments still reach `backend/`. This makes `./themes` always point at the canonical `backend/themes/` directory shipped in the repo, without changing the public `THEMES_DIR` contract (an absolute path or a custom relative path still works).
+- Keep the rest of `listThemes()` unchanged: it still intersects the loader's catalog with on-disk JSON `id`s, so a custom `THEMES_DIR` pointing at a different folder still filters accordingly.
+
+### 2. Frontend / signal hardening (defensive, small)
+
+The job reports a browser console warning
+`RuntimeError: NG0600: Writing to signals is not allowed in a computed or an effect by default`
+when the Admin General page loads with the empty theme list. Once the backend returns 10 themes, this symptom goes away. To make the component robust if the network call fails, no structural change to the signal graph is needed — the existing pattern (`signal<ThemeView[]>([])` written in `ngOnInit` via `this.themes.set(themes)`) is correct and not inside an `effect()`/`computed()`. We **do not** introduce new signal writes; we only verify nothing in `frontend/src/app/features/admin/general.component.ts` writes a signal from a computed/effect, and leave the file untouched unless the implementer finds an unrelated `effect()` that mutates state (none exists today — verified by grep). If the implementer wants to be extra defensive, a single optional one-liner fallback in `general.pure.ts` (e.g. a pure `BUILTIN_THEME_VIEW_FALLBACK` array used when the API returns `[]`) is acceptable but not required.
+
+### 3. `setup.sh` / data layer
+
+No changes. `/data` is not touched by this job. The `THEMES_DIR` env var (and its default) continue to work the same way; only the default resolution now points at the correct directory.
+
+## 4. Test Strategy
+
+### Target Unit Test File
+
+- `tests/backend/admin-general-list-themes.spec.ts` (new) — single focused regression spec for the path-resolution bug.
+
+### Mocking Strategy
+
+- Instantiate `AdminGeneralService` directly with hand-rolled fakes for the three injected collaborators (`SettingsService`, `ThemeLoaderService`, `SseHubService`) and a `ConfigService` whose `.get('THEMES_DIR', './themes')` returns the **default** `'./themes'` (no `process.env` override) so the test exercises the same code path the real backend takes.
+- `ThemeLoaderService.listThemes()` is faked to return 10 entries with the canonical ids (`classic`, `midnight`, `sunset`, `forest`, `cyber`, `paper`, `crimson`, `ocean`, `neon`, `monochrome`).
+- Do NOT mock `fs` or `path`. The test relies on the real `backend/themes/` directory shipped in the repo (this is exactly the regression case).
+
+### Cases (minimal, single-file)
+
+1. **Regression:** with no `THEMES_DIR` override, `listThemes()` returns exactly 10 entries whose ids are a permutation of the canonical `THEME_IDS` set, and each entry has `id === key` and a non-empty `name`. This is the single must-have test that locks in the fix.
+2. **Robustness:** when called twice in a row, the result is referentially equivalent (no I/O ordering surprises).
+
+That is intentionally the entire scope. We do **not** add tests for: visual rendering of the `<select>` (frontend tests focus on logic), custom `THEMES_DIR` overrides (already covered by the existing `admin-general-service.spec.ts` "filter to on-disk themes" suite), `PUT /settings` happy-path (already covered), the unknown-`themeKey` 400 (already covered by the existing `rejects unknown themeKey` case in `admin-general-service.spec.ts`), and the runtime NG0600 warning (a downstream symptom that goes away once the list is non-empty — not a separate contract).
+
+The single test must run with `npm test -- tests/backend/admin-general-list-themes.spec.ts` (or simply `npm test`) from the repository root.

backend/src/modules/admin/general.service.ts

@@ -78,7 +78,7 @@ export class AdminGeneralService {
   }
 
   listThemes(): ThemeView[] {
-    const themesDir = path.resolve(this.config.get<string>('THEMES_DIR', './themes'));
+    const themesDir = this.resolveThemesDir(this.config.get<string>('THEMES_DIR', './themes'));
     let present = new Set<string>();
     try {
       if (fs.existsSync(themesDir)) {
@@ -101,4 +101,10 @@ export class AdminGeneralService {
       .filter((t) => present.has(t.id))
       .map((t) => ({ id: t.id, key: t.id, name: t.name }));
   }
+
+  private resolveThemesDir(raw: string): string {
+    if (path.isAbsolute(raw)) return raw;
+    const backendRoot = path.resolve(__dirname, '..', '..', '..');
+    return path.resolve(backendRoot, raw);
+  }
 }

tests/backend/admin-general-list-themes.spec.ts

@@ -0,0 +1,54 @@
+process.env.DATABASE_PATH = ':memory:';
+process.env.FRONTEND_DIST = './frontend/dist';
+process.env.NODE_ENV = 'test';
+// Intentionally do NOT set process.env.THEMES_DIR — this spec exercises the
+// default path-resolution behaviour so the test fails if the default ever
+// regresses to a CWD-relative path.
+
+import { AdminGeneralService } from '../../backend/src/modules/admin/general.service';
+import { THEME_IDS } from '../../backend/src/common/types/theme-ids';
+
+function makeFakeThemeLoader() {
+  return THEME_IDS.map((id) => ({
+    id,
+    name: id.charAt(0).toUpperCase() + id.slice(1),
+    tokens: {} as any,
+  }));
+}
+
+describe('AdminGeneralService.listThemes - default THEMES_DIR resolution (Job 894)', () => {
+  const fakeSettings = { get: jest.fn(), set: jest.fn() } as any;
+  const fakeHub = { emitEvent: jest.fn() } as any;
+  const fakeThemes = { listThemes: () => makeFakeThemeLoader() } as any;
+  // Mimic the production Nest ConfigService: .get('THEMES_DIR', './themes')
+  // returns the literal default string when no env override is present.
+  const fakeConfig = {
+    get: jest.fn().mockImplementation((key: string, fallback?: any) => fallback),
+  } as any;
+
+  it('returns all 10 canonical themes when THEMES_DIR is left at its default', () => {
+    const svc = new AdminGeneralService(fakeSettings, fakeThemes, fakeHub, fakeConfig);
+    const themes = svc.listThemes();
+    expect(themes.length).toBe(10);
+    const ids = themes.map((t) => t.id).sort();
+    expect(ids).toEqual([...THEME_IDS].sort());
+  });
+
+  it('maps every entry to { id, key, name } with id === key and a non-empty name', () => {
+    const svc = new AdminGeneralService(fakeSettings, fakeThemes, fakeHub, fakeConfig);
+    const themes = svc.listThemes();
+    for (const t of themes) {
+      expect(t.id).toBe(t.key);
+      expect(typeof t.name).toBe('string');
+      expect(t.name.length).toBeGreaterThan(0);
+    }
+  });
+
+  it('produces the same 10-entry list on repeated calls (no I/O ordering surprises)', () => {
+    const svc = new AdminGeneralService(fakeSettings, fakeThemes, fakeHub, fakeConfig);
+    const a = svc.listThemes().map((t) => t.id).sort();
+    const b = svc.listThemes().map((t) => t.id).sort();
+    expect(b).toEqual(a);
+    expect(a.length).toBe(10);
+  });
+});