Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
2d2418a
feat(notifications): webhook notification system for important bot ev…
BillChirico Mar 2, 2026
f750a4e
test(config-listeners): update listener count to 12 for webhook confi…
BillChirico Mar 2, 2026
1c854c6
fix(webhooks): restrict delivery URL validation to https only
BillChirico Mar 2, 2026
1f619ec
chore: remove accidentally committed feat-issue-164 submodule
BillChirico Mar 2, 2026
5a6860e
chore: fix lint errors (biome auto-fix + exclude stray worktree dir)
BillChirico Mar 2, 2026
dc5f990
fix: correct useGuildSelection usage in AiFeedbackStats component
BillChirico Mar 2, 2026
44cd6ba
feat: add Next.js proxy routes for ai-feedback stats and recent endpo…
BillChirico Mar 2, 2026
4561f7c
chore: fix remaining lint errors (biome format + unsafe fixes)
BillChirico Mar 2, 2026
ca04d36
ci: trigger CI
BillChirico Mar 2, 2026
d207c97
chore: merge main into feat/issue-130, resolve conflict in ai-feedbac…
BillChirico Mar 2, 2026
f041302
fix: ai-feedback route/module - pool injection, camelCase mapping, er…
BillChirico Mar 2, 2026
14bcf2c
fix(webhookNotifier): use nullish coalescing to preserve status=0 for…
BillChirico Mar 2, 2026
4051e0b
fix(config): support '*' wildcard in onConfigChange for global change…
BillChirico Mar 2, 2026
1fe44d7
fix: redis test - use class mock for ioredis constructor
BillChirico Mar 2, 2026
51fd8c4
fix(security): SSRF vulnerability in webhook URL validation
Mar 2, 2026
a715c9b
fix(webhook): add idempotency key for delivery retries
Mar 2, 2026
6188984
fix: remove swallowed errors in ai-feedback.js catch blocks
Mar 2, 2026
496e6e7
fix: align notifications.js error handling with API conventions
Mar 2, 2026
1e58a02
fix: add defensive pool access in webhookNotifier.js
Mar 2, 2026
82f6775
fix: restore global.fetch in test instead of deleting
Mar 2, 2026
a73c92a
Merge origin/main into feat/issue-130
Mar 2, 2026
9b69aba
fix: properly check SSRF validation result
Mar 2, 2026
2b3939a
style: fix lint formatting
Mar 2, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion biome.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
"!node_modules",
"!coverage",
"!logs",
"!data"
"!data",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove feat-issue-164 from ignore list and remove the submodule from the repository (per previous feedback)

Prompt To Fix With AI
This is a comment left during a code review.
Path: biome.json
Line: 11

Comment:
Remove `feat-issue-164` from ignore list and remove the submodule from the repository (per previous feedback)

How can I resolve this? If you propose a fix, please make it concise.

"!feat-issue-164"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Check if the feat-issue-164 directory exists
if [ -d "feat-issue-164" ]; then
  echo "=== Directory exists ==="
  echo "Contents:"
  find feat-issue-164 -type f | head -20
  echo ""
  echo "Directory structure:"
  ls -la feat-issue-164
else
  echo "Directory does not exist"
fi

echo ""
echo "=== Searching for references to feat-issue-164 or issue-164 ==="
rg "feat-issue-164|issue-164|feat.issue.164" --max-count 20

Repository: VolvoxLLC/volvox-bot

Length of output: 188


🏁 Script executed:

#!/bin/bash

# Also check the full biome.json file to understand context
echo "=== Full biome.json content ==="
cat biome.json

Repository: VolvoxLLC/volvox-bot

Length of output: 1233


Remove the stale !feat-issue-164 exclusion from biome.json.

The feat-issue-164 directory does not exist in the repository and is not referenced elsewhere in the codebase. This exclusion is leftover from development and should be removed to keep the configuration clean.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@biome.json` at line 12, Remove the stale exclusion string "!feat-issue-164"
from the exclusions array in biome.json: locate the entry with the literal
"!feat-issue-164" and delete it so the JSON no longer references that
non-existent directory (ensure the surrounding commas remain valid and the JSON
stays well-formed).

]
},
"linter": {
Expand Down
42 changes: 42 additions & 0 deletions migrations/005_webhook_notifications.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/* eslint-disable */
'use strict';

/**
* Migration 005: Webhook notifications delivery log
*
* Creates webhook_delivery_log table to store delivery attempts
* per webhook endpoint. Endpoint configs live in the per-guild config JSON.
*/

/** @param {import('node-pg-migrate').MigrationBuilder} pgm */
exports.up = (pgm) => {
pgm.sql(`
CREATE TABLE IF NOT EXISTS webhook_delivery_log (
id SERIAL PRIMARY KEY,
guild_id TEXT NOT NULL,
endpoint_id TEXT NOT NULL,
event_type TEXT NOT NULL,
payload JSONB NOT NULL,
status TEXT NOT NULL CHECK (status IN ('success', 'failed', 'pending')),
response_code INTEGER,
response_body TEXT,
attempt INTEGER NOT NULL DEFAULT 1,
delivered_at TIMESTAMPTZ DEFAULT NOW()
);

CREATE INDEX IF NOT EXISTS idx_webhook_delivery_log_guild
ON webhook_delivery_log (guild_id, delivered_at DESC);

CREATE INDEX IF NOT EXISTS idx_webhook_delivery_log_endpoint
ON webhook_delivery_log (endpoint_id, delivered_at DESC);
`);
};

/** @param {import('node-pg-migrate').MigrationBuilder} pgm */
exports.down = (pgm) => {
pgm.sql(`
DROP INDEX IF EXISTS idx_webhook_delivery_log_endpoint;
DROP INDEX IF EXISTS idx_webhook_delivery_log_guild;
DROP TABLE IF EXISTS webhook_delivery_log;
`);
};
4 changes: 4 additions & 0 deletions src/api/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import healthRouter from './routes/health.js';
import membersRouter from './routes/members.js';
import moderationRouter from './routes/moderation.js';
import notificationsRouter from './routes/notifications.js';
import ticketsRouter from './routes/tickets.js';
import webhooksRouter from './routes/webhooks.js';

Expand Down Expand Up @@ -58,6 +59,9 @@
// GET-only; no audit middleware needed (reads are not mutating actions)
router.use('/guilds', requireAuth(), auditLogRouter);

// Notification webhook management routes — require API secret or OAuth2 JWT
router.use('/guilds', requireAuth(), auditLogMiddleware(), notificationsRouter);

// Webhook routes — require API secret or OAuth2 JWT (endpoint further restricts to api-secret)
router.use('/webhooks', requireAuth(), webhooksRouter);

Expand Down
41 changes: 36 additions & 5 deletions src/api/routes/ai-feedback.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ router.get(
async (req, res, next) => {
try {
const guildId = req.params.id;
const pool = req.app.locals.dbPool;

if (!pool) {
return res.status(503).json({ error: 'Database unavailable' });
}
Comment on lines +95 to +99
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Use route-standard custom errors (with context logging) instead of inline 503 JSON responses.

These early returns bypass the shared API error contract and skip contextual logging for operational diagnosis. Replace direct res.status(503) responses with a custom error from src/utils/errors.js and log route context before re-throwing/forwarding.

As per coding guidelines src/api/routes/*.js: "Always use custom error classes from src/utils/errors.js and log errors with context before re-throwing".

Also applies to: 197-201

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/api/routes/ai-feedback.js` around lines 95 - 99, Replace the inline 503
JSON responses in ai-feedback.js (where you check req.app.locals.dbPool and
similar checks at the other spot) with throwing a route-standard custom error
imported from src/utils/errors.js (e.g., DatabaseUnavailableError or the
appropriate ServiceUnavailable error class), and before throwing call the route
logger with context (use req.log or req.app.locals.logger) including identifiers
like route name, HTTP method and that dbPool is missing; remove the direct
res.status(503).json(...) and let the central error middleware handle the
response; apply the same change for the second occurrence around the later check
(the block currently returning 503 at lines ~197-201).


let days = 30;
if (req.query.days !== undefined) {
Expand All @@ -101,10 +106,15 @@ router.get(
}
}

const [stats, trend] = await Promise.all([
getFeedbackStats(guildId),
getFeedbackTrend(guildId, days),
]);
let stats, trend;
try {
[stats, trend] = await Promise.all([
getFeedbackStats(guildId, pool),
getFeedbackTrend(guildId, days, pool),
]);
} catch (_err) {
return res.status(500).json({ error: 'Failed to fetch AI feedback stats' });
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Do not swallow route errors in local catch blocks.

These catches return 500 directly without logging context or re-throwing a route-standard custom error, which breaks the shared API error contract.

As per coding guidelines src/api/routes/*.js: "Always use custom error classes from src/utils/errors.js and log errors with context before re-throwing".

Also applies to: 216-220

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/api/routes/ai-feedback.js` around lines 110 - 117, The catch blocks
around the parallel calls to getFeedbackStats and getFeedbackTrend (and the
similar one at lines 216-220) are swallowing errors; instead, log the error with
context and re-throw a route-standard custom error from src/utils/errors.js.
Replace the bare return res.status(500)... in the catch by calling the
request/process logger (same logger used elsewhere in this route) with a
descriptive message and the caught error, then throw the appropriate custom
error class from src/utils/errors.js (e.g., ApiError/RouteError) so the shared
API error handler can format the response; keep references to getFeedbackStats
and getFeedbackTrend and the surrounding try/catch when making this change.


res.json({
...stats,
Expand Down Expand Up @@ -188,6 +198,11 @@ router.get(
async (req, res, next) => {
try {
const guildId = req.params.id;
const pool = req.app.locals.dbPool;

if (!pool) {
return res.status(503).json({ error: 'Database unavailable' });
}

let limit = 25;
if (req.query.limit !== undefined) {
Expand All @@ -197,7 +212,23 @@ router.get(
}
}

const feedback = await getRecentFeedback(guildId, limit);
let rawFeedback;
try {
rawFeedback = await getRecentFeedback(guildId, limit, pool);
} catch (_err) {
return res.status(500).json({ error: 'Failed to fetch recent AI feedback' });
}

// Normalize DB row keys to camelCase (handles both raw SQL and aliased results)
const feedback = rawFeedback.map((row) => ({
id: row.id,
messageId: row.messageId ?? row.message_id,
channelId: row.channelId ?? row.channel_id,
userId: row.userId ?? row.user_id,
feedbackType: row.feedbackType ?? row.feedback_type,
createdAt: row.createdAt ?? row.created_at,
}));
Comment on lines +214 to +222
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider removing redundant snake_case fallbacks in response mapping.

getRecentFeedback already returns camelCase keys, so dual-shape mapping here adds maintenance overhead and can mask contract drift. Prefer enforcing a single shape at module boundary.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/api/routes/ai-feedback.js` around lines 214 - 222, The mapping in
ai-feedback.js that normalizes rawFeedback to camelCase is redundantly handling
snake_case fallbacks (message_id, channel_id, user_id, feedback_type,
created_at) even though getRecentFeedback already returns camelCase; remove the
"?? row.snake_case" fallbacks and collapse the mapping to use only camelCase
keys (id, messageId, channelId, userId, feedbackType, createdAt), and if you
need protection add a single normalization function (e.g., normalizeFeedbackRow)
at the DB boundary or update getRecentFeedback to guarantee shape rather than
handling dual shapes here.


res.json({ feedback });
} catch (err) {
next(err);
Expand Down
Loading
Loading