From 95cdd15229bdfbd82c2d3a93126f4018d67d2665 Mon Sep 17 00:00:00 2001 From: guarzo Date: Mon, 10 Aug 2026 15:05:49 -0400 Subject: [PATCH 1/2] fix(admin): name the characters the audit details column left as ids The Details column printed a raw character id where the Target column three cells left resolved the same class of referent to a name, so one row gave two different answers about the same character. Resolution is now keyed off the `details` field name the same way `detailAccountNames` already was, which is what lets a single `access_list.holder_replaced` row name both of the characters it carries. The ids ride the `character` fetch that was already in flight, so the query count is unchanged. Covers every declared action whose payload carries a character id -- nine keys across eight actions -- because a partial fix would leave the remaining raw ids looking like a data problem rather than a rendering one. Character rows are hard-deleted, so an id that no longer resolves falls back to rendering whole, exactly as it did before. --- src/app/admin/audit/page.tsx | 1 + src/app/admin/audit/summarize.ts | 53 ++++++++++--- src/services/audit.ts | 78 ++++++++++++++++++- tests/audit-resolve.test.ts | 66 ++++++++++++++++ tests/audit-summarize.test.ts | 125 +++++++++++++++++++++++++++++++ 5 files changed, 309 insertions(+), 14 deletions(-) diff --git a/src/app/admin/audit/page.tsx b/src/app/admin/audit/page.tsx index d5ead1bf..c40c9695 100644 --- a/src/app/admin/audit/page.tsx +++ b/src/app/admin/audit/page.tsx @@ -682,6 +682,7 @@ export default async function AdminAuditPage({ roleNames, tierLabels, new Map(Object.entries(r.detailAccountNames)), + new Map(Object.entries(r.detailCharacterNames)), )} /> ) : ( diff --git a/src/app/admin/audit/summarize.ts b/src/app/admin/audit/summarize.ts index e19f9c7c..9516436d 100644 --- a/src/app/admin/audit/summarize.ts +++ b/src/app/admin/audit/summarize.ts @@ -27,6 +27,7 @@ type Render = ( roleNames: ReadonlyMap, labels: Record, accountNames: ReadonlyMap, + characterNames: ReadonlyMap, ) => string; /** @@ -164,6 +165,27 @@ function accountRef(word: string, key: string): Part { }); } +/** A character id resolved to its name, e.g. `main Probe Kid`. Unlike + * `accountRef`'s uuid or `roleRef`'s snowflake, a miss degrades to the raw id + * via `fmt`, not to a shortened form: a 9-10 digit character id is already + * legible, so truncating it would destroy information for no gain, and + * rendering it whole keeps the miss path byte-identical to what this column + * showed before names were resolved -- an unresolvable row is no worse than + * it is today. `characterNames` is keyed by the `details` field name, the + * same convention `accountNames` uses -- see `resolveAuditIdentities`'s + * `detailCharacterNames`. The miss path is deliberately `labelled`'s exactly + * -- render on any defined value, nothing only when the key is absent -- so a + * row this cannot resolve reads exactly as it did before, whatever shape the + * unenforced jsonb column turns out to hold. */ +function characterRef(word: string, key: string): Part { + return part([key], (d, _roleNames, _labels, _accountNames, characterNames) => { + const raw = d[key]; + if (raw === undefined) return ""; + const name = characterNames.get(key); + return `${word} ${name ?? fmt(raw)}`; + }); +} + /** A single Discord role id, resolved to its configured tier name where one * exists. The id in `roleId` (role_strip_failed, role_sync_failed) is always * one of `discord.roleIds`' values -- the same set `roleNames` is built from @@ -263,7 +285,7 @@ const PARTS: Record = { "tier.approved": [tierTransition("from", "to"), flag("locked", "locked")], "account.merged": [ shortRef("absorbed", "sourceAccountId"), - labelled("character", "characterId"), + characterRef("character", "characterId"), ], // The only action in the repo whose payload exceeds FALLBACK_KEYS, and the // key the fallback dropped was the price — the reason the row exists. @@ -293,17 +315,21 @@ const PARTS: Record = { // noteChange's doc. "payout.notes_changed": [noteChange("had", "has")], "status.changed": [transition("from", "to"), flag("self", "self-service")], - "admin.bootstrap_granted": [labelled("character", "characterId")], - "account.created": [labelled("main", "mainCharacterId")], - "account.main_changed": [labelled("main →", "mainCharacterId")], + "admin.bootstrap_granted": [characterRef("character", "characterId")], + "account.created": [characterRef("main", "mainCharacterId")], + "account.main_changed": [characterRef("main →", "mainCharacterId")], // Same shape as the member-driven row above; the two are separate actions // only so the log can say who drove it (services/accounts.ts). - "admin.main_changed": [labelled("main →", "mainCharacterId")], + "admin.main_changed": [characterRef("main →", "mainCharacterId")], "character.reclaimed": [accountRef("from", "fromAccount")], "character.unlinked": [scalar("name"), flag("wasMain", "was main")], "token.invalidated": [scalar("reason")], "token.verify_failed": [scalar("error")], - "token.subject_mismatch": [labelled("subject", "subjectCharacterId")], + // The subject is by construction a different character from the row's + // target (jobs/token-health.ts:61 -- the mismatch is the whole point), so + // it will often be a character with no linked account and thus no name to + // resolve. A raw id here is expected, not a sign the lookup is broken. + "token.subject_mismatch": [characterRef("subject", "subjectCharacterId")], // Two writers, two payload shapes. token-health computes a shortfall against // config and sends `missingScopes`; the location job sends the single scope // whose read ESI refused. Each renders nothing for the other's keys, so the @@ -316,10 +342,10 @@ const PARTS: Record = { "tier.unlocked": [tierLabelled("was", "tier")], "status.note_changed": [noteChange("had", "has")], "character.owner_mismatch": [labelled("detected by", "detectedBy")], - "access_list.holder_designated": [labelled("character", "characterId")], + "access_list.holder_designated": [characterRef("character", "characterId")], "access_list.holder_replaced": [ - labelled("character", "characterId"), - labelled("was", "previousCharacterId"), + characterRef("character", "characterId"), + characterRef("was", "previousCharacterId"), ], "access_list.watch_added": [accessListRef("name", "accessListId")], "access_list.watch_removed": [accessListRef("name", "accessListId")], @@ -379,8 +405,10 @@ const FALLBACK_KEYS = 3; * tier value to this deployment's configured label. `accountNames` maps a * `details` field name (not a uuid) to the account it resolved to -- see * `resolveAuditIdentities`'s `detailAccountNames` and `accountRef` above. - * All three passed in rather than imported so this module stays a pure - * function of its arguments and needs no env to test. + * `characterNames` is the same idea for character ids -- see + * `detailCharacterNames` and `characterRef` above. All four passed in rather + * than imported so this module stays a pure function of its arguments and + * needs no env to test. */ export function summarizeDetails( action: string, @@ -388,6 +416,7 @@ export function summarizeDetails( roleNames: ReadonlyMap = new Map(), labels: Record = {}, accountNames: ReadonlyMap = new Map(), + characterNames: ReadonlyMap = new Map(), ): string { const d = (details && typeof details === "object" ? details : {}) as Record< string, @@ -397,7 +426,7 @@ export function summarizeDetails( const parts = PARTS[action]; if (parts) { const rendered = parts - .map((p) => p(d, roleNames, labels, accountNames)) + .map((p) => p(d, roleNames, labels, accountNames, characterNames)) .filter(Boolean); const declared = new Set(parts.flatMap((p) => p.keys)); const hidden = Object.keys(d).filter((k) => !declared.has(k)).length; diff --git a/src/services/audit.ts b/src/services/audit.ts index 4e41b4b2..fb13f845 100644 --- a/src/services/audit.ts +++ b/src/services/audit.ts @@ -84,6 +84,12 @@ export type ResolvedAuditRow = typeof auditLog.$inferSelect & { * without knowing which action wrote it. Empty for every row that carries * no such field — see `DETAIL_ACCOUNT_KEYS`. */ detailAccountNames: Record; + /** Character ids embedded in `details`, resolved to that character's name — + * see `DETAIL_CHARACTER_KEYS`. Keyed by the `details` field name rather than + * by the id: `access_list.holder_replaced` carries two different character + * ids on the same row (`characterId` and `previousCharacterId`), and an + * id-keyed map could not tell a caller which field either name came from. */ + detailCharacterNames: Record; }; const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; @@ -218,6 +224,51 @@ const DETAIL_ACCOUNT_KEYS: Readonly> = { "character.reclaimed": ["fromAccount"], }; +/** + * `details` keys that hold a character id, per action — same idea as + * `DETAIL_ACCOUNT_KEYS`, one join fewer. These ids name a character this app + * had a row for at write time, but a character row is HARD-deleted the moment + * it is unlinked or reclaimed (accounts.ts, both the reclaim and unlink + * paths), so resolution here is best-effort: the renderer falls back to the + * raw id, same as any other unresolved detail. + * + * `access_list.watch_added`/`watch_removed`'s `accessListId` is deliberately + * NOT listed here — it is an EVE access-list id, not a character id, and is + * exactly the false-positive a bare key-name heuristic would produce (see the + * `NAMESPACE_TARGET_KIND` comment above, which flags this same namespace for + * the same reason on the target column). + * + * `token.subject_mismatch`'s `subjectCharacterId` is, by construction, a + * DIFFERENT character from the row's own target (src/jobs/token-health.ts): + * it names whichever character the mismatched token actually belongs to, so + * it will often belong to a still-live character on someone else's account + * and resolve fine, but on occasion belongs to one already gone — that is + * expected, not a bug. + */ +const DETAIL_CHARACTER_KEYS: Readonly> = { + "account.created": ["mainCharacterId"], + "account.main_changed": ["mainCharacterId"], + "admin.main_changed": ["mainCharacterId"], + "account.merged": ["characterId"], + "admin.bootstrap_granted": ["characterId"], + "access_list.holder_designated": ["characterId"], + "access_list.holder_replaced": ["characterId", "previousCharacterId"], + "token.subject_mismatch": ["subjectCharacterId"], +}; + +/** + * `jsonb` gives back a genuine number for anything written as one, but the + * column's shape is unenforced by the type system — a hand-inserted or + * legacy row could hold the same id as a digit string instead. Accept either + * and reject everything else, rather than assuming today's writer shape is + * the only one that will ever be read. + */ +function detailCharacterId(raw: unknown): number | null { + if (typeof raw === "number") return Number.isInteger(raw) ? raw : null; + if (typeof raw === "string" && DIGITS_RE.test(raw)) return Number(raw); + return null; +} + /** * Resolves actor/target ids to human (main character) names in a fixed, * small number of batched queries, independent of row count: @@ -244,6 +295,7 @@ export async function resolveAuditIdentities( const targetCharacterIds = new Set(); const targetDiscordIds = new Set(); const targetPayoutIds = new Set(); + const detailCharacterIds = new Set(); for (const r of rows) { if (r.actor !== "system" && UUID_RE.test(r.actor)) accountIds.add(r.actor); @@ -264,6 +316,13 @@ export async function resolveAuditIdentities( const raw = r.details?.[key]; if (typeof raw === "string" && UUID_RE.test(raw)) accountIds.add(raw); } + // Same idea, one join fewer: a character id living inside `details`, + // folded into the same `characterIds` set seeded below so it rides the + // already in-flight `character` query rather than paying for a new one. + for (const key of DETAIL_CHARACTER_KEYS[r.action] ?? []) { + const id = detailCharacterId(r.details?.[key]); + if (id !== null) detailCharacterIds.add(id); + } } const [directAccounts, links, payoutOperations] = await Promise.all([ @@ -332,7 +391,7 @@ export async function resolveAuditIdentities( } } - const characterIds = new Set(targetCharacterIds); + const characterIds = new Set([...targetCharacterIds, ...detailCharacterIds]); for (const a of accountById.values()) { if (a.mainCharacterId !== null) characterIds.add(a.mainCharacterId); } @@ -410,7 +469,22 @@ export async function resolveAuditIdentities( if (name !== null) detailAccountNames[key] = name; } - return { ...r, actorName, actorKind, targetName, targetKind, detailAccountNames }; + const detailCharacterNames: Record = {}; + for (const key of DETAIL_CHARACTER_KEYS[r.action] ?? []) { + const id = detailCharacterId(r.details?.[key]); + const name = id !== null ? (nameByCharacterId.get(id) ?? null) : null; + if (name !== null) detailCharacterNames[key] = name; + } + + return { + ...r, + actorName, + actorKind, + targetName, + targetKind, + detailAccountNames, + detailCharacterNames, + }; }); } diff --git a/tests/audit-resolve.test.ts b/tests/audit-resolve.test.ts index b247f54b..21b36097 100644 --- a/tests/audit-resolve.test.ts +++ b/tests/audit-resolve.test.ts @@ -251,6 +251,72 @@ describe("resolveAuditIdentities / queryAuditLog resolution", () => { expect(row.detailAccountNames).toEqual({}); }); + it("resolves a detail character id (account.main_changed's mainCharacterId)", async () => { + const acc = await seedAccount(ctx.db); + await seedCharacter(ctx.db, cfg, { + id: 90008, + accountId: acc.id, + name: "New Main", + main: true, + }); + await logAudit(ctx.db, { + actor: acc.id, + action: "account.main_changed", + target: acc.id, + details: { mainCharacterId: 90008 }, + }); + const [row] = await queryAuditLog(ctx.db); + expect(row.detailCharacterNames).toEqual({ mainCharacterId: "New Main" }); + }); + + it("resolves both characterId and previousCharacterId on a holder_replaced row to distinct names", async () => { + const acc1 = await seedAccount(ctx.db); + const acc2 = await seedAccount(ctx.db); + await seedCharacter(ctx.db, cfg, { + id: 90009, + accountId: acc1.id, + name: "New Holder", + }); + await seedCharacter(ctx.db, cfg, { + id: 90010, + accountId: acc2.id, + name: "Old Holder", + }); + await logAudit(ctx.db, { + actor: "system", + action: "access_list.holder_replaced", + target: "some-list", + details: { characterId: 90009, previousCharacterId: 90010 }, + }); + const [row] = await queryAuditLog(ctx.db); + expect(row.detailCharacterNames).toEqual({ + characterId: "New Holder", + previousCharacterId: "Old Holder", + }); + }); + + it("leaves detailCharacterNames empty when the detail character id doesn't exist", async () => { + await logAudit(ctx.db, { + actor: "system", + action: "account.main_changed", + target: "all", + details: { mainCharacterId: 424242 }, + }); + const [row] = await queryAuditLog(ctx.db); + expect(row.detailCharacterNames).toEqual({}); + }); + + it("leaves detailCharacterNames empty for actions that don't declare a character-id detail key", async () => { + await logAudit(ctx.db, { + actor: "system", + action: "tier.changed", + target: "all", + details: { from: "member", to: "alumni" }, + }); + const [row] = await queryAuditLog(ctx.db); + expect(row.detailCharacterNames).toEqual({}); + }); + it("resolves a full page of 200+ rows with a small, constant number of queries (no N+1)", async () => { const accounts = await Promise.all( Array.from({ length: 20 }, () => seedAccount(ctx.db)), diff --git a/tests/audit-summarize.test.ts b/tests/audit-summarize.test.ts index c32fb704..6989bce3 100644 --- a/tests/audit-summarize.test.ts +++ b/tests/audit-summarize.test.ts @@ -534,6 +534,131 @@ describe("summarizeDetails with configured tier labels", () => { ).toBe("character 90000002, was 90000001"); }); + it("renders a resolved character name in place of a raw id", () => { + const names = new Map([["mainCharacterId", "Probe Kid"]]); + expect( + summarizeDetails( + "account.main_changed", + { mainCharacterId: 90000001 }, + new Map(), + {}, + new Map(), + names, + ), + ).toBe("main → Probe Kid"); + expect( + summarizeDetails( + "account.created", + { mainCharacterId: 90000001 }, + new Map(), + {}, + new Map(), + names, + ), + ).toBe("main Probe Kid"); + expect( + summarizeDetails( + "account.merged", + { + sourceAccountId: "7f3a2b1c-0000-4000-8000-000000000001", + characterId: 90000001, + }, + new Map(), + {}, + new Map(), + new Map([["characterId", "Probe Kid"]]), + ), + ).toBe("absorbed 7f3a2b…, character Probe Kid"); + expect( + summarizeDetails( + "admin.bootstrap_granted", + { characterId: 90000001 }, + new Map(), + {}, + new Map(), + new Map([["characterId", "Probe Kid"]]), + ), + ).toBe("character Probe Kid"); + expect( + summarizeDetails( + "access_list.holder_designated", + { characterId: 90000001 }, + new Map(), + {}, + new Map(), + new Map([["characterId", "Probe Kid"]]), + ), + ).toBe("character Probe Kid"); + expect( + summarizeDetails( + "token.subject_mismatch", + { subjectCharacterId: 90000003 }, + new Map(), + {}, + new Map(), + new Map([["subjectCharacterId", "Some Alt"]]), + ), + ).toBe("subject Some Alt"); + }); + + it("renders both sides of a holder replacement by their own field-name key", () => { + // The case that proves keying by details field name, not by id: the same + // action names two different characters under two different keys, and + // each must resolve to its own name rather than the other's. + expect( + summarizeDetails( + "access_list.holder_replaced", + { characterId: 90000002, previousCharacterId: 90000001 }, + new Map(), + {}, + new Map(), + new Map([ + ["characterId", "New Holder"], + ["previousCharacterId", "Old Holder"], + ]), + ), + ).toBe("character New Holder, was Old Holder"); + }); + + it("degrades a character id to the raw value when no name resolves", () => { + // No characterNames map at all -- the default parameter, exercised the + // way a caller that hasn't been updated for this field still would be. + // Output must be byte-identical to what this line rendered before names + // were resolved. + expect(summarizeDetails("account.main_changed", { mainCharacterId: 90000001 })).toBe( + "main → 90000001", + ); + }); + + it("does not turn a declared character key into a hidden-key count", () => { + expect( + summarizeDetails( + "access_list.holder_designated", + { characterId: 90000001 }, + new Map(), + {}, + new Map(), + new Map([["characterId", "Probe Kid"]]), + ), + ).not.toContain("more"); + }); + + // The jsonb column's shape is unenforced, so a hand-inserted or legacy row + // could hold something that is not an id at all. Whatever that is, the miss + // path must render it the way this column always has (`labelled` -> `fmt`) + // rather than silently dropping the part and leaving the row blank. + it("falls back like the plain renderer for a value that is not an id", () => { + expect(summarizeDetails("account.main_changed", { mainCharacterId: null })).toBe( + "main → ?", + ); + expect(summarizeDetails("account.created", { mainCharacterId: true })).toBe( + "main true", + ); + // Absent is the one case the part renders nothing for, leaving the row + // with no summary at all rather than a labelled `?`. + expect(summarizeDetails("account.created", {})).toBe("—"); + }); + it("renders a watch change with the list's name, and without it", () => { expect( summarizeDetails("access_list.watch_added", { From 5be91463bc3a724eff21076078762cff55a7c931 Mon Sep 17 00:00:00 2001 From: guarzo Date: Mon, 10 Aug 2026 15:19:39 -0400 Subject: [PATCH 2/2] fix(audit): correct comment claims and refuse mis-coercing digit strings Polish pass on the detail-character-name change: - six comments corrected against the code they cite (the widened fetch is a separate set unioned later, not an in-flight query; holder_replaced's two ids can be equal; subject_mismatch can name a character never held here; NAMESPACE_TARGET_KIND reaches the opposite conclusion and does so deliberately; the step-3 enumeration omitted the new source; labelled's doc example cited a character id no caller passes it any more) - detailCharacterId now round-trips the digit-string branch through String(), so "0090012" and 17-digit strings degrade to the raw-id render instead of confidently naming whichever character they coerce to --- src/app/admin/audit/summarize.ts | 11 +++--- src/services/audit.ts | 59 ++++++++++++++++++++------------ tests/audit-resolve.test.ts | 39 +++++++++++++++++++++ 3 files changed, 83 insertions(+), 26 deletions(-) diff --git a/src/app/admin/audit/summarize.ts b/src/app/admin/audit/summarize.ts index 9516436d..e2d9b3f3 100644 --- a/src/app/admin/audit/summarize.ts +++ b/src/app/admin/audit/summarize.ts @@ -102,7 +102,9 @@ function scalar(key: string): Part { return part([key], (d) => (d[key] === undefined ? "" : fmt(d[key]))); } -/** A payload value behind a fixed word, e.g. `character 90000001`. */ +/** A payload value behind a fixed word, e.g. `detected by token-health`. For a + * value that is a character id, use `characterRef` — it renders exactly this on + * the miss path but resolves a name when there is one. */ function labelled(word: string, key: string): Part { return part([key], (d) => (d[key] === undefined ? "" : `${word} ${fmt(d[key])}`)); } @@ -326,9 +328,10 @@ const PARTS: Record = { "token.invalidated": [scalar("reason")], "token.verify_failed": [scalar("error")], // The subject is by construction a different character from the row's - // target (jobs/token-health.ts:61 -- the mismatch is the whole point), so - // it will often be a character with no linked account and thus no name to - // resolve. A raw id here is expected, not a sign the lookup is broken. + // target (jobs/token-health.ts:61 -- the mismatch is the whole point). It + // names whichever character the token's EVE subject belongs to, which this + // app need never have held a row for, so a raw id here is expected rather + // than a sign the lookup is broken. "token.subject_mismatch": [characterRef("subject", "subjectCharacterId")], // Two writers, two payload shapes. token-health computes a shortfall against // config and sends `missingScopes`; the location job sends the single scope diff --git a/src/services/audit.ts b/src/services/audit.ts index fb13f845..72b41adf 100644 --- a/src/services/audit.ts +++ b/src/services/audit.ts @@ -86,9 +86,10 @@ export type ResolvedAuditRow = typeof auditLog.$inferSelect & { detailAccountNames: Record; /** Character ids embedded in `details`, resolved to that character's name — * see `DETAIL_CHARACTER_KEYS`. Keyed by the `details` field name rather than - * by the id: `access_list.holder_replaced` carries two different character - * ids on the same row (`characterId` and `previousCharacterId`), and an - * id-keyed map could not tell a caller which field either name came from. */ + * by the id: `access_list.holder_replaced` carries two character ids on the + * same row (`characterId` and `previousCharacterId`), and an id-keyed map + * could not tell a caller which field either name came from — nor hold both + * when a re-designation of the sitting holder makes the two ids equal. */ detailCharacterNames: Record; }; @@ -226,24 +227,28 @@ const DETAIL_ACCOUNT_KEYS: Readonly> = { /** * `details` keys that hold a character id, per action — same idea as - * `DETAIL_ACCOUNT_KEYS`, one join fewer. These ids name a character this app - * had a row for at write time, but a character row is HARD-deleted the moment - * it is unlinked or reclaimed (accounts.ts, both the reclaim and unlink - * paths), so resolution here is best-effort: the renderer falls back to the - * raw id, same as any other unresolved detail. + * `DETAIL_ACCOUNT_KEYS`, one join fewer. Resolution is best-effort in both + * directions: a character row is HARD-deleted the moment it is unlinked or + * reclaimed (accounts.ts, both the reclaim and unlink paths), and + * `token.subject_mismatch` below can name a character this app never held a + * row for at all. Either way the renderer falls back to the raw id, same as + * any other unresolved detail. * * `access_list.watch_added`/`watch_removed`'s `accessListId` is deliberately * NOT listed here — it is an EVE access-list id, not a character id, and is - * exactly the false-positive a bare key-name heuristic would produce (see the - * `NAMESPACE_TARGET_KIND` comment above, which flags this same namespace for - * the same reason on the target column). + * exactly the false-positive a bare key-name heuristic would produce. The + * `NAMESPACE_TARGET_KIND` comment above flags the same namespace but reaches + * the opposite conclusion, and deliberately: there the collision is tolerated + * because a ~6-digit list id will not match a ~9-10-digit character id in + * practice, and nothing user-facing reads that resolution anyway. Here the + * result is rendered, so the key is excluded outright rather than left to a + * size argument. * * `token.subject_mismatch`'s `subjectCharacterId` is, by construction, a - * DIFFERENT character from the row's own target (src/jobs/token-health.ts): - * it names whichever character the mismatched token actually belongs to, so - * it will often belong to a still-live character on someone else's account - * and resolve fine, but on occasion belongs to one already gone — that is - * expected, not a bug. + * DIFFERENT character from the row's own target (src/jobs/token-health.ts:61 — + * the mismatch is the whole point): it names whichever character the token's + * EVE subject actually belongs to, which need never have been linked here. + * A raw id on those rows is expected, not a sign the lookup is broken. */ const DETAIL_CHARACTER_KEYS: Readonly> = { "account.created": ["mainCharacterId"], @@ -262,10 +267,19 @@ const DETAIL_CHARACTER_KEYS: Readonly> = { * legacy row could hold the same id as a digit string instead. Accept either * and reject everything else, rather than assuming today's writer shape is * the only one that will ever be read. + * + * The string branch round-trips through `String` rather than trusting + * `Number`, because this path renders a NAME: `"0090000001"` and a 17-digit + * string both coerce to some other character's id, and the summary would then + * confidently name a character the payload never referred to. Failing the + * round-trip degrades to the raw-id render, which is honest. */ function detailCharacterId(raw: unknown): number | null { if (typeof raw === "number") return Number.isInteger(raw) ? raw : null; - if (typeof raw === "string" && DIGITS_RE.test(raw)) return Number(raw); + if (typeof raw === "string" && DIGITS_RE.test(raw)) { + const id = Number(raw); + return String(id) === raw ? id : null; + } return null; } @@ -280,8 +294,8 @@ function detailCharacterId(raw: unknown): number | null { * 2b. names of deleted payout operations, recovered from the `payout.deleted` * audit row's own details — skipped entirely if step 1 resolved every * payout target already - * 3. every character name needed (target characters + all main characters - * collected above), in one shot + * 3. every character name needed (target characters + character ids read out + * of `details` + all main characters collected above), in one shot * Anything that doesn't resolve is left as `null`/`"unresolved"`; the raw * `actor`/`target` strings on the row are always preserved unchanged. */ @@ -316,9 +330,10 @@ export async function resolveAuditIdentities( const raw = r.details?.[key]; if (typeof raw === "string" && UUID_RE.test(raw)) accountIds.add(raw); } - // Same idea, one join fewer: a character id living inside `details`, - // folded into the same `characterIds` set seeded below so it rides the - // already in-flight `character` query rather than paying for a new one. + // Same idea, one join fewer: a character id living inside `details`. + // Collected separately only because the name lookup needs to know which + // ids came from here; unioned into `characterIds` at step 3 so it shares + // that one query rather than paying for a new one. for (const key of DETAIL_CHARACTER_KEYS[r.action] ?? []) { const id = detailCharacterId(r.details?.[key]); if (id !== null) detailCharacterIds.add(id); diff --git a/tests/audit-resolve.test.ts b/tests/audit-resolve.test.ts index 21b36097..774ebcdb 100644 --- a/tests/audit-resolve.test.ts +++ b/tests/audit-resolve.test.ts @@ -317,6 +317,45 @@ describe("resolveAuditIdentities / queryAuditLog resolution", () => { expect(row.detailCharacterNames).toEqual({}); }); + it("resolves a character id the payload carried as a digit string", async () => { + const acc = await seedAccount(ctx.db); + await seedCharacter(ctx.db, cfg, { + id: 90011, + accountId: acc.id, + name: "String Main", + main: true, + }); + await logAudit(ctx.db, { + actor: acc.id, + action: "account.main_changed", + target: acc.id, + details: { mainCharacterId: "90011" }, + }); + const [row] = await queryAuditLog(ctx.db); + expect(row.detailCharacterNames).toEqual({ mainCharacterId: "String Main" }); + }); + + it("refuses a digit string that is not this id's own decimal form", async () => { + const acc = await seedAccount(ctx.db); + await seedCharacter(ctx.db, cfg, { + id: 90012, + accountId: acc.id, + name: "Zero Padded", + main: true, + }); + // `Number("0090012")` is 90012, so a naive coercion would render the + // summary as "main → Zero Padded" for a payload that never held that id. + // Naming the wrong character is worse than showing the raw value. + await logAudit(ctx.db, { + actor: acc.id, + action: "account.main_changed", + target: acc.id, + details: { mainCharacterId: "0090012" }, + }); + const [row] = await queryAuditLog(ctx.db); + expect(row.detailCharacterNames).toEqual({}); + }); + it("resolves a full page of 200+ rows with a small, constant number of queries (no N+1)", async () => { const accounts = await Promise.all( Array.from({ length: 20 }, () => seedAccount(ctx.db)),