Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughImplements a transactional outbox for audit logging: adds DB schema and migrations, audit event types and OutboxAdapter, Kysely outbox adapter/handlers, DataAdapters.transaction wiring, management-API transaction middleware with background relay, LogsDestination, and widespread success logging calls. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant M as Management API Middleware
participant DB as Database/Transaction
participant Out as Outbox Adapter
participant R as Outbox Relay
participant D as Event Destination
C->>M: Mutating request (POST/PATCH/DELETE)
alt outbox.enabled
M->>DB: begin transaction
DB->>DB: apply entity mutation
DB->>Out: outbox.create(tenant, AuditEvent)
Out->>DB: INSERT outbox_events
DB->>DB: commit
M->>C: return response
M->>R: waitUntil(drainOutbox)
R->>Out: getUnprocessed(limit)
Out-->>R: [OutboxEvent...]
loop per event
R->>D: transform(event)
R->>D: deliver(payload)
alt deliver succeeds
R->>Out: markProcessed([id])
else deliver fails
R->>Out: markRetry(id, error, nextRetryAt)
end
end
R->>Out: cleanup(olderThan)
else
M->>DB: persist mutation (no transactional outbox)
M->>C: return response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/authhero/src/routes/management-api/sessions.ts (1)
133-135:⚠️ Potential issue | 🟡 MinorUse
toISOString()instead oftoDateString()for consistent date formatting.
toDateString()returns a locale-specific string like"Thu Apr 03 2026"which lacks time precision and may cause parsing issues. Other parts of the codebase (e.g., outbox events) use ISO 8601 format.🔧 Suggested fix
const result = await ctx.env.data.sessions.update(ctx.var.tenant_id, id, { - revoked_at: new Date().toDateString(), + revoked_at: new Date().toISOString(), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/management-api/sessions.ts` around lines 133 - 135, The session revocation timestamp uses new Date().toDateString(), which is locale-specific and lacks time info; update the call in the sessions.update invocation (ctx.env.data.sessions.update with tenant id ctx.var.tenant_id and session id id) to set revoked_at = new Date().toISOString() so the stored timestamp is ISO 8601 and consistent with other outbox/event timestamps.packages/authhero/src/routes/management-api/organizations.ts (1)
331-353:⚠️ Potential issue | 🟡 MinorCapture the pre-update organization before logging this mutation.
logMessage()can only compute a diff when bothbeforeStateandafterStateare present, but Lines 347-353 pass only the post-image. That makes organization update audit events less useful than the user update path. Fetch the organization beforeupdate()and pass it asbeforeStatehere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/management-api/organizations.ts` around lines 331 - 353, Before calling ctx.env.data.organizations.update(tenant_id, id, body), fetch the current organization via ctx.env.data.organizations.get(tenant_id, id) and store it (e.g., beforeOrg); then perform the update, re-fetch the post-image (organization) as you already do, and call logMessage(...) with both beforeState: beforeOrg and afterState: organization so LogTypes.SUCCESS_API_OPERATION can compute a proper diff for the mutation.packages/authhero/src/routes/management-api/users.ts (1)
267-279:⚠️ Potential issue | 🟡 MinorThe logged response status does not match the actual handler response.
Lines 273-276 record a
204, but Line 279 returnsctx.text("OK"), which is a 200. That will make the new audit event disagree with the real HTTP response. Either return 204 here or log the actual status/body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/management-api/users.ts` around lines 267 - 279, The audit log records a 204 but the handler returns ctx.text("OK") (HTTP 200), causing a mismatch; update the handler so the actual response matches the logged response: either change the return to a 204 no-content response (e.g., set ctx.status = 204 and return an empty body) or modify the logMessage call (LogTypes.SUCCESS_API_OPERATION) to record status 200 with body "OK"; adjust the code around logMessage, userToDelete, user_id and the final ctx.text call so both the logged response and the real HTTP status/body are identical.
🧹 Nitpick comments (7)
packages/adapter-interfaces/src/adapters/Outbox.ts (1)
18-18: Avoid duplicate tenant source increate(...)to prevent tenant drift.
tenantIdandevent.tenant_idcan diverge. Keep a single authoritative tenant field in this contract.♻️ Suggested contract cleanup
export interface OutboxAdapter { /** Write an audit event to the outbox */ - create(tenantId: string, event: AuditEventInsert): Promise<void>; + create(event: AuditEventInsert): Promise<void>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/adapter-interfaces/src/adapters/Outbox.ts` at line 18, The Outbox.create contract currently accepts both tenantId and event.tenant_id which can diverge; change the API to have a single source of truth by removing the separate tenantId parameter from the create method signature (i.e., change create(tenantId: string, event: AuditEventInsert): Promise<void> to create(event: AuditEventInsert): Promise<void>), update all implementations of Outbox.create to use event.tenant_id as the authoritative tenant, and update all call sites to pass only the event (or extract tenant from event before calling) so no duplicate tenant argument remains.packages/authhero/src/routes/management-api/hooks.ts (1)
191-197: CapturebeforeStatefor hook update/delete audit events.Line 191 and Line 290 currently omit pre-mutation state, so diffs are less useful (especially for deletes). Consider fetching the existing hook before mutation and passing
beforeStatein these two logs.Proposed refactor
@@ - await ctx.env.data.hooks.update(ctx.var.tenant_id, hook_id, hook); + const existingHook = await ctx.env.data.hooks.get(ctx.var.tenant_id, hook_id); + if (!existingHook) { + throw new HTTPException(404, { message: "Hook not found" }); + } + await ctx.env.data.hooks.update(ctx.var.tenant_id, hook_id, hook); @@ await logMessage(ctx, ctx.var.tenant_id, { type: LogTypes.SUCCESS_API_OPERATION, description: "Update a Hook", targetType: "hook", targetId: hook_id, + beforeState: existingHook as Record<string, unknown>, afterState: result as Record<string, unknown>, }); @@ - const result = await ctx.env.data.hooks.remove( + const existingHook = await ctx.env.data.hooks.get(ctx.var.tenant_id, hook_id); + if (!existingHook) { + throw new HTTPException(404, { message: "Hook not found" }); + } + const result = await ctx.env.data.hooks.remove( ctx.var.tenant_id, hook_id, ); @@ await logMessage(ctx, ctx.var.tenant_id, { type: LogTypes.SUCCESS_API_OPERATION, description: "Delete a Hook", targetType: "hook", targetId: hook_id, + beforeState: existingHook as Record<string, unknown>, });Also applies to: 290-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/management-api/hooks.ts` around lines 191 - 197, The audit logs for hook update/delete (the logMessage calls that set LogTypes.SUCCESS_API_OPERATION with targetType "hook" and targetId hook_id) are missing a beforeState; fetch the existing hook record before performing the mutation (e.g., call the existing lookup utility or DB method that retrieves a hook by hook_id) and pass that object as beforeState in the log payload in addition to the current afterState/result; do the same in the delete handler (the logMessage around lines ~290) so deletes include the pre-mutation state.packages/authhero/src/routes/management-api/custom-domains.ts (1)
141-146: Consider addingbeforeStatefor custom-domain delete/update logs.At Line 141 and Line 212, the event payload lacks pre-mutation state, which weakens diff quality and post-incident reconstruction.
Proposed refactor
@@ - const result = await ctx.env.data.customDomains.remove( + const existingCustomDomain = await ctx.env.data.customDomains.get( + ctx.var.tenant_id, + id, + ); + if (!existingCustomDomain) { + throw new HTTPException(404, { message: "Custom domain not found" }); + } + const result = await ctx.env.data.customDomains.remove( ctx.var.tenant_id, id, ); @@ await logMessage(ctx, ctx.var.tenant_id, { type: LogTypes.SUCCESS_API_OPERATION, description: "Delete a Custom Domain", targetType: "custom_domain", targetId: id, + beforeState: existingCustomDomain as Record<string, unknown>, }); @@ - const result = await ctx.env.data.customDomains.update( + const existingCustomDomain = await ctx.env.data.customDomains.get( + ctx.var.tenant_id, + id, + ); + if (!existingCustomDomain) { + throw new HTTPException(404); + } + const result = await ctx.env.data.customDomains.update( ctx.var.tenant_id, id, body, ); @@ await logMessage(ctx, ctx.var.tenant_id, { type: LogTypes.SUCCESS_API_OPERATION, description: "Update a Custom Domain", targetType: "custom_domain", targetId: id, + beforeState: existingCustomDomain as Record<string, unknown>, afterState: customDomain as Record<string, unknown>, });Also applies to: 212-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/management-api/custom-domains.ts` around lines 141 - 146, Before calling logMessage in the custom-domain delete and update flows, fetch the current custom-domain record (using the existing read method for custom domains, e.g., getCustomDomainById/fetchCustomDomain or the repository/ORM used in this file) and include that object as beforeState in the log payload; update the calls to logMessage (the one using LogTypes.SUCCESS_API_OPERATION at the delete site and the similar call in the update flow) to pass beforeState: <previousRecord> alongside targetType/targetId so logs contain the pre-mutation state for diffs and reconstruction.packages/authhero/src/routes/management-api/connections.ts (1)
239-246: Same pattern as clients.ts - consider early validation.Similar to the clients PATCH handler,
connectionBeforemay beundefinedif the connection doesn't exist. The update will fail, but an early check would be more explicit and avoid unnecessary DB operations.💡 Optional improvement
const connectionBefore = await ctx.env.data.connections.get(tenantId, id); + if (!connectionBefore) { + throw new HTTPException(404, { message: "Connection not found" }); + } const result = await ctx.env.data.connections.update(tenantId, id, body); - if (!result) { - throw new HTTPException(404, { - message: "Connection not found", - }); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/management-api/connections.ts` around lines 239 - 246, The code fetches connectionBefore but proceeds to call ctx.env.data.connections.update regardless; add an explicit existence check for connectionBefore (the same pattern used in the clients PATCH handler) and throw an HTTPException(404, { message: "Connection not found" }) if connectionBefore is undefined before calling ctx.env.data.connections.update, so you avoid the unnecessary update call and make the error path explicit.apps/docs/customization/adapter-interfaces/outbox.md (1)
112-118: Consider adding error handling to the webhook example.The example
WebhookDestination.delivermethod doesn't handle fetch failures, which could mislead implementers. A production implementation should handle errors.📝 Suggested improvement
async deliver(events: unknown[]) { const response = await fetch(this.webhookUrl, { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ events }), }); if (!response.ok) { throw new Error(`Webhook delivery failed: ${response.status}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/customization/adapter-interfaces/outbox.md` around lines 112 - 118, The WebhookDestination.deliver example currently ignores fetch failures; update the deliver method (WebhookDestination.deliver) to capture the fetch response, check response.ok, and surface failures (e.g., throw an Error or log and return an error result) including the HTTP status and any relevant body text; also consider wrapping the fetch in try/catch to handle network errors from fetch(this.webhookUrl) and include the caught error details in the thrown/logged message so callers can react appropriately.packages/authhero/src/routes/management-api/clients.ts (1)
232-238: Consider handling case where client doesn't exist before update.The
clientBeforefetch on line 232 may returnundefinedif the client doesn't exist, but the update on line 233 is called anyway. Whileclients.updatewill fail/return false if the client doesn't exist, the current flow allows the update attempt before validating existence.The 404 check on lines 236-238 happens after the update attempt and relies on the re-fetch. This is acceptable since the update would be a no-op, but for consistency with other patterns (and to avoid unnecessary DB calls), consider checking existence first:
💡 Optional improvement
const clientBefore = await ctx.env.data.clients.get(tenant_id, id); + if (!clientBefore) { + throw new HTTPException(404, { message: "Client not found" }); + } await ctx.env.data.clients.update(tenant_id, id, clientUpdate); const client = await ctx.env.data.clients.get(tenant_id, id); - if (!client) { - throw new HTTPException(404, { message: "Client not found" }); - } + // client should exist since clientBefore existed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/management-api/clients.ts` around lines 232 - 238, The code fetches clientBefore using ctx.env.data.clients.get but still calls ctx.env.data.clients.update before verifying existence; change the flow in the handler so you check clientBefore and if it's falsy throw new HTTPException(404, { message: "Client not found" }) immediately (before calling ctx.env.data.clients.update), then proceed to call ctx.env.data.clients.update(tenant_id, id, clientUpdate) and re-fetch via ctx.env.data.clients.get only if needed; this avoids an unnecessary update attempt and aligns with the existing 404 handling around the client variable.packages/authhero/src/helpers/outbox-relay.ts (1)
44-44: Remove unused variablefailedIds.The variable is populated on line 78 but never read.
♻️ Proposed fix
const processedIds: string[] = []; - const failedIds: string[] = [];And remove line 78:
- failedIds.push(event.id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/helpers/outbox-relay.ts` at line 44, Remove the unused array failedIds: delete its declaration const failedIds: string[] = []; and also remove the only push to it (the code that pushes to failedIds on the later block around line 78). Update any surrounding logic so there are no references to failedIds remaining (e.g., if there is conditional logic relying on its length, replace it with the appropriate direct check or remove that conditional).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/docs/architecture/audit-events.md`:
- Around line 23-33: The fenced code block containing the DB Transaction diagram
(the ASCII box showing "DB Transaction (request-scoped)" and steps 1–3) is
missing a language tag; update that triple-backtick fence to include a language
(e.g., add `text` after the opening ```), so the diagram block is ```text ...
``` to satisfy markdownlint and keep formatting consistent.
In `@apps/docs/features/audit-logging.md`:
- Around line 18-20: Update the AuditEvent/outbox_events description to clarify
the transaction guarantee is conditional: explicitly state that the "written to
an outbox_events table within the same database transaction" guarantee only
applies when transactions are enabled, and call out the passthrough mode
(useTransactions: false) where the outbox write is best-effort and not
rollback-safe; mention useTransactions: false by name and that in that mode the
background relay may deliver events even if the entity mutation is rolled back.
- Around line 61-73: The fenced flow-diagram block containing the "Request
Handler" / "Outbox Relay" ASCII diagram is missing a language tag; update the
triple-backtick fence that surrounds the diagram (the block starting with the
"Request Handler" header) to include a language tag such as text (```text) so
markdownlint passes without altering rendering.
In `@packages/authhero/src/helpers/logging.ts`:
- Around line 143-155: The body field being assigned in the request object
(body: params.body || ctx.var.body || undefined) needs to be redacted to prevent
exposing sensitive information like passwords. Apply the existing redaction
helper that is used for before/after state to the request body before storing it
in the audit payload. This ensures that plaintext credentials from user
management routes are not persisted in outbox_events.payload and forwarded to
logs.
In `@packages/authhero/src/helpers/outbox-destinations/logs.ts`:
- Around line 67-70: The deliver method in LogsDestination is not
idempotent—because drainOutbox() can deliver the same event multiple times,
calling this.logs.create(tenantId, log) will insert duplicates; update deliver
to compute or accept an idempotency key for each event (e.g., use an existing
log.id or the outbox event id) and call a deduplicating API on the storage layer
instead of blind insert: either add an idempotencyKey field to the LogInsert
passed to this.logs.create and change logs.create to
upsert/insert-if-not-exists, or call a new this.logs.createIfNotExists /
this.logs.upsert(tenantId, log, idempotencyKey) and handle unique-constraint
conflicts (swallow duplicate errors) so repeated deliveries are no-ops. Ensure
references to deliver, this.logs.create, and drainOutbox/markProcessed are
updated accordingly.
- Around line 15-49: The mapper currently flips actor-centric fields to
target-centric ones and drops geo enrichment; change the returned object so
user_id remains actor-centric (set user_id = event.actor.id || fallback) and
populate user_name from the actor (e.g., event.actor.username or
event.actor.name) instead of leaving it empty; restore location_info enrichment
by invoking the same geo adapter used by buildAuditEvent (or the existing geo
enrichment helper) with event.request.ip and fall back to event.location if the
adapter returns nothing; keep target details separate (don’t replace user_id
when event.target.type === "user") and preserve all other fields (e.g., details,
client_id, scope) as-is.
In `@packages/authhero/src/helpers/outbox-relay.ts`:
- Around line 27-30: The implementation currently stops processing remaining
destinations due to a break in the loop that delivers events to destinations;
either remove/replace that break so failures in one destination do not block
others (catch/log the error and continue the loop) or, if all-or-nothing
behavior is intended, update the function comment to state that a failure halts
processing for the current event; locate the break inside the outbox
drain/delivery loop (the code that iterates over destinations and calls the
delivery handler) and either change it to continue after logging the error or
update the comment to reflect the stopping behavior.
- Around line 46-50: The loop in outbox-relay.ts currently skips events where
event.retry_count >= maxRetries, leaving them returned by getUnprocessed()
forever; modify the drain logic in the same loop (where events, maxRetries and
retry_count are referenced) to handle exhausted events by marking them processed
or moving them to a dead-letter state instead of continue-ing — e.g., call the
existing processing helper (or add a new helper like
markEventProcessed/markEventDeadLetter or deleteOutboxEvent) with an error state
and reason so exhausted events are removed from future getUnprocessed() results
and optionally logged for observability.
In `@packages/authhero/src/routes/management-api/index.ts`:
- Around line 198-210: The current code only triggers outbox delivery via
waitUntil(), which is a no-op outside workerd; fix by adding a non-Workers
fallback that directly invokes drainOutbox when managementAdapter.outbox exists.
After the existing waitUntil(...) call, add a branch that checks if waitUntil
returned a no-op (or detect runtime != 'workerd') and then call
drainOutbox(managementAdapter.outbox, [new
LogsDestination(managementAdapter.logs)], { maxRetries:
ctx.env.outbox?.maxRetries, retentionDays: ctx.env.outbox?.retentionDays }) in a
fire-and-forget manner (e.g. void drainOutbox(...).catch(err => /* log via
managementAdapter.logs or ctx.logger */)) so the outbox runs asynchronously on
Node/Bun/local and errors are logged.
In `@packages/authhero/src/types/AuthHeroConfig.ts`:
- Around line 119-127: OutboxConfig.captureEntityState is exposed but ignored;
update the buildAuditEvent logic in helpers/logging.ts to honor
OutboxConfig.captureEntityState by only populating target.before, target.after,
and target.diff when captureEntityState is true; locate where buildAuditEvent
(and any helper it calls for computing diffs) unconditionally sets
target.before/after/diff and wrap that behavior with a conditional that checks
the passed config (or event config) flag so setting captureEntityState: false
results in those fields being omitted or left undefined.
In `@packages/aws/src/index.ts`:
- Around line 104-110: transaction() currently just passes through adapters and
cannot roll back partial mutations, breaking atomicity for outbox-wrapped
requests; update the transaction implementation in packages/aws/src/index.ts
(function transaction) to use DynamoDB transactional APIs (TransactWriteItems)
or a compensating/prepare-commit pattern so multiple writes are executed
atomically: collect mutations from the provided fn using the same DataAdapters
interface, run them as a single TransactWrite call (or implement a two-phase
commit using a prepared outbox item plus commit step) and surface failures by
throwing to allow caller rollback logic; ensure adapters reference (adapters /
DataAdapters) can produce the item-level operations needed for
transactional/prepare steps and fall back to a clear error if true atomicity is
not available.
In
`@packages/kysely/migrate/migrations/2026-04-03T10`:00:00_create_outbox_events.ts:
- Line 19: The retry_count column is currently created as DEFAULT 0 but still
allows NULLs; update the column builder in the migration (the addColumn call for
"retry_count") to make it NOT NULL by adding .notNull() to the column options
(e.g. use col => col.notNull().defaultTo(0)) so the column is created as NOT
NULL DEFAULT 0.
---
Outside diff comments:
In `@packages/authhero/src/routes/management-api/organizations.ts`:
- Around line 331-353: Before calling
ctx.env.data.organizations.update(tenant_id, id, body), fetch the current
organization via ctx.env.data.organizations.get(tenant_id, id) and store it
(e.g., beforeOrg); then perform the update, re-fetch the post-image
(organization) as you already do, and call logMessage(...) with both
beforeState: beforeOrg and afterState: organization so
LogTypes.SUCCESS_API_OPERATION can compute a proper diff for the mutation.
In `@packages/authhero/src/routes/management-api/sessions.ts`:
- Around line 133-135: The session revocation timestamp uses new
Date().toDateString(), which is locale-specific and lacks time info; update the
call in the sessions.update invocation (ctx.env.data.sessions.update with tenant
id ctx.var.tenant_id and session id id) to set revoked_at = new
Date().toISOString() so the stored timestamp is ISO 8601 and consistent with
other outbox/event timestamps.
In `@packages/authhero/src/routes/management-api/users.ts`:
- Around line 267-279: The audit log records a 204 but the handler returns
ctx.text("OK") (HTTP 200), causing a mismatch; update the handler so the actual
response matches the logged response: either change the return to a 204
no-content response (e.g., set ctx.status = 204 and return an empty body) or
modify the logMessage call (LogTypes.SUCCESS_API_OPERATION) to record status 200
with body "OK"; adjust the code around logMessage, userToDelete, user_id and the
final ctx.text call so both the logged response and the real HTTP status/body
are identical.
---
Nitpick comments:
In `@apps/docs/customization/adapter-interfaces/outbox.md`:
- Around line 112-118: The WebhookDestination.deliver example currently ignores
fetch failures; update the deliver method (WebhookDestination.deliver) to
capture the fetch response, check response.ok, and surface failures (e.g., throw
an Error or log and return an error result) including the HTTP status and any
relevant body text; also consider wrapping the fetch in try/catch to handle
network errors from fetch(this.webhookUrl) and include the caught error details
in the thrown/logged message so callers can react appropriately.
In `@packages/adapter-interfaces/src/adapters/Outbox.ts`:
- Line 18: The Outbox.create contract currently accepts both tenantId and
event.tenant_id which can diverge; change the API to have a single source of
truth by removing the separate tenantId parameter from the create method
signature (i.e., change create(tenantId: string, event: AuditEventInsert):
Promise<void> to create(event: AuditEventInsert): Promise<void>), update all
implementations of Outbox.create to use event.tenant_id as the authoritative
tenant, and update all call sites to pass only the event (or extract tenant from
event before calling) so no duplicate tenant argument remains.
In `@packages/authhero/src/helpers/outbox-relay.ts`:
- Line 44: Remove the unused array failedIds: delete its declaration const
failedIds: string[] = []; and also remove the only push to it (the code that
pushes to failedIds on the later block around line 78). Update any surrounding
logic so there are no references to failedIds remaining (e.g., if there is
conditional logic relying on its length, replace it with the appropriate direct
check or remove that conditional).
In `@packages/authhero/src/routes/management-api/clients.ts`:
- Around line 232-238: The code fetches clientBefore using
ctx.env.data.clients.get but still calls ctx.env.data.clients.update before
verifying existence; change the flow in the handler so you check clientBefore
and if it's falsy throw new HTTPException(404, { message: "Client not found" })
immediately (before calling ctx.env.data.clients.update), then proceed to call
ctx.env.data.clients.update(tenant_id, id, clientUpdate) and re-fetch via
ctx.env.data.clients.get only if needed; this avoids an unnecessary update
attempt and aligns with the existing 404 handling around the client variable.
In `@packages/authhero/src/routes/management-api/connections.ts`:
- Around line 239-246: The code fetches connectionBefore but proceeds to call
ctx.env.data.connections.update regardless; add an explicit existence check for
connectionBefore (the same pattern used in the clients PATCH handler) and throw
an HTTPException(404, { message: "Connection not found" }) if connectionBefore
is undefined before calling ctx.env.data.connections.update, so you avoid the
unnecessary update call and make the error path explicit.
In `@packages/authhero/src/routes/management-api/custom-domains.ts`:
- Around line 141-146: Before calling logMessage in the custom-domain delete and
update flows, fetch the current custom-domain record (using the existing read
method for custom domains, e.g., getCustomDomainById/fetchCustomDomain or the
repository/ORM used in this file) and include that object as beforeState in the
log payload; update the calls to logMessage (the one using
LogTypes.SUCCESS_API_OPERATION at the delete site and the similar call in the
update flow) to pass beforeState: <previousRecord> alongside targetType/targetId
so logs contain the pre-mutation state for diffs and reconstruction.
In `@packages/authhero/src/routes/management-api/hooks.ts`:
- Around line 191-197: The audit logs for hook update/delete (the logMessage
calls that set LogTypes.SUCCESS_API_OPERATION with targetType "hook" and
targetId hook_id) are missing a beforeState; fetch the existing hook record
before performing the mutation (e.g., call the existing lookup utility or DB
method that retrieves a hook by hook_id) and pass that object as beforeState in
the log payload in addition to the current afterState/result; do the same in the
delete handler (the logMessage around lines ~290) so deletes include the
pre-mutation state.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 385196ea-68b9-4a19-ab43-95ea2d4dcaa9
📒 Files selected for processing (61)
.changeset/famous-clouds-fold.mdapps/docs/.vitepress/config.tsapps/docs/architecture/audit-events.mdapps/docs/customization/adapter-interfaces/outbox.mdapps/docs/features/audit-logging.mdpackages/adapter-interfaces/src/adapters/Outbox.tspackages/adapter-interfaces/src/adapters/index.tspackages/adapter-interfaces/src/types/AuditEvent.tspackages/adapter-interfaces/src/types/index.tspackages/authhero/src/authentication-flows/refresh-token.tspackages/authhero/src/authentication-flows/silent.tspackages/authhero/src/helpers/cache-wrapper.tspackages/authhero/src/helpers/logging.tspackages/authhero/src/helpers/outbox-destinations/index.tspackages/authhero/src/helpers/outbox-destinations/logs.tspackages/authhero/src/helpers/outbox-relay.tspackages/authhero/src/helpers/server-timing.tspackages/authhero/src/index.tspackages/authhero/src/middlewares/tenant.tspackages/authhero/src/routes/auth-api/logout.tspackages/authhero/src/routes/auth-api/well-known.tspackages/authhero/src/routes/management-api/authentication-methods.tspackages/authhero/src/routes/management-api/branding.tspackages/authhero/src/routes/management-api/client-grants.tspackages/authhero/src/routes/management-api/clients.tspackages/authhero/src/routes/management-api/connections.tspackages/authhero/src/routes/management-api/custom-domains.tspackages/authhero/src/routes/management-api/emails.tspackages/authhero/src/routes/management-api/flows.tspackages/authhero/src/routes/management-api/forms.tspackages/authhero/src/routes/management-api/guardian.tspackages/authhero/src/routes/management-api/hooks.tspackages/authhero/src/routes/management-api/index.tspackages/authhero/src/routes/management-api/organizations.tspackages/authhero/src/routes/management-api/prompts.tspackages/authhero/src/routes/management-api/refresh_tokens.tspackages/authhero/src/routes/management-api/resource-servers.tspackages/authhero/src/routes/management-api/roles.tspackages/authhero/src/routes/management-api/sessions.tspackages/authhero/src/routes/management-api/tenants.tspackages/authhero/src/routes/management-api/themes.tspackages/authhero/src/routes/management-api/users.tspackages/authhero/src/routes/universal-login/screens/mfa-login-options.tspackages/authhero/src/routes/universal-login/screens/passkey-enrollment-nudge.tspackages/authhero/src/routes/universal-login/screens/passkey-enrollment.tspackages/authhero/src/types/AuthHeroConfig.tspackages/authhero/src/types/Bindings.tspackages/authhero/test/routes/auth-api/token.spec.tspackages/authhero/test/routes/auth-api/well-known.spec.tspackages/aws/src/index.tspackages/kysely/migrate/migrations/2026-04-03T10:00:00_create_outbox_events.tspackages/kysely/migrate/migrations/index.tspackages/kysely/src/db.tspackages/kysely/src/index.tspackages/kysely/src/outbox/cleanup.tspackages/kysely/src/outbox/create.tspackages/kysely/src/outbox/getUnprocessed.tspackages/kysely/src/outbox/index.tspackages/kysely/src/outbox/markProcessed.tspackages/kysely/src/outbox/markRetry.tspackages/multi-tenancy/test/settings-inheritance.test.ts
packages/kysely/migrate/migrations/2026-04-03T10:00:00_create_outbox_events.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
packages/aws/src/index.ts (1)
113-124:⚠️ Potential issue | 🟠 Major
transaction()still violates transactional semantics promised by the interface.Line 123 executes the callback on the same live adapters with no rollback boundary. Even with the outbox guard, callers can still observe partial writes if the callback fails after earlier mutations. This conflicts with the
DataAdapters.transactioncontract (transaction-scoped adapters + rollback on throw).A safer stopgap is to fail fast unconditionally until true transactional behavior exists for this adapter.
Suggested stopgap change
async transaction<T>( fn: (trxAdapters: DataAdapters) => Promise<T>, ): Promise<T> { - if (adapters.outbox) { - throw new Error( - "DynamoDB adapter does not support atomic transactions. " + - "Cannot guarantee atomicity between entity writes and outbox " + - "events. Use a SQL-backed adapter for outbox-enabled tenants.", - ); - } - return fn(adapters); + throw new Error( + "DynamoDB adapter does not implement DataAdapters.transaction " + + "semantics (transaction-scoped writes + rollback on failure).", + ); },Use this script to verify contract/implementation mismatch and enumerate call sites relying on
transaction(...):#!/bin/bash set -euo pipefail echo "== DataAdapters.transaction contract ==" fd -i 'index.ts' packages/adapter-interfaces/src/adapters --exec sed -n '80,120p' {} echo echo "== AWS adapter transaction implementation ==" fd -i 'index.ts' packages/aws/src --exec sed -n '104,132p' {} echo echo "== transaction(...) call sites ==" rg -nP --type=ts -C3 '\.transaction\s*(<[^>]+>)?\s*\('Expected result: interface docs indicate rollback semantics, while AWS implementation is passthrough; call sites then show where this semantic gap can affect behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/aws/src/index.ts` around lines 113 - 124, The transaction method in the AWS adapter (async transaction<T>(fn: (trxAdapters: DataAdapters) => Promise<T>)) violates the DataAdapters.transaction contract by executing the callback on live adapters with no rollback; instead make this a fail-fast operation: remove the current conditional passthrough and always throw a clear error indicating that the DynamoDB adapter does not support transactional semantics (referencing the transaction function and the adapters variable/outbox guard), preventing any caller from relying on rollback until a real transactional implementation is provided.packages/authhero/src/helpers/outbox-destinations/logs.ts (1)
59-69:⚠️ Potential issue | 🟠 MajorMake
LogsDestinationidempotent.
drainOutbox()is at-least-once, and this transformed payload drops the outbox id beforedeliver()does a blindlogs.create(). A retry, failedmarkProcessed(), or overlapping drainer will create another log row for the same event. Please carry the event id throughtransform()and write with an insert-if-not-exists/upsert path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/helpers/outbox-destinations/logs.ts` around lines 59 - 69, The transform/deliver flow in LogsDestination drops the outbox event id so retries can create duplicate log rows; modify transform(event: AuditEvent) to include the original event id (e.g., event.id or event.outbox_id) in the returned object and change deliver(events) to call a new idempotent persistence path on this.logs (use an insert-if-not-exists/upsert) instead of the blind this.logs.create(tenantId, log); update toLogInsert if needed to carry the id through and ensure the persistence method (e.g., logs.createOrIgnore / logs.upsertByEventId) uses the event id as a unique key to prevent duplicate inserts when drainOutbox(), markProcessed(), or overlapping drainers retry.packages/authhero/src/helpers/logging.ts (2)
213-230:⚠️ Potential issue | 🟠 MajorPreserve the
detailsoverride in outbox mode.This early return skips the legacy
LogInsertpath, butbuildAuditEvent()has no way to carryparams.details.packages/authhero/src/helpers/outbox-destinations/logs.tslater reconstructs the default{ request, response }payload, so enabling outbox silently discards caller-supplied compact details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/helpers/logging.ts` around lines 213 - 230, The outbox early-return path discards caller-supplied params.details because buildAuditEvent doesn't carry it; before calling ctx.env.data.outbox.create(tenantId, event) in the branch that checks ctx.env.outbox?.enabled && ctx.env.data.outbox, merge/preserve params.details into the event (e.g., set event.details = params.details ?? event.details or otherwise copy params.details into the event payload) so the outbox entry retains caller-provided compact details (refer to buildAuditEvent and the outbox branch where ctx.env.data.outbox.create is invoked; ensure this mirrors what packages/authhero/src/helpers/outbox-destinations/logs.ts expects).
148-169:⚠️ Potential issue | 🔴 CriticalRedact request and response bodies before enqueueing the audit event.
params.body ?? ctx.var.bodyandparams.response.bodyare still stored verbatim here. Management API payloads can contain secrets likepasswordandclient_secret, so outbox mode will persist them inoutbox_events.payloadand forward them to destinations unless they go through the same redaction path asbeforeState/afterState.🔐 Minimal fix
+function redactPayload(value: unknown): unknown { + return value && typeof value === "object" + ? redactSensitiveFields(value as Record<string, unknown>) + : value; +} ... - body: params.body || ctx.var.body || undefined, + body: redactPayload(params.body ?? ctx.var.body), ... - body: params.response.body, + body: redactPayload(params.response.body),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/helpers/logging.ts` around lines 148 - 169, The request and response bodies (params.body ?? ctx.var.body and params.response.body) are being stored verbatim; before constructing the request and response objects for the audit event in helpers/logging.ts, pass those payloads through the same redaction used for beforeState/afterState (or implement a small redactPayload utility that strips secrets like password/client_secret) and assign the redacted result to request.body and response.body so outbox_events.payload never contains raw secrets.packages/authhero/src/helpers/outbox-relay.ts (1)
27-30:⚠️ Potential issue | 🟡 MinorUpdate the relay contract comment.
Line 75 breaks on the first destination failure, so later destinations are skipped for that attempt and retried later. The header comment on Lines 27-30 says the opposite.
📝 Suggested wording
- * Each destination is processed independently — a failure in one does not block others. + * Delivery is all-or-nothing per event: if one destination fails, remaining + * destinations are skipped for that attempt and the event is retried.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/helpers/outbox-relay.ts` around lines 27 - 30, Update the header comment in outbox-relay.ts that currently claims "Each destination is processed independently" to accurately state that the relay stops processing remaining destinations when a destination fails (the destination processing loop breaks on failure), so later destinations are skipped for that run and will be retried later; refer to the outbox-relay header comment and the destination processing loop where failures cause an early exit and adjust the wording accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/authhero/src/helpers/logging.ts`:
- Around line 217-227: The outbox path is awaiting
ctx.env.data.geo.getGeoInfo(headers) which blocks audited writes; move geo
enrichment out of the outbox/enqueue branch so event.location is not set
synchronously during the write. Specifically, remove the await/getGeoInfo call
from the outbox code path that sets event.location (the block referencing
ctx.env.data.geo and getGeoInfo) and instead perform location enrichment in the
relay/destination code or as an async best-effort post-enqueue task that updates
the enqueued event later; ensure the enqueue flow only stores the event without
awaiting geo.getGeoInfo.
In `@packages/authhero/src/helpers/outbox-relay.ts`:
- Around line 40-61: The code currently fetches events with
outbox.getUnprocessed() and then delivers them, which allows race conditions;
add a claim/lease step immediately after fetching so only claimed rows are
processed. Implement and call a new adapter method (e.g.,
outbox.claimEvents(ids: string[], workerId: string, leaseMs: number)) right
after outbox.getUnprocessed(), filter the events to only those successfully
claimed (skip others), and only call destination.transform/deliver for claimed
events; ensure you record processedIds/failedIds based on the claimed subset and
respect existing retry/mark-as-processed logic. Use unique identifiers from this
file (outbox.getUnprocessed, the local events loop, processedIds/failedIds) to
locate where to add the claim and filtering logic.
---
Duplicate comments:
In `@packages/authhero/src/helpers/logging.ts`:
- Around line 213-230: The outbox early-return path discards caller-supplied
params.details because buildAuditEvent doesn't carry it; before calling
ctx.env.data.outbox.create(tenantId, event) in the branch that checks
ctx.env.outbox?.enabled && ctx.env.data.outbox, merge/preserve params.details
into the event (e.g., set event.details = params.details ?? event.details or
otherwise copy params.details into the event payload) so the outbox entry
retains caller-provided compact details (refer to buildAuditEvent and the outbox
branch where ctx.env.data.outbox.create is invoked; ensure this mirrors what
packages/authhero/src/helpers/outbox-destinations/logs.ts expects).
- Around line 148-169: The request and response bodies (params.body ??
ctx.var.body and params.response.body) are being stored verbatim; before
constructing the request and response objects for the audit event in
helpers/logging.ts, pass those payloads through the same redaction used for
beforeState/afterState (or implement a small redactPayload utility that strips
secrets like password/client_secret) and assign the redacted result to
request.body and response.body so outbox_events.payload never contains raw
secrets.
In `@packages/authhero/src/helpers/outbox-destinations/logs.ts`:
- Around line 59-69: The transform/deliver flow in LogsDestination drops the
outbox event id so retries can create duplicate log rows; modify
transform(event: AuditEvent) to include the original event id (e.g., event.id or
event.outbox_id) in the returned object and change deliver(events) to call a new
idempotent persistence path on this.logs (use an insert-if-not-exists/upsert)
instead of the blind this.logs.create(tenantId, log); update toLogInsert if
needed to carry the id through and ensure the persistence method (e.g.,
logs.createOrIgnore / logs.upsertByEventId) uses the event id as a unique key to
prevent duplicate inserts when drainOutbox(), markProcessed(), or overlapping
drainers retry.
In `@packages/authhero/src/helpers/outbox-relay.ts`:
- Around line 27-30: Update the header comment in outbox-relay.ts that currently
claims "Each destination is processed independently" to accurately state that
the relay stops processing remaining destinations when a destination fails (the
destination processing loop breaks on failure), so later destinations are
skipped for that run and will be retried later; refer to the outbox-relay header
comment and the destination processing loop where failures cause an early exit
and adjust the wording accordingly.
In `@packages/aws/src/index.ts`:
- Around line 113-124: The transaction method in the AWS adapter (async
transaction<T>(fn: (trxAdapters: DataAdapters) => Promise<T>)) violates the
DataAdapters.transaction contract by executing the callback on live adapters
with no rollback; instead make this a fail-fast operation: remove the current
conditional passthrough and always throw a clear error indicating that the
DynamoDB adapter does not support transactional semantics (referencing the
transaction function and the adapters variable/outbox guard), preventing any
caller from relying on rollback until a real transactional implementation is
provided.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdbe7d26-e599-44e6-83cb-24c6d35522d3
📒 Files selected for processing (7)
apps/docs/architecture/audit-events.mdapps/docs/features/audit-logging.mdpackages/authhero/src/helpers/logging.tspackages/authhero/src/helpers/outbox-destinations/logs.tspackages/authhero/src/helpers/outbox-relay.tspackages/aws/src/index.tspackages/kysely/migrate/migrations/2026-04-03T10:00:00_create_outbox_events.ts
✅ Files skipped from review due to trivial changes (2)
- apps/docs/architecture/audit-events.md
- apps/docs/features/audit-logging.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/kysely/migrate/migrations/2026-04-03T10:00:00_create_outbox_events.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/authhero/src/helpers/outbox-relay.ts (1)
28-31:⚠️ Potential issue | 🟡 MinorDocumentation is inconsistent with implementation.
The comment states "a failure in one does not block others," but line 85 breaks out of the destination loop when one fails. Either update the comment to reflect the actual behavior, or change the implementation to continue processing other destinations on failure.
📝 Suggested comment fix (if break is intentional)
/** * Drain unprocessed events from the outbox and deliver to all destinations. - * Each destination is processed independently — a failure in one does not block others. + * If delivery to any destination fails, remaining destinations are skipped for that event + * and the event is marked for retry. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/helpers/outbox-relay.ts` around lines 28 - 31, The file's header comment claims "a failure in one does not block others" but the destination-processing loop in outbox-relay.ts contains a break on error; either remove the break so each destination is try/caught and the loop continues (log the error and continue) or update the header comment to state that the loop stops on the first destination failure; locate the destination loop (the code that iterates over destinations and currently executes a break on error) and implement the chosen fix consistently (either change break -> continue with proper error handling/logging, or change the comment text to reflect that failures abort the remaining destinations).
🧹 Nitpick comments (5)
packages/authhero/src/helpers/logging.ts (1)
23-36: Shallow redaction may miss nested sensitive fields.
redactSensitiveFieldsonly redacts top-level keys. Ifbodycontains nested objects (e.g.,{ user: { password: "..." } }), the sensitive data will be logged. Consider recursive redaction if nested sensitive fields are possible in your API payloads.♻️ Optional recursive redaction
function redactSensitiveFields( obj: Record<string, unknown> | undefined, ): Record<string, unknown> | undefined { if (!obj) return undefined; const result: Record<string, unknown> = {}; for (const [key, value] of Object.entries(obj)) { if (SENSITIVE_FIELDS.has(key)) { result[key] = "[REDACTED]"; + } else if (value && typeof value === "object" && !Array.isArray(value)) { + result[key] = redactSensitiveFields(value as Record<string, unknown>); } else { result[key] = value; } } return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/helpers/logging.ts` around lines 23 - 36, redactSensitiveFields currently only inspects top-level keys so nested sensitive values (e.g., { user: { password: "..." } }) won't be redacted; update redactSensitiveFields to walk the input recursively (handle objects and arrays), checking each key against SENSITIVE_FIELDS and replacing matching values with "[REDACTED]" while copying non-sensitive primitives, preserving structure, and guarding against prototype pollution/cycles (use a WeakSet to track visited objects). Ensure the updated implementation is used wherever redactSensitiveFields is called so nested secrets are redacted consistently.packages/authhero/src/helpers/outbox-relay.ts (1)
91-93:failedIdsis collected but never used.The array is populated on delivery failure but never referenced afterward. Either remove it or use it for logging/metrics.
♻️ Option 1: Remove unused variable
const processedIds: string[] = []; - const failedIds: string[] = []; for (const event of claimedEvents) { // ... if (allSucceeded) { processedIds.push(event.id); - } else { - failedIds.push(event.id); } }♻️ Option 2: Add logging for observability
// Mark all successful events as processed in one batch if (processedIds.length > 0) { try { await outbox.markProcessed(processedIds); } catch { // Best effort } } + + if (failedIds.length > 0) { + console.warn(`Outbox drain: ${failedIds.length} events failed delivery`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/helpers/outbox-relay.ts` around lines 91 - 93, The array failedIds is being populated in the else branch (pushing event.id) but never used—either remove the failedIds variable and the push to it to avoid dead code, or emit observability when failures occur: capture failedIds after the delivery loop and log or record metrics (e.g., via processLogger.warn or a metrics.increment call) with its contents and context; locate the failedIds usage and the else { failedIds.push(event.id); } block in outbox-relay.ts (the delivery/relay function) and apply one of these two fixes consistently.packages/kysely/migrate/migrations/2026-04-03T11:00:00_add_outbox_claim_columns.ts (1)
7-17: Consider adding an index onclaim_expires_at.The
getUnprocessedquery filters onclaim_expires_at <= nowto find events with expired leases. Without an index, this becomes a full table scan as the outbox grows. Consider adding a composite or single-column index.♻️ Proposed index addition
await db.schema .alterTable("outbox_events") .addColumn("claim_expires_at", "varchar(35)") .execute(); + + await db.schema + .createIndex("idx_outbox_claim_expires") + .on("outbox_events") + .column("claim_expires_at") + .execute(); }And update the down migration:
export async function down(db: Kysely<Database>): Promise<void> { + await db.schema.dropIndex("idx_outbox_claim_expires").execute(); + await db.schema .alterTable("outbox_events") .dropColumn("claimed_by")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kysely/migrate/migrations/2026-04-03T11`:00:00_add_outbox_claim_columns.ts around lines 7 - 17, Add a database index on the claim_expires_at column to avoid full table scans: in the migration's up function (exported function up) alter the outbox_events table to create an index on "claim_expires_at" (e.g., idx_outbox_events_claim_expires_at or a composite index including claimed_by if you prefer) and in the corresponding down function drop that index so rollbacks remove it; reference the outbox_events table, the claim_expires_at column, and ensure the index name used in up is the same name dropped in down.packages/kysely/src/outbox/getUnprocessed.ts (1)
31-31: Add error handling forJSON.parse.If a row contains malformed JSON in
payload, this will throw and abort processing the entire batch. Consider wrapping in try-catch to log and skip corrupted rows.♻️ Proposed defensive handling
return rows.map((row) => { - const payload = JSON.parse(row.payload) as AuditEvent; + let payload: AuditEvent; + try { + payload = JSON.parse(row.payload) as AuditEvent; + } catch (e) { + console.error(`Malformed outbox payload for event ${row.id}:`, e); + return null; + } return { + id: row.id, ...payload, created_at: row.created_at, processed_at: row.processed_at, retry_count: row.retry_count, next_retry_at: row.next_retry_at, error: row.error, }; - }); + }).filter((e): e is OutboxEvent => e !== null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kysely/src/outbox/getUnprocessed.ts` at line 31, The JSON.parse call in getUnprocessed (const payload = JSON.parse(row.payload) as AuditEvent) can throw on malformed payloads and abort the batch; wrap the parse in a try-catch inside the loop that processes rows (or inside the function that maps DB rows to AuditEvent) so that if JSON.parse throws you log the row id/metadata and error via the existing logger and skip that row (do not rethrow), allowing the rest of the batch to be processed; ensure you still return only successfully parsed AuditEvent items from getUnprocessed.packages/authhero/src/routes/management-api/organizations.ts (1)
590-595: Consider capturing member details in audit log.The logging captures
targetId: organizationIdbut doesn't include which members were added. Consider adding the member IDs toafterStateor as part of the description for better audit forensics.♻️ Optional enhancement to capture member details
await logMessage(ctx, ctx.var.tenant_id, { type: LogTypes.SUCCESS_API_OPERATION, description: "Add Members to an Organization", targetType: "organization_member", targetId: organizationId, + afterState: { members }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/management-api/organizations.ts` around lines 590 - 595, The audit log currently uses logMessage with LogTypes.SUCCESS_API_OPERATION and targetId: organizationId but omits which members were added; update the log call to include the added member IDs (e.g., as afterState or appended to description) so audits show who was added. Locate the logMessage invocation in organizations.ts and include the collection of member IDs (from the request payload variable that holds new members) in either an afterState field or extend description to list member IDs while keeping targetType "organization_member" and targetId organizationId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/kysely/src/outbox/getUnprocessed.ts`:
- Around line 30-40: The mapped OutboxEvent returned from getUnprocessed (inside
the rows.map in getUnprocessed.ts) is missing the database record id, causing
drainOutbox (outbox-relay.ts) to pass the wrong identifier into claimEvents();
update the mapping to include the DB id by adding row.id to the returned object
(keep existing payload fields and
created_at/processed_at/retry_count/next_retry_at/error) so downstream code can
claim and update the correct outbox record.
---
Duplicate comments:
In `@packages/authhero/src/helpers/outbox-relay.ts`:
- Around line 28-31: The file's header comment claims "a failure in one does not
block others" but the destination-processing loop in outbox-relay.ts contains a
break on error; either remove the break so each destination is try/caught and
the loop continues (log the error and continue) or update the header comment to
state that the loop stops on the first destination failure; locate the
destination loop (the code that iterates over destinations and currently
executes a break on error) and implement the chosen fix consistently (either
change break -> continue with proper error handling/logging, or change the
comment text to reflect that failures abort the remaining destinations).
---
Nitpick comments:
In `@packages/authhero/src/helpers/logging.ts`:
- Around line 23-36: redactSensitiveFields currently only inspects top-level
keys so nested sensitive values (e.g., { user: { password: "..." } }) won't be
redacted; update redactSensitiveFields to walk the input recursively (handle
objects and arrays), checking each key against SENSITIVE_FIELDS and replacing
matching values with "[REDACTED]" while copying non-sensitive primitives,
preserving structure, and guarding against prototype pollution/cycles (use a
WeakSet to track visited objects). Ensure the updated implementation is used
wherever redactSensitiveFields is called so nested secrets are redacted
consistently.
In `@packages/authhero/src/helpers/outbox-relay.ts`:
- Around line 91-93: The array failedIds is being populated in the else branch
(pushing event.id) but never used—either remove the failedIds variable and the
push to it to avoid dead code, or emit observability when failures occur:
capture failedIds after the delivery loop and log or record metrics (e.g., via
processLogger.warn or a metrics.increment call) with its contents and context;
locate the failedIds usage and the else { failedIds.push(event.id); } block in
outbox-relay.ts (the delivery/relay function) and apply one of these two fixes
consistently.
In `@packages/authhero/src/routes/management-api/organizations.ts`:
- Around line 590-595: The audit log currently uses logMessage with
LogTypes.SUCCESS_API_OPERATION and targetId: organizationId but omits which
members were added; update the log call to include the added member IDs (e.g.,
as afterState or appended to description) so audits show who was added. Locate
the logMessage invocation in organizations.ts and include the collection of
member IDs (from the request payload variable that holds new members) in either
an afterState field or extend description to list member IDs while keeping
targetType "organization_member" and targetId organizationId.
In
`@packages/kysely/migrate/migrations/2026-04-03T11`:00:00_add_outbox_claim_columns.ts:
- Around line 7-17: Add a database index on the claim_expires_at column to avoid
full table scans: in the migration's up function (exported function up) alter
the outbox_events table to create an index on "claim_expires_at" (e.g.,
idx_outbox_events_claim_expires_at or a composite index including claimed_by if
you prefer) and in the corresponding down function drop that index so rollbacks
remove it; reference the outbox_events table, the claim_expires_at column, and
ensure the index name used in up is the same name dropped in down.
In `@packages/kysely/src/outbox/getUnprocessed.ts`:
- Line 31: The JSON.parse call in getUnprocessed (const payload =
JSON.parse(row.payload) as AuditEvent) can throw on malformed payloads and abort
the batch; wrap the parse in a try-catch inside the loop that processes rows (or
inside the function that maps DB rows to AuditEvent) so that if JSON.parse
throws you log the row id/metadata and error via the existing logger and skip
that row (do not rethrow), allowing the rest of the batch to be processed;
ensure you still return only successfully parsed AuditEvent items from
getUnprocessed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f351108d-5411-42a2-b75d-0b0b0e40b708
📒 Files selected for processing (15)
apps/docs/customization/adapter-interfaces/outbox.mdpackages/adapter-interfaces/src/adapters/Outbox.tspackages/authhero/src/helpers/logging.tspackages/authhero/src/helpers/outbox-relay.tspackages/authhero/src/helpers/wait-until.tspackages/authhero/src/routes/management-api/organizations.tspackages/authhero/src/routes/management-api/sessions.tspackages/authhero/src/routes/management-api/users.tspackages/authhero/test/hooks/user-deletion-logging.spec.tspackages/authhero/test/hooks/user-deletion.test.tspackages/kysely/migrate/migrations/2026-04-03T11:00:00_add_outbox_claim_columns.tspackages/kysely/src/db.tspackages/kysely/src/outbox/claimEvents.tspackages/kysely/src/outbox/getUnprocessed.tspackages/kysely/src/outbox/index.ts
✅ Files skipped from review due to trivial changes (1)
- apps/docs/customization/adapter-interfaces/outbox.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/kysely/src/outbox/index.ts
- packages/authhero/src/routes/management-api/sessions.ts
- packages/kysely/src/db.ts
- packages/adapter-interfaces/src/adapters/Outbox.ts
- packages/authhero/src/routes/management-api/users.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added an outbox pattern for audit events
Summary by CodeRabbit
New Features
Documentation
Other