feat: add AuditLogManager for dashboard settings audit logging#188
feat: add AuditLogManager for dashboard settings audit logging#188RedStar071 wants to merge 14 commits intomainfrom
Conversation
- Add AuditLog Prisma model with migration - Add AuditLogManager following PermissionNodeManager conventions - Add AuditLogChange type and PrismaJson augmentation - Integrate into SettingsContext with constructor/onPatch pattern - Wire audit logging into settings.patch.ts with structuredClone snapshot - Add GET /guilds/:guild/audit-logs endpoint with pagination - Add 18 unit tests for AuditLogManager
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request implements a comprehensive audit logging system for guild settings modifications. It introduces type-safe audit action definitions, hash-chained audit event storage in PostgreSQL, deterministic envelope hashing for integrity verification, an AuditLogManager class for recording changes, API endpoints for audit retrieval, integration with the settings update flow, comprehensive test coverage, and a standalone verification script. The PR also updates dependencies, CI workflows, and development tooling. ChangesGuild Settings Audit Logging System
Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Introduces dashboard settings audit logging by adding a new AuditLog Prisma model plus a settings-side AuditLogManager, wiring it into the settings PATCH flow, and exposing a paginated API endpoint to read audit log entries.
Changes:
- Add
AuditLogPrisma model + migration (tableaudit_log, index on(guild_id, created_at DESC)). - Add
AuditLogManagerand integrate it intoSettingsContext, plus exportedreadSettingsAuditLog()helper. - Add
GET /guilds/:guild/audit-logsendpoint and add audit-log writes toPATCH /guilds/:guild/settings, plus unit tests forAuditLogManager.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/lib/database/settings/structures/AuditLogManager.test.ts | Adds unit tests for audit log manager behavior (constructor/onPatch/update/add/remove + section/value derivation). |
| src/routes/guilds/[guild]/settings.patch.ts | Adds fire-and-forget audit logging on settings mutation (currently has a snapshot correctness issue). |
| src/routes/guilds/[guild]/audit-logs.get.ts | Adds paginated endpoint to fetch audit logs for a guild (minor consistency issue vs other routes). |
| src/lib/types/Augments.d.ts | Adds PrismaJson augmentation type AuditLogChanges. |
| src/lib/database/settings/types.ts | Introduces AuditLogChange type. |
| src/lib/database/settings/structures/AuditLogManager.ts | Implements AuditLogManager with update/add/remove/write + section/value helpers. |
| src/lib/database/settings/index.ts | Re-exports AuditLogManager. |
| src/lib/database/settings/functions.ts | Adds readSettingsAuditLog() helper (currently returns cached manager, affecting snapshot use-cases). |
| src/lib/database/settings/context/SettingsContext.ts | Adds auditLog manager to SettingsContext and updates it on patch. |
| prisma/schema.prisma | Adds AuditLog model and Guild.auditLogs relation. |
| prisma/migrations/20260416120000_add_dashboard_audit_log/migration.sql | Creates audit_log table, index, and FK to guilds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { authenticated, canManage, ratelimit } from '#lib/api/utils'; | ||
| import { seconds } from '#utils/common'; | ||
| import { container } from '@sapphire/framework'; | ||
| import { HttpCodes, Route } from '@sapphire/plugin-api'; |
There was a problem hiding this comment.
This route uses the global container import instead of the this.container instance property used by the other guild routes in this folder. For consistency (and to make testing/mocking easier), prefer this.container.client / this.container.prisma here as in channels.get.ts, roles.get.ts, etc.
- Replace AuditChainHead per-tenant design with singleton id=default - Rewrite migration.sql: UUID PK on audit_event, actorType/targetType columns, hash as UNIQUE (not PK), no requestId/traceId columns - Remove sequence: bigint from AuditEnvelopeInput (no lastSequence) - Rewrite AuditLogManager#write: global advisory lock, findUnique by id=default, correct auditEvent column set (actorType, no requestId) - Regenerate Prisma client (singleton AuditChainHead shape) - Fix tests: makeHead singleton shape, drop lastSequence assertions
- Replace auditLog.findMany with auditEvent.findMany filtered by
tenantId + action IN DASHBOARD_AUDIT_ACTIONS
- Rename take/skip -> limit/offset (parseInt, clamped to [1,100])
- Add patchToChanges helper: auditDiff {before,after} -> {added,removed,changed}
- Parallel member resolution with fallback for unresolved actors
- Return DashboardAuditEntry[] with id=UUID, guildId from route param
- settings.patch: capture before/after snapshots around submit() and
pass both to auditLog.update(actorId, before, after)
|
@copilot resolve the merge conflicts in this pull request |
Agent-Logs-Url: https://github.com/wolfstar-project/wolfstar/sessions/fdaf6952-6f06-465f-9108-8d00604a0b16 Co-authored-by: RedStar071 <76824516+RedStar071@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
prisma/schema.prisma (2)
246-252: Operational consideration: Global chain head serialization.The singleton chain head (
id='default') means all audit writes across all guilds serialize through one advisory lock. For low-frequency dashboard settings changes, this is acceptable. If audit logging expands to high-frequency events, consider per-tenant chain heads to reduce lock contention.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prisma/schema.prisma` around lines 246 - 252, The AuditChainHead model uses a singleton id ("default") which serializes all tenants through one chain head; update the model to support per-tenant chain heads by adding a tenant identifier (e.g., guildId or tenantId String field), make the primary key include that tenant identifier (either by replacing the single id with guildId as `@id` or by introducing a composite @@id([guildId, id]) ), and ensure hash/updatedAt remain and map stays @@map("audit_chain_head"); adjust any code that references AuditChainHead.id to use the tenant-aware key (AuditChainHead.guildId/AuditChainHead.tenantId) so advisory locks can be taken per-tenant.
227-228: ⚡ Quick winConsider aligning ID column types with Guild model.
actorIdandtenantIduse unboundedTEXT, whilstGuild.idusesVARCHAR(19)(Discord snowflake size). Using consistent types would improve index efficiency and make the schema more self-documenting.♻️ Suggested schema alignment
- actorId String `@map`("actor_id") + actorId String `@map`("actor_id") `@db.VarChar`(19) ... - targetId String? `@map`("target_id") + targetId String? `@map`("target_id") `@db.VarChar`(19) ... - tenantId String? `@map`("tenant_id") + tenantId String? `@map`("tenant_id") `@db.VarChar`(19)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prisma/schema.prisma` around lines 227 - 228, actorId and tenantId are currently mapped to unbounded TEXT while Guild.id uses VARCHAR(19); change the Prisma field definitions for actorId and tenantId to use the same fixed-length DB type as Guild.id (e.g., String with `@db.VarChar`(19)) so index size and semantics match; update the fields actorId and tenantId in the relevant model(s) to use the VarChar(19) mapping and keep existing `@map` annotations intact to preserve column names.src/lib/database/settings/structures/AuditLogManager.ts (1)
125-133: 💤 Low valueConsider defensive handling of non-JSON-safe values.
#toJsonSafehandlesbigint, arrays, and plain objects, but edge cases likeDateobjects, functions, or symbols would either serialize incorrectly or fail silently. For guild settings data, these edge cases are unlikely, but adding explicit guards would improve robustness.♻️ More defensive implementation
`#toJsonSafe`(value: unknown): unknown { + if (value === undefined) return null; if (typeof value === 'bigint') return value.toString(); + if (value instanceof Date) return value.toISOString(); if (Array.isArray(value)) return value.map((v) => this.#toJsonSafe(v)); if (value !== null && typeof value === 'object') { + if (Object.getPrototypeOf(value) !== Object.prototype) { + throw new TypeError(`Cannot serialize non-plain object: ${value.constructor.name}`); + } return Object.fromEntries(Object.entries(value as Record<string, unknown>).map(([k, v]) => [k, this.#toJsonSafe(v)])); } return value; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/database/settings/structures/AuditLogManager.ts` around lines 125 - 133, The `#toJsonSafe` method currently only handles bigint, arrays, and plain objects; update it to defensively handle Date (convert to ISO string), Map/Set (convert to plain object/array), symbol (convert to string via Symbol.prototype.toString or fallback), functions (replace with a descriptive placeholder like "[Function]"), and undefined (convert to null), and ensure it falls back to a safe serializable representation for any other non-plain object types to avoid silent failures when AuditLogManager serializes guild settings; keep the recursive structure and use Object.entries for objects and Array.from for iterables so existing callers of `#toJsonSafe` continue to work.prisma/migrations/20260416120000_add_dashboard_audit_log/migration.sql (1)
1-1: Migration data loss consideration.
DROP TABLE ... CASCADEdestroys existingaudit_logdata and dependent objects. For development iterations, this is acceptable. Before production deployment, ensure appropriate data migration or archival strategy if preserving audit history is required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prisma/migrations/20260416120000_add_dashboard_audit_log/migration.sql` at line 1, The migration currently issues a destructive DROP TABLE IF EXISTS "audit_log" CASCADE which will erase all audit_log rows and dependent objects; update the migration in migration.sql to preserve production data by replacing the unconditional DROP with a safe upgrade path: either (a) create a new table (or ALTER the existing one) and migrate rows from "audit_log" into it, or (b) RENAME the existing "audit_log" to an archival table before creating the new schema, and include an explicit data-copy/transform step and/or a backup table creation so dependent objects are preserved; ensure all steps reference "audit_log" and the DROP is removed or guarded for dev-only runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/database/settings/structures/AuditLogEnvelope.ts`:
- Around line 21-27: The canonicalize function fails to return a string for
undefined (JSON.stringify(undefined) yields undefined), breaking its return
contract and array joining; update canonicalize to explicitly handle undefined
(e.g., return JSON.stringify(null) or another stable string) before the
typeof/object checks so calls to canonicalize (and Array.map + join) always
yield strings; reference the canonicalize function and ensure the undefined
branch is placed before the "typeof value !== 'object'" check.
In `@src/routes/guilds/`[guild]/audit-logs.get.ts:
- Around line 100-103: The current code builds uniqueActorIds then calls
guild.members.fetch for every id (resolvedMembers) which hits the API even for
cached members; change to a cache-first approach: for each id in uniqueActorIds
check guild.members.cache.get(id) and use serializeMember on cached ones,
collect only the ids missing from cache, call guild.members.fetch only for those
missing ids (Promise.all), then merge fetch results with cached results into the
same memberMap (keep keys as id and values from serializeMember) so you avoid
unnecessary API fetches while preserving existing memberMap construction.
In `@src/routes/guilds/`[guild]/settings.patch.ts:
- Line 49: The call to auditLog.update(request.auth!.id, before, after)
currently swallows all errors; update this to preserve async behavior but
surface failures by attaching a .catch handler that logs the error along with
the guild and user context (use request.auth!.id and the guild identifier and/or
before/after summary) instead of returning null, so failures to write audit
entries are recorded for troubleshooting while keeping the call non-blocking.
---
Nitpick comments:
In `@prisma/migrations/20260416120000_add_dashboard_audit_log/migration.sql`:
- Line 1: The migration currently issues a destructive DROP TABLE IF EXISTS
"audit_log" CASCADE which will erase all audit_log rows and dependent objects;
update the migration in migration.sql to preserve production data by replacing
the unconditional DROP with a safe upgrade path: either (a) create a new table
(or ALTER the existing one) and migrate rows from "audit_log" into it, or (b)
RENAME the existing "audit_log" to an archival table before creating the new
schema, and include an explicit data-copy/transform step and/or a backup table
creation so dependent objects are preserved; ensure all steps reference
"audit_log" and the DROP is removed or guarded for dev-only runs.
In `@prisma/schema.prisma`:
- Around line 246-252: The AuditChainHead model uses a singleton id ("default")
which serializes all tenants through one chain head; update the model to support
per-tenant chain heads by adding a tenant identifier (e.g., guildId or tenantId
String field), make the primary key include that tenant identifier (either by
replacing the single id with guildId as `@id` or by introducing a composite
@@id([guildId, id]) ), and ensure hash/updatedAt remain and map stays
@@map("audit_chain_head"); adjust any code that references AuditChainHead.id to
use the tenant-aware key (AuditChainHead.guildId/AuditChainHead.tenantId) so
advisory locks can be taken per-tenant.
- Around line 227-228: actorId and tenantId are currently mapped to unbounded
TEXT while Guild.id uses VARCHAR(19); change the Prisma field definitions for
actorId and tenantId to use the same fixed-length DB type as Guild.id (e.g.,
String with `@db.VarChar`(19)) so index size and semantics match; update the
fields actorId and tenantId in the relevant model(s) to use the VarChar(19)
mapping and keep existing `@map` annotations intact to preserve column names.
In `@src/lib/database/settings/structures/AuditLogManager.ts`:
- Around line 125-133: The `#toJsonSafe` method currently only handles bigint,
arrays, and plain objects; update it to defensively handle Date (convert to ISO
string), Map/Set (convert to plain object/array), symbol (convert to string via
Symbol.prototype.toString or fallback), functions (replace with a descriptive
placeholder like "[Function]"), and undefined (convert to null), and ensure it
falls back to a safe serializable representation for any other non-plain object
types to avoid silent failures when AuditLogManager serializes guild settings;
keep the recursive structure and use Object.entries for objects and Array.from
for iterables so existing callers of `#toJsonSafe` continue to work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8e1d5469-57e2-4218-900b-a36b253772b2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
package.jsonprisma/migrations/20260416120000_add_dashboard_audit_log/migration.sqlprisma/schema.prismasrc/lib/database/settings/auditActions.tssrc/lib/database/settings/context/SettingsContext.tssrc/lib/database/settings/functions.tssrc/lib/database/settings/index.tssrc/lib/database/settings/structures/AuditLogEnvelope.tssrc/lib/database/settings/structures/AuditLogManager.tssrc/lib/database/settings/types.tssrc/lib/types/Augments.d.tssrc/routes/guilds/[guild]/audit-logs.get.tssrc/routes/guilds/[guild]/settings.patch.tstests/lib/database/settings/structures/AuditLogEnvelope.test.tstests/lib/database/settings/structures/AuditLogManager.test.ts
Resolved in f7bf803. I merged in the current main-branch changes, fixed the package.json/pnpm-lock.yaml conflicts, and validated the result with lint, build, targeted settings-manager tests, and final review/security checks. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.coderabbit.yml (1)
21-23: 💤 Low valueVerify the slop detection label value.
The label
'007'is valid but unconventional for slop detection. Typically, labels such as "slop", "spam", or "low-quality" are used. Please confirm this is intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.coderabbit.yml around lines 21 - 23, The slop_detection label currently uses the unconventional value '007'; verify whether this was intentional and if not change the slop_detection.label value to a more conventional identifier (e.g., "slop", "spam", or "low-quality") or document why '007' is required; update the slop_detection block (symbol: slop_detection.label) accordingly and ensure any downstream code or rules that reference this label are adjusted to match.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/continuous-integration.yml:
- Line 90: Ensure the workflow only upgrades to codecov/codecov-action@v6 after
verifying runner environments support Node.js 24: check your GitHub Enterprise /
self-hosted runner images for Node 24 or add a conditional that uses the current
v5 (codecov/codecov-action@v5) when Node 24 is unavailable; keep all existing
input parameters passed to the action (the same outputs/with keys) to maintain
compatibility; and for security/stability consider pinning the action to a
specific commit SHA (e.g., codecov/codecov-action@<sha>) instead of the loose v6
tag.
In `@tests/lib/database/settings/structures/PermissionNodeManager.test.ts`:
- Line 154: The test mistakenly uses createGuildMember to create a GuildMember
instead of a Role, so replace the line that creates the wrong entity with
creation of a Role (use the existing helper used elsewhere, e.g., createRole or
createGuildRole, or construct a Role for tests) so the test exercises
PermissionNodeManager.reset behaviour for a Role; update the variable name if
needed and ensure the created Role is passed into the reset/assertion calls that
currently reference role.
- Around line 119-131: The test doesn't assert that reset(user) actually threw,
so add a guard: either assert caughtError is defined before casting (e.g.,
expect(caughtError).toBeDefined()) or replace the try/catch with Vitest's
expect(() => reset(user)).toThrow() pattern; when using toThrow, assert the
thrown is an instance of UserError and that its identifier equals
LanguageKeys.Commands.Management.PermissionNodesNodeNotExists and context.target
equals user. Target symbols: reset, UserError,
LanguageKeys.Commands.Management.PermissionNodesNodeNotExists.
---
Nitpick comments:
In @.coderabbit.yml:
- Around line 21-23: The slop_detection label currently uses the unconventional
value '007'; verify whether this was intentional and if not change the
slop_detection.label value to a more conventional identifier (e.g., "slop",
"spam", or "low-quality") or document why '007' is required; update the
slop_detection block (symbol: slop_detection.label) accordingly and ensure any
downstream code or rules that reference this label are adjusted to match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ab87d86b-09d2-42b2-a833-6ba8c6e2590c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
.coderabbit.yml.github/workflows/autofix.yml.github/workflows/continuous-integration.yml.github/workflows/update-tlds.yml.vscode/settings.jsoncompose.dev.yamlpackage.jsonpnpm-workspace.yamlsrc/lib/database/settings/index.tssrc/lib/moderation/workers/WorkerHandler.tstests/lib/database/settings/structures/PermissionNodeManager.test.tstests/lib/moderation/workers/WorkerHandler.test.tstests/lib/moderation/workers/WorkerManager.test.tstests/lib/util/functions/emojis/getEmojiTextFormat.test.tstsdown.config.ts
✅ Files skipped from review due to trivial changes (6)
- .github/workflows/update-tlds.yml
- compose.dev.yaml
- .github/workflows/autofix.yml
- .vscode/settings.json
- pnpm-workspace.yaml
- src/lib/database/settings/index.ts
Standalone Node ESM script that verifies the audit_event chain without depending on a TypeScript build: - Walks chain via prevHash links (timestamp-independent) - Detects HASH MISMATCH, CHAIN FORK, CYCLE, ORPHAN - Reports all rows as orphaned when no root exists - Uses raw pg pool + inline canonicalize/hashEnvelope - Reads DATABASE_URL from src/.env or environment - Exits 1 on any violation, 0 on clean chain
…wolfstar into feat/audit-log-manager
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/audit-verify.mjs (1)
93-158: Consider memory usage for large audit tables.The script loads all
audit_eventrows into memory (line 85). For guilds with extensive audit histories, this could consume significant memory. If you anticipate tables with hundreds of thousands of rows, consider streaming the verification or processing in batches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ce138f7d-6d4c-43f2-811b-6b516330d21f
📒 Files selected for processing (2)
package.jsonscripts/audit-verify.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
|
@copilot apply changes based on this thread |
Agent-Logs-Url: https://github.com/wolfstar-project/wolfstar/sessions/166db01e-8382-45f7-9a41-45933e67f964 Co-authored-by: RedStar071 <76824516+RedStar071@users.noreply.github.com>
|
@copilot resolve the merge conflicts in this pull request |
Agent-Logs-Url: https://github.com/wolfstar-project/wolfstar/sessions/166db01e-8382-45f7-9a41-45933e67f964 Co-authored-by: RedStar071 <76824516+RedStar071@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: RedStar071 <76824516+RedStar071@users.noreply.github.com>
Agent-Logs-Url: https://github.com/wolfstar-project/wolfstar/sessions/c1a391df-b150-40ff-97b9-8e4b46279263 Co-authored-by: RedStar071 <76824516+RedStar071@users.noreply.github.com>
Checked the thread against the current branch state. The
Resolved in abf5a80 and validated again in 938767a. I merged the latest Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Summary
Adds an AuditLogManager to track dashboard settings changes, adapted from wolfstar.rocks PR #144 and following the same conventions as `PermissionNodeManager` and other existing managers.
Changes
Database
Core
Routes
Tests