From e228bae6290fb419105e22b4310a7165a8e332b3 Mon Sep 17 00:00:00 2001 From: Marek Chmielowski Date: Wed, 5 Aug 2026 11:49:54 +0200 Subject: [PATCH] fix(tag): require reasons for sticky removals --- .../schemas/__tests__/tag-schemas.test.ts | 52 +++++++++ packages/core/src/contracts/schemas/tag.ts | 4 +- .../pam/tag/__tests__/tag.router.int.test.ts | 108 ++++++++++++++++++ packages/core/src/pam/tag/router/index.ts | 16 ++- .../core/src/pam/tag/service/tag.service.ts | 22 +++- 5 files changed, 192 insertions(+), 10 deletions(-) create mode 100644 packages/core/src/contracts/schemas/__tests__/tag-schemas.test.ts diff --git a/packages/core/src/contracts/schemas/__tests__/tag-schemas.test.ts b/packages/core/src/contracts/schemas/__tests__/tag-schemas.test.ts new file mode 100644 index 00000000..c78f9dfa --- /dev/null +++ b/packages/core/src/contracts/schemas/__tests__/tag-schemas.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from 'vitest'; +import { removePlayerTagSchema, replacePlayerTagSchema } from '../tag.js'; + +const removeBase = { + playerId: '11111111-1111-4111-8111-111111111111', + tagKey: 'vip' as const, + removalActor: 'manual' as const, + removalActorUserId: null, +}; + +const replaceBase = { + playerId: '11111111-1111-4111-8111-111111111111', + tagKey: 'level' as const, + assignReason: 'level changed', + assignActor: 'scheduled' as const, + assignActorUserId: null, +}; + +describe('player tag removal reason schemas', () => { + it.each([ + ['empty', ''], + ['whitespace-only', ' '], + ['four non-whitespace characters', ' abcd '], + ])('rejects %s for removePlayerTag', (_label, removalReason) => { + expect(removePlayerTagSchema.safeParse({ ...removeBase, removalReason }).success).toBe(false); + }); + + it('allows a missing reason so the service can apply sticky-tag rules', () => { + expect(removePlayerTagSchema.safeParse(removeBase).success).toBe(true); + }); + + it.each([ + ['missing', undefined], + ['empty', ''], + ['whitespace-only', ' '], + ['four non-whitespace characters', ' abcd '], + ])('rejects %s for replacePlayerTag', (_label, removalReason) => { + expect(replacePlayerTagSchema.safeParse({ ...replaceBase, removalReason }).success).toBe(false); + }); + + it('accepts exactly five non-whitespace characters and trims removePlayerTag output', () => { + expect( + removePlayerTagSchema.parse({ ...removeBase, removalReason: ' abcde ' }).removalReason, + ).toBe('abcde'); + }); + + it('accepts exactly five non-whitespace characters and trims replacePlayerTag output', () => { + expect( + replacePlayerTagSchema.parse({ ...replaceBase, removalReason: ' abcde ' }).removalReason, + ).toBe('abcde'); + }); +}); diff --git a/packages/core/src/contracts/schemas/tag.ts b/packages/core/src/contracts/schemas/tag.ts index b55e5479..318ab136 100644 --- a/packages/core/src/contracts/schemas/tag.ts +++ b/packages/core/src/contracts/schemas/tag.ts @@ -71,7 +71,7 @@ export type AssignPlayerTagInput = z.infer; export const removePlayerTagSchema = playerTagSchema.pick({ playerId: true }).extend({ tagKey: TagKeySchema, - removalReason: z.string().min(5), + removalReason: z.string().trim().min(5).optional(), removalActor: tagAssignRemoveSourceSchema, removalActorUserId: UuidSchema.nullable(), }); @@ -80,7 +80,7 @@ export type RemovePlayerTagInput = z.infer; // Atomic same-key swap (the mutable `level` tag): one actor performs both halves, // so the assign actor fields double as the removal actor fields. export const replacePlayerTagSchema = assignPlayerTagSchema.extend({ - removalReason: z.string().min(5), + removalReason: z.string().trim().min(5), }); export type ReplacePlayerTagInput = z.infer; diff --git a/packages/core/src/pam/tag/__tests__/tag.router.int.test.ts b/packages/core/src/pam/tag/__tests__/tag.router.int.test.ts index f25bcb39..46cc4acb 100644 --- a/packages/core/src/pam/tag/__tests__/tag.router.int.test.ts +++ b/packages/core/src/pam/tag/__tests__/tag.router.int.test.ts @@ -56,6 +56,10 @@ async function seedTag(key: TagKey) { await db.drizzle.db.insert(tag).values({ key }); } +async function storedPlayerTags() { + return db.drizzle.db.select().from(playerTag); +} + async function storedRules() { return db.drizzle.db.select().from(tagRule); } @@ -241,3 +245,107 @@ describe('tag router error mapping', () => { ).rejects.toMatchObject({ code: 'NOT_FOUND' }); }); }); + +describe('tag router player-tag removal', () => { + it('normalizes the removal reason before persistence and audit emission', async () => { + await seedTag('high_roller'); + const { router, events } = build(allowingGuard()); + const assignInput = { + playerId: UID, + tagKey: 'high_roller' as TagKey, + assignReason: 'manual review', + assignActor: 'manual' as const, + }; + await call(router.assignPlayerTag, assignInput, { context: AUTHED_CTX }); + events.emit.mockClear(); + + const result = await call( + router.removePlayerTag, + { + playerId: UID, + tagKey: 'high_roller', + removalReason: ' cleared after review ', + removalActor: 'manual', + }, + { context: AUTHED_CTX }, + ); + + expect(result.removalReason).toBe('cleared after review'); + expect((await storedPlayerTags()).at(0)?.removalReason).toBe('cleared after review'); + expect(events.emit).toHaveBeenCalledWith( + 'tag.player.removed', + expect.objectContaining({ reason: 'cleared after review' }), + ); + }); + + it('rejects a whitespace-only removal reason at the API boundary', async () => { + await seedTag('high_roller'); + const { router } = build(allowingGuard()); + + await expect( + call( + router.removePlayerTag, + { + playerId: UID, + tagKey: 'high_roller', + removalReason: ' ', + removalActor: 'manual', + }, + { context: AUTHED_CTX }, + ), + ).rejects.toBeInstanceOf(ORPCError); + expect(await storedPlayerTags()).toHaveLength(0); + }); + + it('requires a reason when removing a sticky tag', async () => { + await db.drizzle.db.insert(tag).values({ key: 'vip', isSticky: true }); + const { router } = build(allowingGuard()); + await call( + router.assignPlayerTag, + { + playerId: UID, + tagKey: 'vip', + assignReason: 'manual review', + assignActor: 'manual', + }, + { context: AUTHED_CTX }, + ); + + await expect( + call( + router.removePlayerTag, + { playerId: UID, tagKey: 'vip', removalActor: 'manual' }, + { context: AUTHED_CTX }, + ), + ).rejects.toMatchObject({ code: 'BAD_REQUEST' }); + expect((await storedPlayerTags()).at(0)?.removedAt).toBeNull(); + }); + + it('allows removing a non-sticky tag without a reason', async () => { + await seedTag('high_roller'); + const { router, events } = build(allowingGuard()); + await call( + router.assignPlayerTag, + { + playerId: UID, + tagKey: 'high_roller', + assignReason: 'manual review', + assignActor: 'manual', + }, + { context: AUTHED_CTX }, + ); + events.emit.mockClear(); + + await call( + router.removePlayerTag, + { playerId: UID, tagKey: 'high_roller', removalActor: 'manual' }, + { context: AUTHED_CTX }, + ); + + expect((await storedPlayerTags()).at(0)?.removalReason).toBe('manual tag removal'); + expect(events.emit).toHaveBeenCalledWith( + 'tag.player.removed', + expect.objectContaining({ reason: 'manual tag removal' }), + ); + }); +}); diff --git a/packages/core/src/pam/tag/router/index.ts b/packages/core/src/pam/tag/router/index.ts index 909113c3..71f29264 100644 --- a/packages/core/src/pam/tag/router/index.ts +++ b/packages/core/src/pam/tag/router/index.ts @@ -8,6 +8,7 @@ import { TagAssignmentNotFoundError, TagKeyConflictError, TagInUseError, + TagRemovalReasonRequiredError, } from '../service/tag.service.js'; import { TagRuleService, TagRuleNotFoundError } from '../service/tag-rule.service.js'; @@ -52,11 +53,16 @@ export function createTagRouter(tag: TagService, rule: TagRuleService, adminGuar removePlayerTag: os.removePlayerTag.handler(async ({ context, input }) => { await adminGuard.assert(context, 'tag', 'delete'); - return mapErrors({ NOT_FOUND: [TagNotFoundError, TagAssignmentNotFoundError] }, () => - tag.removePlayerTag( - { ...input, removalActorUserId: getUserId(context) }, - context.clientMeta, - ), + return mapErrors( + { + BAD_REQUEST: TagRemovalReasonRequiredError, + NOT_FOUND: [TagNotFoundError, TagAssignmentNotFoundError], + }, + () => + tag.removePlayerTag( + { ...input, removalActorUserId: getUserId(context) }, + context.clientMeta, + ), ); }), diff --git a/packages/core/src/pam/tag/service/tag.service.ts b/packages/core/src/pam/tag/service/tag.service.ts index b29713ad..62cca6a6 100644 --- a/packages/core/src/pam/tag/service/tag.service.ts +++ b/packages/core/src/pam/tag/service/tag.service.ts @@ -65,6 +65,20 @@ export const TagInUseError = makeConflictError( 'TagInUseError', 'Tag is still referenced by a player tag or tag rule and cannot be deleted', ); +export const TagRemovalReasonRequiredError = makeConflictError( + 'TagRemovalReasonRequiredError', + 'A removal reason is required for sticky tags', +); + +const DEFAULT_MANUAL_REMOVAL_REASON = 'manual tag removal'; + +function resolveRemovalReason(reason: string | undefined, isSticky: boolean) { + const normalized = reason?.trim(); + if (isSticky && !normalized) { + throw new TagRemovalReasonRequiredError(); + } + return normalized ?? DEFAULT_MANUAL_REMOVAL_REASON; +} // "First evidence wins per breach dimension, but a dimension that was never recorded gets // filled in the moment it's observed." Never overwrites an already-populated dimension with @@ -342,6 +356,7 @@ export class TagService implements PlayerTags { private async _removePlayerTagOnTx(trx: DrizzleTx, args: RemovePlayerTagInput) { const foundTag = await this._findTagByKeyOrThrow(args.tagKey, trx); + const removalReason = resolveRemovalReason(args.removalReason, foundTag.isSticky); const active = findOneOrThrow( await trx .select() @@ -360,7 +375,7 @@ export class TagService implements PlayerTags { .update(playerTag) .set({ removedAt: new Date(), - removalReason: args.removalReason, + removalReason, removalActor: args.removalActor, removalActorUserId: args.removalActorUserId, }) @@ -374,6 +389,7 @@ export class TagService implements PlayerTags { const db = this.drizzle.db; const result = await db.transaction(async (trx) => { const foundTag = await this._findTagByKeyOrThrow(args.tagKey, trx); + const removalReason = resolveRemovalReason(args.removalReason, foundTag.isSticky); const active = findOneOrThrow( await trx .select() @@ -392,7 +408,7 @@ export class TagService implements PlayerTags { .update(playerTag) .set({ removedAt: new Date(), - removalReason: args.removalReason, + removalReason, removalActor: args.removalActor, removalActorUserId: args.removalActorUserId, }) @@ -403,7 +419,7 @@ export class TagService implements PlayerTags { void this.event.emit('tag.player.removed', { playerId: args.playerId, tagKey: args.tagKey, - reason: args.removalReason, + reason: result.removalReason ?? DEFAULT_MANUAL_REMOVAL_REASON, actorId: args.removalActorUserId ?? SYSTEM_ACTOR_ID, ip: meta?.ip ?? null, userAgent: meta?.userAgent ?? null,