Conversation
- Add user_id, outcome, duration_ms columns to messages table (SQLite + MySQL schemas) - Add ALTER TABLE migrations for existing databases + 2 audit indexes - Extend ChatRepository: appendMessage with audit fields, queryAuditLogs, getMessageById - Capture audit data in SSE tool_execution_start/end: userId, outcome (success/error/blocked), durationMs - Add audit.list and audit.detail RPC methods with cursor pagination and permission control - Add Audit tab to Metrics page with filter bar, sortable list, and expandable detail panel - Add technical implementation doc (command-audit-impl.md)
Previously required clicking Search to see data. Now triggers initial query automatically when WebSocket is connected.
- queryAuditLogs LEFT JOIN users table to return userName directly - Remove userNames state and mapping logic from frontend - Display userName in audit list with userId as fallback
- Add full-width Command code block in audit detail expand panel - Add audit columns (user_id, outcome, duration_ms) to MySQL init-schema - Reorder migrate-sqlite: run migrations before index creation
- Remove redundant ALTER TABLE migrations for audit columns (already in CREATE TABLE) - Show Audit tab only for admin users via usePermissions hook
- Add userName param to queryAuditLogs with LIKE fuzzy matching - Pass userName from audit.list RPC (admin only) - Frontend sends userName instead of userId, update placeholder
nightmeng
left a comment
nightmeng
left a comment
There was a problem hiding this comment.
PR Review: feat(audit): command audit system for tool execution tracking
Overall approach is sound — leveraging the existing messages table + SSE pipeline is the right way to add audit without a new table. A few issues to address:
P0 — Must Fix
P0-1: Missing MySQL ALTER TABLE migrations for existing databases
init-schema.ts adds user_id, outcome, duration_ms to the CREATE TABLE messages DDL — this covers fresh installs. But there are no ALTER TABLE messages ADD COLUMN ... statements in the MIGRATIONS array. Existing MySQL deployments will fail when Drizzle tries to write these columns.
Same applies to SQLite — the columns are in CREATE TABLE IF NOT EXISTS (no-op for existing DBs), but no ALTER TABLE statements in migrate-sqlite.ts.
P0-2: Missing MySQL audit indexes
migrate-sqlite.ts adds idx_messages_audit and idx_messages_tool_name, but init-schema.ts INDEX_STATEMENTS does not. Production MySQL will do full table scans on audit queries.
P0-3: SQLite outcome CHECK constraint not in Drizzle schema
migrate-sqlite.ts DDL: outcome TEXT CHECK(outcome IN ('success', 'error', 'blocked'))
schema-sqlite.ts: text("outcome") — no CHECK
schema-mysql.ts: varchar("outcome", { length: 16 }) — no constraint
If the app writes an unexpected value, SQLite will reject it with an unclear error while MySQL silently accepts it. Either add the CHECK consistently everywhere, or remove it from the DDL and validate at application layer.
P0-4: Frontend hides Audit tab but backend doesn't require admin
audit.list calls requireAuth (not requireAdmin), then scopes non-admin users to their own data. But the frontend {isAdmin && (...)} hides the tab entirely. This creates a hidden endpoint non-admins can call via WebSocket. If it's meant to be admin-only, use requireAdmin. If non-admins should see their own data, show the tab.
P1 — Should Fix
P1-1: pendingToolStartTime race with parallel tool calls
The SSE pipeline uses a single pendingToolStartTime variable. If the agent fires parallel tool calls, tool_execution_start for tool B overwrites tool A's start time, giving tool A a wrong duration. Consider using a Map<toolName, startTime> or keying by tool call ID.
P1-2: LIKE meta-characters not escaped in userName search
if (opts.userName) conditions.push(like(users.username, `%${opts.userName}%`));Searching for % or _ matches unintended rows. Escape LIKE wildcards before interpolation.
P1-3: Non-admin users see zero historical results
messages.user_id is null for all pre-migration data. Non-admin queries filter WHERE messages.user_id = :userId — which returns nothing for all historical rows. Document this limitation or fall back to sessions.user_id JOIN for ownership.
P1-4: Cursor pagination index doesn't cover id
idx_messages_audit ON messages(role, user_id, timestamp) — the cursor uses OR(timestamp < cursor, AND(timestamp = cursor, id < cursorId)). The id comparison falls outside the index. Consider adding id to the index or using timestamp-only cursor (with sufficient precision).
P2 — Nits
detailCachestate grows without bound — consider an LRU limit- DetailPanel shows command twice for bash tools (parsed + raw toolInput)
audit.detailresponse omitsuserId— admin loses "who" context in detail view- No runtime validation on
outcomeparameter from client
Looks Good
- Migration reorder (ALTER TABLE before index creation) is a correct structural fix
- SSE pipeline integration is minimal and clean — follows existing
pendingToolInputpattern - Security:
audit.detailownership check via session lookup is correct redactText()applied before DB write — credential leak through audit prevented- Cursor pagination with
limit + 1/hasMoreis standard and efficient - All queries use Drizzle typed API — no SQL injection risk
nightmeng
left a comment
nightmeng
left a comment
There was a problem hiding this comment.
Review: feat(audit): command audit system for tool execution tracking
Good approach — reusing the messages table + SSE pipeline keeps this lightweight. A few issues to address:
P0
1. Missing MySQL audit indexes in init-schema.ts
migrate-sqlite.ts adds idx_messages_audit and idx_messages_tool_name, but init-schema.ts INDEX_STATEMENTS does not include them. MySQL deployments will do full table scans on audit queries.
2. SQLite outcome CHECK constraint not in Drizzle schema
migrate-sqlite.ts DDL: outcome TEXT CHECK(outcome IN ('success', 'error', 'blocked'))
schema-sqlite.ts: text("outcome") — no CHECK
schema-mysql.ts: varchar("outcome", { length: 16 }) — no constraint
If the app writes an unexpected value, SQLite rejects it with an unclear error while MySQL silently accepts. Either add the CHECK consistently or remove it from DDL and validate at the application layer.
3. Frontend hides Audit tab but backend doesn't require admin
audit.list calls requireAuth (not requireAdmin), then scopes non-admin users to their own data. But the frontend hides the tab entirely with {isAdmin && (...)}. This creates a hidden endpoint non-admins can call via WebSocket. If admin-only is the intent, use requireAdmin. If non-admins should see their own data, show the tab to everyone.
P1
4. pendingToolStartTime race with parallel tool calls
The SSE pipeline uses a single pendingToolStartTime. If the agent fires parallel tool calls, tool_execution_start for tool B overwrites tool A's start time, giving tool A a wrong duration. Consider using a Map keyed by tool call ID.
5. LIKE meta-characters not escaped in userName search
conditions.push(like(users.username, `%${opts.userName}%`));Searching for % or _ matches unintended rows. Escape LIKE wildcards before interpolation.
6. Non-admin users see zero historical results
messages.user_id is null for all pre-migration data. Non-admin queries filter WHERE messages.user_id = :userId — returns nothing for historical rows. Document this or fall back to sessions.user_id JOIN for ownership.
7. Cursor pagination index doesn't cover id
idx_messages_audit ON messages(role, user_id, timestamp) — the cursor uses OR(timestamp < cursor, AND(timestamp = cursor, id < cursorId)). The id comparison falls outside the index. Consider adding id to the index.
P2
detailCachestate grows without bound — consider an LRU limit- DetailPanel shows command twice for bash tools (parsed + raw toolInput)
audit.detailresponse omitsuserId— admin loses "who" context in detail view- No runtime validation on
outcomeparameter from client
Looks Good
- Migration reorder (ALTER TABLE before index creation) is a correct structural fix
- SSE pipeline integration is minimal and clean — follows existing
pendingToolInputpattern audit.detailownership check via session lookup is correctredactText()applied before DB write — prevents credential leak through audit logs- Cursor pagination with
limit + 1/hasMoreis standard and efficient - All queries use Drizzle typed API — no SQL injection risk in core query path
- P0-2: add MySQL audit indexes (idx_messages_audit, idx_messages_tool_name) - P0-3: remove SQLite CHECK constraint on outcome, validate at app layer - P0-4: show Audit tab to all users (non-admin sees own data only) - P1-1: fix parallel tool call timing race with Map keyed by toolName - P1-2: escape LIKE wildcards in userName search - P1-4: add id to audit index for cursor pagination coverage - P2: cap detailCache at 50 entries, remove duplicate bash command in detail - P2: add userId to audit.detail response, validate outcome param - Rename sidebar Metrics→Dashboard, tab Dashboard→Metrics
nightmeng
left a comment
nightmeng
left a comment
There was a problem hiding this comment.
All review items addressed in the latest commit:
- P0: MySQL indexes added (with
idin composite), CHECK constraint removed (app-layer validation), Audit tab visible to all users - P1: Parallel tool calls handled via
Map<toolName, startTime>, LIKE wildcards escaped, cursor index coversid - P2: detailCache LRU capped at 50, duplicate command block removed,
userIdadded to detail response, outcome validated against whitelist
One known limitation remains: non-admin users see no historical audit data (pre-migration user_id is null). New data will populate correctly. Not a blocker.
LGTM.
2 participants
Summary
Changes
user_id,outcome,duration_mscolumns to messages table (SQLite + MySQL)audit.listandaudit.detailRPC methods with cursor paginationAuditTabcomponent with filters, sortable table, detail panel with Command and Output code blocksTest plan