Feature/add admin commands#277
Conversation
…ints with logging
📝 WalkthroughWalkthroughAdds a permissioned admin command system and supporting services: telemetry, health checks, queue and cache management, user administration, admin action logging with DB migration, Discord integration (cog, permissions, views), startup wiring, documentation, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Discord User
participant Bot as Admin Cog
participant Perm as Permissions
participant Service as Admin Service
participant Queue as Queue Manager
participant DB as Supabase/DB
participant Logger as Admin Logger
User->>Bot: /admin stats
Bot->>Perm: check_permissions(interaction)
alt permitted
Perm->>Logger: log_admin_action (initiated)
Bot->>Service: BotStatsService.get_all_stats()
Service->>DB: query message stats
Service->>Queue: query queue stats
Service->>Service: compute system metrics
Service-->>Bot: BotStats
Bot->>Logger: log_admin_action (success)
Bot-->>User: reply embed with stats
else denied
Perm->>Logger: log_admin_action (denied)
Bot-->>User: ephemeral permission error
end
sequenceDiagram
participant User as Discord User
participant Bot as Admin Cog
participant View as ConfirmActionView
participant Service as UserManagementService
participant Queue as Queue Manager
participant DB as Supabase/DB
participant Logger as Admin Logger
User->>Bot: /admin user_reset user=@joe reset_memory=true
Bot->>View: present confirmation
User->>View: click Confirm
View-->>Bot: confirmed
Bot->>Logger: log_admin_action (initiated)
Bot->>Service: reset_user(reset_memory=True)
Service->>Queue: enqueue memory cleanup
Service->>DB: update user verification fields
Service-->>Bot: ResetResult
Bot->>Logger: log_admin_action (success)
Bot-->>User: result embed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_weaviate.py (1)
48-48:⚠️ Potential issue | 🔴 CriticalSyntax error: extra
"creates an unclosed string literal.
"weaviate_user_profile"")"— the second"after the closing quote of the first string opens a new string literal that is never terminated ()becomes string content). This will cause aSyntaxErrorat parse time. The same issue exists on lines 61 and 80.🐛 Proposed fix for all three occurrences
- questions = client.collections.get("weaviate_user_profile"") + questions = client.collections.get("weaviate_user_profile")- questions = get_client().collections.get("weaviate_user_profile"") + questions = get_client().collections.get("weaviate_user_profile")- questions = get_client().collections.get("weaviate_user_profile"") + questions = get_client().collections.get("weaviate_user_profile")
🤖 Fix all issues with AI agents
In @.gitignore:
- Line 111: Remove "poetry.lock" from the ignore list so the lockfile is
committed (or alternatively update the adjacent .gitignore comment to explicitly
justify ignoring it). Specifically, edit the .gitignore entry that currently
contains "poetry.lock" and either delete that line to allow committing the
lockfile (recommended for reproducible deployments) or replace/update the
comment block above to explain the deliberate decision and risks of ignoring
"poetry.lock".
In `@backend/app/services/admin/health_check_service.py`:
- Around line 88-103: The check_falkordb() function is making an HTTP GET to a
Redis port which is invalid; replace the aiohttp call with a Redis protocol
check using the project's FalkorDB client or redis.asyncio: connect with an
asyncio-aware Redis client, send a PING (or use the FalkorDB client's
health/ping method) and await the response, measure latency the same way as
start = datetime.now(), validate the ping reply (e.g., "PONG") and return
ServiceHealth("FalkorDB", "healthy", round(latency,2)) on success, return
degraded with timeout or error details on asyncio.TimeoutError or other
Exceptions (include trimmed error text as in the existing code), and ensure you
use the same timeout semantics (self.timeout) and maintain the ServiceHealth
shape.
In `@backend/app/services/admin/user_info_service.py`:
- Around line 43-52: get_user_message_count (and similarly get_last_message) is
querying a non-existent message_logs table and passing a Discord snowflake
string as user_id; instead, query the interactions table using the internal user
UUID from the users table. Change the logic in
get_user_message_count/get_last_message to either 1) lookup the user's UUID by
querying users where discord_id == discord_id and use that UUID to filter
interactions.user_id, or 2) perform a join between interactions and users (join
users.id == interactions.user_id) and filter users.discord_id == discord_id,
then count/select from interactions; continue using get_supabase_client and
return res.count or 0 but ensure you use the interactions table and the
users.discord_id -> users.id mapping.
In `@backend/app/services/admin/user_management_service.py`:
- Around line 68-82: The timestamp used in reset_verification is naive
(datetime.now())—update the call in the reset_verification method to produce an
aware UTC timestamp (e.g., use datetime.now(timezone.utc).isoformat() or
equivalent) and add the necessary import for timezone from datetime; ensure the
updated_at value sent to supabase.table("users").update(...) is timezone-aware
UTC.
In `@backend/app/utils/admin_logger.py`:
- Around line 165-181: get_admin_log_stats currently builds a
supabase.table("admin_logs").select("*") query and calls response = await
query.execute() with no limit, which can return an unbounded result set; update
get_admin_log_stats to avoid SELECT * without bounds by either adding a
reasonable default limit (e.g., .limit(n)) and exposing paging params, or
implement server-side pagination by looping with .range(offset,
offset+batchSize-1) / .limit and aggregating results incrementally, or
preferably push aggregation into a DB function/view and call that instead;
locate the query construction in get_admin_log_stats (the
supabase.table("admin_logs").select("*") block) and change it to use one of
these approaches and handle multiple pages until completion or return aggregated
results.
- Around line 58-71: The timestamp for log entries is created with
datetime.now() which yields a naive local time; update the code that builds
log_entry (in admin_logger.py, e.g., inside the log_admin_action flow where
log_entry is defined) to produce an aware UTC timestamp such as
datetime.now(timezone.utc). Also add the necessary import (timezone) from
datetime or alternatively use datetime.utcnow() and convert to ISO with a 'Z' or
timezone-aware object so stored timestamps match the DB TIMESTAMPTZ behavior and
comparisons in get_admin_logs are consistent.
- Around line 13-32: The current ensure_admin_logs_table() caches a False result
in _admin_logs_table_ready which permanently disables DB writes; change the
logic so only a successful check sets _admin_logs_table_ready = True and do not
persistently set it to False (or implement a short TTL/backoff instead) so
subsequent calls (e.g., from log_admin_action) will retry table existence
checks; update ensure_admin_logs_table() to omit assigning False on exception
(or add a timestamped cache and re-check after X seconds) and ensure callers
like log_admin_action rely on the function’s dynamic result rather than a
permanently-false flag.
In `@backend/database/02_create_admin_logs_table.sql`:
- Around line 38-42: The INSERT RLS policy "Service role can insert admin logs"
currently allows auth.role() = 'authenticated', which lets any authenticated
user write audit entries; change the policy on table admin_logs (policy name
"Service role can insert admin logs") to only permit auth.role() =
'service_role' by removing the 'authenticated' clause so the WITH CHECK uses
auth.role() = 'service_role' exclusively.
In `@backend/integrations/discord/admin_cog.py`:
- Around line 76-93: The error handler is named cog_command_error which only
catches prefix commands; rename it to cog_app_command_error so it handles
application/slash command errors; update the function definition and docstring
from async def cog_command_error(self, interaction: Interaction, error:
Exception): to async def cog_app_command_error(self, interaction: Interaction,
error: Exception): and leave the existing logic (checks for
app_commands.MissingPermissions and app_commands.CommandInvokeError, usage of
interaction.response.is_done(), await interaction.followup.send(...) and await
interaction.response.send_message(...), and logger.error calls) unchanged so
slash-command exceptions are properly caught and reported.
In `@backend/integrations/discord/bot.py`:
- Around line 40-53: The code uses self.tree.sync() which is not provided by
py-cord; replace those calls with py-cord compatible command syncing (e.g., call
await self.sync_commands() for global sync and await
self.sync_commands(guild=guild) for per-guild sync) or enable auto-sync via
auto_sync_commands=True when constructing the Bot; update the try/except blocks
around the existing sync logic (references: self.tree.sync, guild_synced
variable and the loop over self.guilds) to invoke self.sync_commands(...) so
runtime failures with py-cord are eliminated.
In `@backend/integrations/discord/permissions.py`:
- Around line 240-270: _extract_command_args is incorrectly slicing positional
args with args[2:], which double-skips parameters because the wrapper already
strips self and interaction; update the function to iterate over args (not
args[2:]) when building positional command_args so the first positional
parameter maps to param_names[0]; keep the existing param_names filtering
(removing 'self' and 'interaction') and the existing JSON-serializable
conversion logic so keyword handling remains unchanged.
In `@tests/test_embedding_service.py`:
- Around line 1-10: The test imports
app.services.embedding_service.service.EmbeddingService before the test modifies
sys.path, causing ModuleNotFoundError when run directly; to fix, move the
sys.path insertions (the ROOT and ROOT/"backend" inserts) above the line that
imports EmbeddingService (and any other app.* imports) so path setup occurs
first, or alternatively delay importing EmbeddingService by importing it inside
the test functions/class after sys.path setup; update
tests/test_embedding_service.py (and the same pattern in other tests) to ensure
sys.path modifications happen prior to importing EmbeddingService.
In `@tests/test_supabase.py`:
- Around line 1-10: The file currently imports app modules (get_supabase_client
and User/Interaction/CodeChunk/Repository) before the test adjusts sys.path,
causing ModuleNotFoundError; fix by moving the ROOT and sys.path.insert(0, ...)
setup lines so they appear before any app.* imports (ensure ROOT is defined
first), then import get_supabase_client and the model classes (User,
Interaction, CodeChunk, Repository) after the path setup so the module
resolution succeeds.
In `@tests/test_weaviate.py`:
- Around line 1-12: The test imports (WeaviateUserProfile from
app.models.database.weaviate and get_client from app.database.weaviate.client)
occur before sys.path is adjusted, causing import failures; move the ROOT and
sys.path.insert(0, ...) setup (the Path resolution and both sys.path.insert
calls that reference ROOT) to the top of tests/test_weaviate.py before any app.*
imports so that WeaviateUserProfile and get_client are imported after the path
is configured.
- Around line 1-3: Tests reference two missing model classes: WeaviateCodeChunk
and WeaviateInteraction; add them to the weaviate models module following the
existing patterns used by WeaviateUserProfile, WeaviateRepository, and
WeaviatePullRequest. Implement both classes in
backend/app/models/database/weaviate.py with the same dataclass/ORM structure
and fields consistent with how they are used in tests (ensure
serialization/typing matches existing models), then export/import them where the
other models are exported so tests can import WeaviateCodeChunk and
WeaviateInteraction without NameError.
In `@tests/tests_db.py`:
- Around line 1-10: The import order is wrong: move the sys.path setup (the ROOT
calculation and sys.path.insert calls that add ROOT and ROOT/"backend") above
any imports from app so that "from app.services.vector_db.service import
EmbeddingItem, VectorDBService" runs after the path is updated; locate these
symbols in tests/tests_db.py and reorder so the Path/sys.path insert lines
precede the app.* import, keeping the rest of the file unchanged.
🟡 Minor comments (7)
backend/app/services/admin/cache_service.py-48-59 (1)
48-59:⚠️ Potential issue | 🟡 MinorInvalid
cache_typesilently returns an empty dict.If a caller passes a typo like
"acive_threads",clear_cachereturns{}with no indication of an error. Consider validating the input and raisingValueErrorfor unrecognized types.Proposed fix
async def clear_cache(self, cache_type: str = "all") -> Dict[str, int]: + valid_types = {"all", "active_threads", "embeddings", "memories"} + if cache_type not in valid_types: + raise ValueError(f"Invalid cache_type '{cache_type}'. Must be one of {valid_types}") + cleared = {}backend/integrations/discord/views.py-76-78 (1)
76-78:⚠️ Potential issue | 🟡 Minor
on_timeoutdisables items but never edits the message, so buttons remain visually active.Disabling
self.childrenwithout callingmessage.edit(view=self)has no visible effect for the user. They'll still see clickable buttons (though callbacks won't fire since the view stopped).Proposed fix
async def on_timeout(self): for item in self.children: item.disabled = True + if self.message: + try: + await self.message.edit(view=self) + except Exception: + passNote: You'll need to ensure
self.messageis set when sending the view (e.g., by assigning the message returned fromsendtoview.message, or by usingawait interaction.followup.send(..., view=view)which may set it automatically depending on the py-cord version).backend/app/services/admin/bot_stats_service.py-37-43 (1)
37-43:⚠️ Potential issue | 🟡 MinorNaive
datetime.now()may cause incorrect message counts if Supabase stores UTC timestamps.
today_startandweek_startare computed with local time, but Supabasecreated_atcolumns typically store UTC. This can over- or under-count messages depending on the server's timezone offset.Consider using
datetime.now(timezone.utc)(fromdatetime import timezone) to ensure consistent comparisons:Proposed fix
- now = datetime.now() + now = datetime.now(timezone.utc)And update the import:
-from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezonedocs/ADMIN_COMMANDS.md-227-233 (1)
227-233:⚠️ Potential issue | 🟡 MinorFalkorDB missing from the Health Check Failures troubleshooting section.
The
/admin healthcommand checks FalkorDB (documented on line 76), but the troubleshooting section doesn't include guidance for FalkorDB failures.Proposed addition
- **Weaviate**: Check Docker container and port 8080 +- **FalkorDB**: Verify Docker container is running and port 6379 is accessible - **Gemini API**: Verify API key is validbackend/integrations/discord/admin_cog.py-206-272 (1)
206-272:⚠️ Potential issue | 🟡 MinorEarly-return at Line 224 sends a new response before
@require_admincan log the outcome.When no reset option is selected, the command replies and returns
None. Therequire_adminwrapper then logs this as a"success"action, even though nothing was actually done. Consider this a minor semantic gap in the audit trail — the logged "success" is for a no-op validation rejection.backend/app/services/admin/user_management_service.py-45-51 (1)
45-51:⚠️ Potential issue | 🟡 Minor
close_user_threadreturnsFalsewhenbot is None, causing a misleading "Failed to close thread" error inreset_user.If the bot reference isn't available, thread management isn't applicable — returning
True(or handling it distinctly) would avoid a false-negative error in the aggregated result.Proposed fix
async def close_user_thread(self, user_id: str) -> bool: try: if not self.bot: - return False + logger.info("Bot not available — skipping thread close") + return True if user_id not in self.bot.active_threads: return Truetests/test_admin_management.py-10-27 (1)
10-27:⚠️ Potential issue | 🟡 Minor
test_queue_statuspatches the method under test — it only validates the mock, not real behavior.
patch.object(service, 'get_queue_stats', ...)replaces the very method being tested, so the assertions just confirm the mock's return value. To test real logic, provide a mockedqueue_managerwith a mockedchannel(liketest_queue_cleardoes) and let the realget_queue_statsexecute.Proposed fix: test against real service logic
async def test_queue_status(): from app.services.admin.queue_service import QueueService, FullQueueStatus, QueueStats + from app.core.orchestration.queue_manager import QueuePriority + from unittest.mock import Mock, AsyncMock - service = QueueService(None) + mock_channel = Mock() + mock_queue_manager = Mock() + mock_queue_manager.channel = mock_channel + mock_queue_manager.queues = { + QueuePriority.HIGH: "high_task_queue", + QueuePriority.MEDIUM: "medium_task_queue", + QueuePriority.LOW: "low_task_queue", + } - with patch.object(service, 'get_queue_stats', new_callable=AsyncMock) as mock_stats: - mock_stats.return_value = FullQueueStatus( - high=QueueStats("high", 5, 2), - medium=QueueStats("medium", 10, 3), - low=QueueStats("low", 3, 1), - total_pending=18 - ) + mock_queue = Mock() + mock_queue.declaration_result = Mock() + mock_queue.declaration_result.message_count = 5 + mock_queue.declaration_result.consumer_count = 2 + mock_channel.declare_queue = AsyncMock(return_value=mock_queue) - status = await service.get_queue_stats() + service = QueueService(mock_queue_manager) + status = await service.get_queue_stats() - assert status.total_pending == 18 - assert status.high.pending == 5 - print("PASS: test_queue_status") + assert status.total_pending == 15 # 5 * 3 queues + assert status.high.pending == 5 + print("PASS: test_queue_status")
🧹 Nitpick comments (12)
backend/integrations/discord/bot.py (2)
38-39: Duplicate log on startup.Lines 38 and 39 both log that the bot is ready/logged in. One of them should be removed.
- logger.info(f'Enhanced Discord bot logged in as {self.user}') - logger.info(f'Bot is ready! Logged in as {self.user}') + logger.info(f'Bot is ready! Logged in as {self.user}')
46-53: Per-guild sync may hit rate limits; outer handler should preserve traceback.Two observations:
Rate limits: If the bot is in many guilds, sequential
tree.sync(guild=guild)calls with no delay can trigger Discord API rate limits. Consider adding a smallasyncio.sleepbetween iterations or only syncing to specific guilds (e.g., a configured allowlist) rather than all.Line 53: Per static analysis (TRY400), use
logger.exceptioninstead oflogger.errorto capture the full traceback for the global sync failure — this is the more critical failure path.Suggested fix for traceback logging
except Exception as e: - logger.error(f"Failed to sync slash commands: {e}") + logger.exception(f"Failed to sync slash commands: {e}")backend/app/services/admin/cache_service.py (1)
8-12:CacheInfodataclass is defined but never used.It's not referenced anywhere in this file or (based on the summary) by consumers. If it's intended for future use, consider removing it until needed to keep the module lean, or wire it into the return types of the service methods.
backend/app/services/admin/queue_service.py (1)
30-60: Duplicate queue-stats retrieval logic across services.
get_queue_statshere closely mirrors the implementation inBotStatsService.get_queue_stats(seebackend/app/services/admin/bot_stats_service.py, lines 56–73). Both iterateself.queue_manager.queues, declare queues passively, and readmessage_count. Consider extracting the shared queue-inspection logic into a helper to avoid drift between the two implementations.backend/main.py (1)
60-64: Inconsistent exception handling compared to the existing cog loading pattern.Line 57 catches
(ImportError, commands.ExtensionError)specifically, but the admin cog loading catches bareException. Consider aligning for consistency and to avoid silently swallowing unexpected errors.Proposed fix
try: await self.discord_bot.load_extension("integrations.discord.admin_cog") logger.info("Admin cog loaded successfully") - except Exception as e: + except (ImportError, commands.ExtensionError) as e: logger.error("Failed to load admin cog extension: %s", e, exc_info=True)tests/test_admin_permissions.py (2)
195-231: Consider using pytest withpytest-asyncioinstead of a custom test runner.The hand-rolled
run_all_testsorchestrator is repeated across multiple test files and won't integrate with standard CI tooling (pytestdiscovery, coverage, JUnit reporting). Adoptingpytest+pytest-asynciowould eliminate the boilerplaterun_all_testsfunction, the manual pass/fail counting, and theif __name__ == "__main__"block across all test files.This applies to
test_admin_integration.pyandtest_admin_monitoring.pyas well.
7-9: Fragilesys.pathmanipulation.Hardcoding relative
sys.path.insertassumes a fixed directory layout. If tests are run from a different working directory or the repo structure changes, imports will break. A proper package setup (e.g.,pyproject.tomlwith[tool.pytest.ini_options]pythonpath) orconftest.pywould be more robust.tests/test_admin_integration.py (1)
62-103: Permission tests duplicate those intest_admin_permissions.py.
test_permission_denies_regular_userandtest_permission_allows_adminhere replicate the same logic already covered intest_admin_permissions.py(test_admin_decorator_denies_regular_userandtest_admin_decorator_allows_administrator). Consider removing the duplicates or extracting a shared helper (like the existingcreate_mock_interactionintest_admin_permissions.py) to reduce maintenance burden.tests/test_admin_management.py (1)
6-8:sys.pathmanipulation is fragile — consider adoptingpytestwith proper package configuration.Manually inserting paths makes imports dependent on the working directory and can break when the repo layout changes. Using
pytestwithpyproject.toml/conftest.pyorPYTHONPATHin CI is more robust and discoverable. This is consistent across multiple test files in this PR, so it can be addressed holistically.backend/app/services/admin/health_check_service.py (1)
105-125: Gemini API key is passed as a URL query parameter.While functional, API keys in URL query strings can be captured by proxy logs, browser history, and CDN/WAF access logs. For a server-side health check this is lower risk, but consider using a request header instead if the API supports it.
backend/integrations/discord/permissions.py (1)
42-133:require_adminandrequire_bot_ownershare ~90% of their logic — consider extracting a shared helper.The two decorators differ only in the permission check and some metadata fields. A common
_make_permission_decorator(check_fn, ...)would eliminate the duplication and make future changes (e.g., adding a new log field) less error-prone.Also applies to: 136-227
backend/integrations/discord/admin_cog.py (1)
293-294: Redundant color assignment — embed is created withColor.blue()and immediately overwritten.Proposed fix
- embed = discord.Embed(title="Queue Status", color=discord.Color.blue()) - embed.color = color + embed = discord.Embed(title="Queue Status", color=color)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_weaviate.py (2)
50-50:⚠️ Potential issue | 🔴 CriticalSyntax error: extra closing quote.
"weaviate_user_profile""has a stray"at the end, making this a syntax error. The same issue appears on lines 63 and 82. The file won't parse at all until these are fixed.- questions = client.collections.get("weaviate_user_profile"") + questions = client.collections.get("weaviate_user_profile")Apply the same fix on lines 63 and 82.
26-44:⚠️ Potential issue | 🟠 Major
WeaviateUserProfileconstructed with non-existent fields.The fields used here (
supabase_user_id,profile_summary,primary_languages,expertise_areas,embedding) don't exist onWeaviateUserProfile, which expectsuser_id,github_username,profile_text_for_embedding, etc. This will raise a PydanticValidationErrorat runtime. Since the import path was updated to point at the new model module, this test function needs to be updated to match the actual model schema.
🧹 Nitpick comments (2)
backend/app/models/database/weaviate.py (1)
157-163: Nit:__all__is not sorted.Per the Ruff RUF022 hint, consider sorting the entries alphabetically for consistency.
Suggested fix
__all__ = [ + "WeaviateCodeChunk", + "WeaviateInteraction", + "WeaviatePullRequest", "WeaviateRepository", - "WeaviatePullRequest", - "WeaviateCodeChunk", - "WeaviateInteraction", "WeaviateUserProfile", ]tests/test_weaviate.py (1)
103-121: Pre-existing: uses deprecated.dict()instead of.model_dump().Lines 114, 37, and 188 all use
.dict(by_alias=True)which is deprecated in Pydantic v2. The project uses Pydantic v2 (ConfigDict,model_config), so these should be.model_dump(by_alias=True).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/agents/devrel/nodes/gather_context.py (1)
78-78:⚠️ Potential issue | 🟡 Minor
last_interaction_timestill uses naivedatetime.now()— inconsistent with the UTC migration.Line 78 and Line 93 both set
last_interaction_timewithdatetime.now()(naive), whilenew_message["timestamp"]on Line 30 was correctly updated todatetime.now(timezone.utc). This creates a mix of timezone-aware and naive datetimes in the same return dict, which can causeTypeErroron comparison and defeats the purpose of this PR's UTC migration.Proposed fix
return { "messages": [new_message], "context": {**state.context, **context_data}, "conversation_summary": prev_context.get("conversation_summary"), "key_topics": prev_context.get("key_topics", []), "current_task": "context_gathered", - "last_interaction_time": datetime.now() + "last_interaction_time": datetime.now(timezone.utc) }And similarly on Line 93:
result: Dict[str, Any] = { "messages": [new_message], "context": updated_context, "current_task": "context_gathered", - "last_interaction_time": datetime.now(), + "last_interaction_time": datetime.now(timezone.utc), }
🤖 Fix all issues with AI agents
In `@backend/app/services/admin/user_info_service.py`:
- Around line 92-95: The has_active_thread method can raise AttributeError when
self.bot exists but lacks active_threads; update has_active_thread to safely
check for the attribute (e.g., using getattr or hasattr) on self.bot before
membership testing, and return False when either self.bot is falsy or
active_threads is missing, referencing the has_active_thread method and the
self.bot.active_threads attribute in your fix.
In `@backend/integrations/discord/admin_cog.py`:
- Around line 360-407: The handler cache_clear currently only prompts
confirmation for cache_type == "all"; update cache_clear so any destructive
cache clear (i.e., any cache_type) requires confirmation by calling
self._confirm_action for non-empty cache_type (use cache_type to customize the
title/description, e.g., "Confirm Cache Clear - {cache_type}") before calling
self.cache_service.clear_cache; keep the existing defer/response flow (use
interaction.response.defer for the non-confirmed branch as currently done), and
ensure the followup/edit_original_response logic and exception handling remain
unchanged.
🧹 Nitpick comments (9)
backend/integrations/discord/bot.py (2)
38-39: Duplicate "ready" log messages.Line 38 and 39 both log that the bot is ready/logged in. One of these is redundant.
Suggested fix
- logger.info(f'Enhanced Discord bot logged in as {self.user}') - logger.info(f'Bot is ready! Logged in as {self.user}') + logger.info(f'Bot is ready! Logged in as {self.user}')
54-55: Uselogger.exceptionto preserve the stack trace.When the global sync fails, the traceback is lost with
logger.error. Usinglogger.exception(orlogger.error(..., exc_info=True)) will include the full traceback, which is valuable for diagnosing startup failures.Suggested fix
except Exception as e: - logger.error(f"Failed to sync slash commands: {e}") + logger.exception(f"Failed to sync slash commands: {e}")backend/app/services/admin/health_check_service.py (2)
148-163: Failed gather results lose the service name — consider mapping by index.When a check raises (instead of returning a
ServiceHealth), it's replaced withServiceHealth("Unknown", ...). Sinceasyncio.gatherpreserves input order, you can recover the service name from the index.Suggested improvement
async def get_all_health(self) -> SystemHealth: + check_names = ["Supabase", "RabbitMQ", "Weaviate", "FalkorDB", "Gemini API"] checks = await asyncio.gather( self.check_supabase(), self.check_rabbitmq(), self.check_weaviate(), self.check_falkordb(), self.check_gemini_api(), return_exceptions=True ) services = [] - for check in checks: + for i, check in enumerate(checks): if isinstance(check, Exception): - services.append(ServiceHealth("Unknown", "unhealthy", 0, str(check)[:100])) + name = check_names[i] if i < len(check_names) else "Unknown" + services.append(ServiceHealth(name, "unhealthy", 0, str(check)[:100])) else: services.append(check)
126-146: Gemini API key sent as a query parameter — standard but worth noting.Passing the key via
params={"key": ...}is the documented Google API pattern, but be aware that query-string parameters can appear in proxy/CDN/access logs. If the project later adds request-level logging or an HTTP-tracing middleware, ensure API keys are redacted.backend/app/services/admin/user_info_service.py (1)
97-127:_get_internal_user_idis called twice — once perget_user_message_countandget_last_message.
get_full_user_infoinvokes bothget_user_message_countandget_last_message, each resolving the internal user ID independently. This doubles the lookup. Consider resolving the ID once and passing it down, or adding a lightweight internal helper that accepts an optional pre-resolved ID.backend/app/services/admin/user_management_service.py (1)
26-43: Memory clear is fire-and-forget — consider noting this in the return semantics.
clear_user_memoryreturnsTrueas soon as the message is enqueued, not when the memory is actually cleared. This matches the existing pattern incogs.py(reset_thread), but callers (and theResetResult.memory_clearedfield) may mislead admins into thinking the operation completed. A docstring clarifying "queued, not confirmed" would help.backend/database/02_create_admin_logs_table.sql (1)
16-26: Redundant single-column indexes alongside composite indexes.
idx_admin_logs_executor_id(line 18) is redundant withidx_admin_logs_executor_timestamp(line 25), andidx_admin_logs_command_name(line 19) is redundant withidx_admin_logs_command_timestamp(line 26). PostgreSQL can use the leading column of a composite index for equality lookups. Dropping the single-column indexes saves storage and write overhead on an append-heavy audit table.Proposed cleanup
CREATE INDEX IF NOT EXISTS idx_admin_logs_timestamp ON admin_logs(timestamp DESC); -CREATE INDEX IF NOT EXISTS idx_admin_logs_executor_id ON admin_logs(executor_id); -CREATE INDEX IF NOT EXISTS idx_admin_logs_command_name ON admin_logs(command_name); CREATE INDEX IF NOT EXISTS idx_admin_logs_action_result ON admin_logs(action_result); CREATE INDEX IF NOT EXISTS idx_admin_logs_server_id ON admin_logs(server_id); CREATE INDEX IF NOT EXISTS idx_admin_logs_target_id ON admin_logs(target_id) WHERE target_id IS NOT NULL;backend/integrations/discord/permissions.py (1)
42-133:require_adminandrequire_bot_ownershare ~90% identical code — consider extracting a shared helper.The two decorators differ only in the permission check and a couple of metadata fields. The logging, argument extraction, command execution, and error-handling logic is duplicated verbatim. Extracting a common
_permission_decorator(check_fn, ...)would reduce maintenance burden and the risk of the two drifting out of sync.Also applies to: 136-227
backend/integrations/discord/admin_cog.py (1)
292-294: Redundant initial color on embed.Line 293 sets
color=discord.Color.blue(), then line 294 immediately overrides it with the computedcolor. Drop the initial value.Proposed fix
- embed = discord.Embed(title="Queue Status", color=discord.Color.blue()) - embed.color = color + embed = discord.Embed(title="Queue Status", color=color)
| def has_active_thread(self, discord_id: str) -> bool: | ||
| if not self.bot: | ||
| return False | ||
| return discord_id in self.bot.active_threads |
There was a problem hiding this comment.
self.bot.active_threads may raise AttributeError if the attribute doesn't exist on the bot.
If the bot instance doesn't have an active_threads attribute, this will throw instead of returning False.
Proposed fix
def has_active_thread(self, discord_id: str) -> bool:
if not self.bot:
return False
- return discord_id in self.bot.active_threads
+ active_threads = getattr(self.bot, 'active_threads', {})
+ return discord_id in active_threads🤖 Prompt for AI Agents
In `@backend/app/services/admin/user_info_service.py` around lines 92 - 95, The
has_active_thread method can raise AttributeError when self.bot exists but lacks
active_threads; update has_active_thread to safely check for the attribute
(e.g., using getattr or hasattr) on self.bot before membership testing, and
return False when either self.bot is falsy or active_threads is missing,
referencing the has_active_thread method and the self.bot.active_threads
attribute in your fix.
| @app_commands.command(name="cache_clear", description="Clear cached data") | ||
| @app_commands.describe(cache_type="What to clear") | ||
| @app_commands.choices(cache_type=[ | ||
| app_commands.Choice(name="All", value="all"), | ||
| app_commands.Choice(name="Active Threads", value="active_threads"), | ||
| app_commands.Choice(name="Embeddings", value="embeddings"), | ||
| app_commands.Choice(name="Memories", value="memories") | ||
| ]) | ||
| @require_admin | ||
| async def cache_clear(self, interaction: Interaction, cache_type: str = "all"): | ||
| """Clear various caches.""" | ||
| try: | ||
| if cache_type == "all": | ||
| confirmed = await self._confirm_action( | ||
| interaction, | ||
| title="Confirm Cache Clear", | ||
| description=( | ||
| "You are about to clear **all** caches.\n" | ||
| "This action may affect active sessions and performance." | ||
| ), | ||
| ) | ||
| if not confirmed: | ||
| return | ||
| else: | ||
| await interaction.response.defer(ephemeral=True) | ||
|
|
||
| cleared = await self.cache_service.clear_cache(cache_type=cache_type) | ||
|
|
||
| embed = discord.Embed( | ||
| title="Cache Clear", | ||
| description=f"Cleared cache type: {cache_type}", | ||
| color=discord.Color.blue(), | ||
| ) | ||
| for name, count in cleared.items(): | ||
| embed.add_field(name=name.replace("_", " ").title(), value=str(count), inline=True) | ||
|
|
||
| if cache_type == "all": | ||
| await interaction.edit_original_response(embed=embed, view=None) | ||
| else: | ||
| await interaction.followup.send(embed=embed, ephemeral=True) | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Cache clear failed: {e}", exc_info=True) | ||
| if interaction.response.is_done(): | ||
| await interaction.followup.send(f"Clear failed: {str(e)}", ephemeral=True) | ||
| else: | ||
| await interaction.response.send_message(f"Clear failed: {str(e)}", ephemeral=True) | ||
|
|
There was a problem hiding this comment.
Non-"all" cache clears skip confirmation — intentional?
The PR objectives state cache_clear is destructive and requires confirmation. Currently, only cache_type="all" triggers the confirmation dialog (line 372); individual types like active_threads or embeddings go straight through. If this is intentional, a brief code comment would clarify the rationale. If not, the confirmation should apply to all cache_type values.
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 404-404: Use explicit conversion flag
Replace with conversion flag
(RUF010)
[warning] 406-406: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In `@backend/integrations/discord/admin_cog.py` around lines 360 - 407, The
handler cache_clear currently only prompts confirmation for cache_type == "all";
update cache_clear so any destructive cache clear (i.e., any cache_type)
requires confirmation by calling self._confirm_action for non-empty cache_type
(use cache_type to customize the title/description, e.g., "Confirm Cache Clear -
{cache_type}") before calling self.cache_service.clear_cache; keep the existing
defer/response flow (use interaction.response.defer for the non-confirmed branch
as currently done), and ensure the followup/edit_original_response logic and
exception handling remain unchanged.
1 participant
Closes #202
📝 Description
This PR completes Discord admin command implementation by wiring commands to the admin service layer, replacing placeholder behavior, and improving operational safety for admin actions.
It also improves reliability by extending health coverage and validating admin logging readiness at startup.
🔧 Changes Made
/admincommands to real admin service functionsuser_resetqueue_clearcache_clearadmin_logstable and safe logging fallback✅ Checklist
Summary by CodeRabbit
New Features
Monitoring
Audit / Database
Permissions
Documentation
Env
Tests