fix(settings): drop dead wiring and the speculative deleteForUser; strict row assertions

server/src/db/repositories/Settings.repository.ts

@@ -46,12 +46,21 @@ export class SettingsRepository extends EntityRepository<Settings> {
    * `persist(false)` scalar twin (`user_id`) — same rule as every other
    * `create`/`upsert` call in this migration (D4's defaults rule): the
    * relation is the one column MikroORM actually writes, the twin is filled
-   * by a subsequent read. `onConflictFields: ['user', 'key']` matches
-   * `Settings`'s own `uniques: [{ properties: ['user_id', 'key'] }]`
-   * (`Settings.entity.ts`, generated off the schema's inline
-   * `UNIQUE(user_id, key)` by the `fix(db)` commit that precedes this one) —
-   * `Settings.repository.test.ts` asserts both the schema-level unique index
-   * and the entity metadata before this method relies on either.
+   * by a subsequent read. `Settings`'s own `uniques: [{ properties: ['user',
+   * 'key'] }]` (`Settings.entity.ts`, generated off the schema's inline
+   * `UNIQUE(user_id, key)` by the `fix(db)` commit that precedes this one)
+   * names that same relation field — `Settings.repository.test.ts` asserts
+   * both the schema-level unique index and the entity metadata.
+   *
+   * `onConflictFields` is passed explicitly here for CLARITY at the call
+   * site, not because `em.upsert` needs it to find the conflict target: the
+   * `uniques` entry above is fidelity to the schema first, but as a
+   * consequence it is also consumable — `em.upsert` infers the same
+   * `(user, key)` target from it alone when `onConflictFields` is omitted
+   * (`getWhereCondition` in `@mikro-orm/core/utils/upsert-utils.js` matches a
+   * `uniques` entry's `properties` against the payload's own property-named
+   * keys). `SETTINGSREPO-014` proves that inference directly, independent of
+   * this method's explicit option.
    *
    * Only `value` is ever updated on conflict, matching the legacy statement's
    * `DO UPDATE SET value = excluded.value` exactly (no other column, no
@@ -60,21 +69,4 @@ export class SettingsRepository extends EntityRepository<Settings> {
   async upsertForUser(userId: number, key: string, value: string): Promise<void> {
     await this.upsert({ user: userId, key, value }, { onConflictFields: ['user', 'key'], onConflictAction: 'merge' });
   }
-
-  /**
-   * `DELETE FROM settings WHERE user_id = ?` (`key` omitted) or
-   * `DELETE FROM settings WHERE user_id = ? AND key = ?` (`key` given).
-   *
-   * No site in `nest/settings` itself calls this today (the inventory's 10
-   * sites have no `DELETE FROM settings` — the one existing call,
-   * `plugin-runtime.service.ts`'s per-plugin `key LIKE 'plugin:<id>:%'`
-   * sweep, is `nest/plugins`' own conversion, out of this task's scope).
-   * Declared for the same reason `AppSettingsRepository.deleteValue` is: the
-   * repository's surface should be symmetric, and a future caller (a
-   * user-cleanup/account-deletion flow, say) gets a real method instead of
-   * reaching for raw SQL. Exercised by its own repository test only.
-   */
-  async deleteForUser(userId: number, key?: string): Promise<number> {
-    return this.nativeDelete(key === undefined ? { user: userId } : { user: userId, key });
-  }
 }

server/src/nest/settings/settings.module.ts

@@ -8,7 +8,6 @@ import { SettingsMcp } from './settings.mcp';
 import { AppConfigModule } from '../app-config/app-config.module';
 import { AppSettings } from '../../db/entities/AppSettings.entity';
 import { Settings } from '../../db/entities/Settings.entity';
-import { Users } from '../../db/entities/Users.entity';
 
 /** Exports SettingsService for in-container consumers (admin, share, llm-parse). */
 @Module({
@@ -17,14 +16,13 @@ import { Users } from '../../db/entities/Users.entity';
   // registers AppSettingsRepository/SettingsRepository for the
   // @InjectRepository(...) constructor params below (Plan 3a Task 0/5) — the
   // forFeature + @InjectRepository wiring pattern every later domain copies.
-  // `Users` is registered too even though nothing here uses
-  // `@InjectRepository(Users)`: `instance-api-keys.ts`'s repository-backed
-  // functions resolve `UsersRepository` off the ACTIVE MikroORM request
-  // context instead (see that file's own docstring for why — its callers
-  // span ~6 domains outside this module's DI graph), so this entry is here
-  // for parity with the brief/inventory's documented wiring, not because a
-  // provider in this module needs the token.
-  imports: [AppConfigModule, AuthModule, AuditModule, MikroOrmModule.forFeature([AppSettings, Settings, Users])],
+  // No `Users` entry: `instance-api-keys.ts`'s repository-backed functions
+  // take a `UsersRepository` as an explicit parameter from their caller
+  // (nothing resolves it off an ambient MikroORM request context — an
+  // earlier design tried that and broke ~185 hand-built unit tests, see
+  // task-5-review.md §4), so nothing in this module ever needs
+  // `@InjectRepository(Users)`.
+  imports: [AppConfigModule, AuthModule, AuditModule, MikroOrmModule.forFeature([AppSettings, Settings])],
   controllers: [SettingsController, AdminDefaultUserSettingsController],
   providers: [SettingsService, SettingsMcp],
   exports: [SettingsService],

server/tests/helpers/test-uow.ts

@@ -128,15 +128,13 @@ export function createTestSettingsRepo(db: Database.Database): Promise<SettingsR
 }
 
 /**
- * The `UsersRepository` `instance-api-keys.ts`'s repository-backed functions
- * resolve through the active MikroORM request context rather than a
- * constructor argument (Plan 3a Task 5 — see the file's own docstring for
- * why: its callers span ~6 domains outside this plan, still passing
- * `DatabaseService`, and none of them are touched by this conversion). A
- * hand-built test that exercises those functions still needs a real
- * `UsersRepository` bound to the same ORM its `withRequestContext`-style
- * wrapper forks from — this is that repository, same memoisation as the
- * others in this file.
+ * The `UsersRepository` a hand-constructed service passes into
+ * `instance-api-keys.ts`'s `resolveApiKey` (and the other functions there
+ * that take one) — an explicit parameter, not resolved off an ambient
+ * MikroORM request context (Plan 3a Task 5; an earlier version of that file
+ * tried the request-context approach and broke 185 unit tests across 12
+ * files that build a service with no request context around it — see that
+ * file's own docstring). Same memoisation as the others in this file.
  */
 export function createTestUsersRepo(db: Database.Database): Promise<UsersRepository> {
   const existing = usersPerHandle.get(db);

server/tests/unit/mcp/tools-trips.test.ts

@@ -470,23 +470,6 @@ describe('Tool: search_cover_images', () => {
     })));
   }
 
-  // UnsplashService.getUnsplashKey resolves through instance-api-keys.ts's
-  // resolveApiKey (Plan 3a Task 5), which now reads AppSettingsRepository/
-  // UsersRepository off the active MikroORM request context when the caller
-  // has no repository of its own to pass — this in-memory MCP harness
-  // (mcp-test-controllers.ts) builds every controller by hand with no HTTP
-  // request around it, so there is no such context here (unlike buildApp()'s
-  // real e2e/production path, or a DI-injected repository test like
-  // AppSettingsRepository's own, both of which are covered elsewhere).
-  // These two tests are about the SEARCH RESULT handling (parsing Unsplash's
-  // response into photos / an error), not the key-resolution precedence
-  // (env > instance > user row), which unsplash.service.test.ts's own
-  // getUnsplashKey suite already covers directly — so an operator env key
-  // here short-circuits resolveApiKey before it ever needs that context,
-  // without losing any coverage this describe block is actually for.
-  beforeEach(() => { process.env.UNSPLASH_ACCESS_KEY = 'operator-test-key'; });
-  afterEach(() => { delete process.env.UNSPLASH_ACCESS_KEY; });
-
   it('returns the photo candidates for a query', async () => {
     const { user } = createUser(testDb);
     stubUnsplash({