DRAFT: v0.1.0 #1020
Open
DRAFT: v0.1.0 #1020
Conversation
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
Deploying tux with
|
| Latest commit: |
b132c80
|
| Status: | 🚫 Build failed. |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found. |
|
@sourcery-ai review |
Reviewer's GuideThis PR modernizes the Tux codebase by reorganizing source files under a Entity relationship diagram for database controller module reorganizationerDiagram
AfkController ||--o{ DatabaseController : provides
CaseController ||--o{ DatabaseController : provides
GuildController ||--o{ DatabaseController : provides
GuildConfigController ||--o{ DatabaseController : provides
LevelsController ||--o{ DatabaseController : provides
NoteController ||--o{ DatabaseController : provides
ReminderController ||--o{ DatabaseController : provides
SnippetController ||--o{ DatabaseController : provides
StarboardController ||--o{ DatabaseController : provides
StarboardMessageController ||--o{ DatabaseController : provides
DatabaseController }o--|| IDatabaseService : implements
Class diagram for Tux bot and dependency injection changesclassDiagram
class Tux {
+ServiceContainer container
+SentryManager sentry_manager
+EmojiManager emoji_manager
+DatabaseService db
+setup()
+_setup_container()
+_validate_container()
+_raise_container_validation_error()
+_cleanup_container()
}
class ServiceContainer {
+get_optional(interface)
}
class ServiceRegistry {
+configure_container(bot)
+validate_container(container)
+get_registered_services(container)
}
class SentryManager {
+setup()
+capture_exception()
+finish_transaction_on_error()
+set_command_context()
+flush_async()
+is_initialized
}
Tux --> ServiceContainer
Tux --> SentryManager
Tux --> EmojiManager
Tux --> DatabaseService
ServiceRegistry --> ServiceContainer
ServiceContainer --> SentryManager
ServiceContainer --> EmojiManager
ServiceContainer --> DatabaseService
Class diagram for ConfigSet* UI views with DI changesclassDiagram
class ConfigSetPrivateLogs {
+__init__(timeout, bot, db_service)
-db: GuildConfigController
}
class ConfigSetPublicLogs {
+__init__(timeout, bot, db_service)
-db: GuildConfigController
}
class ConfigSetChannels {
+__init__(timeout, bot, db_service)
-db: GuildConfigController
}
class IDatabaseService {
+get_controller()
}
class GuildConfigController {
}
ConfigSetPrivateLogs --> IDatabaseService
ConfigSetPublicLogs --> IDatabaseService
ConfigSetChannels --> IDatabaseService
IDatabaseService --> GuildConfigController
Class diagram for Setup cog refactor to use DI and base classclassDiagram
class Setup {
+__init__(bot)
-config: GuildConfigController
}
class BaseCog {
+__init__(bot)
-db: IDatabaseService
}
class IDatabaseService {
+get_controller()
}
class GuildConfigController {
}
Setup --|> BaseCog
BaseCog --> IDatabaseService
IDatabaseService --> GuildConfigController
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @kzndotsh - I've reviewed your changes and they look great!
Blocking issues:
- By not specifying a USER, a program in the container may run as 'root'. This is a security hazard. If an attacker can control a process running as root, they may have control over the container. Ensure that the last USER in a Dockerfile is a USER other than 'root'. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/tux/services/tracing.py:80` </location>
<code_context>
+# --- Common Helpers ---
+
+
+def safe_set_name(obj: Any, name: str) -> None:
+ """
+ Safely set the name on a span or transaction object.
</code_context>
<issue_to_address>
safe_set_name may fail silently if set_name is not callable.
Add a check to ensure set_name is callable before invoking it to prevent silent failures.
</issue_to_address>
### Comment 2
<location> `src/tux/services/tracing.py:598` </location>
<code_context>
+ raise
+
+
+def instrument_bot_commands(bot: commands.Bot) -> None:
+ """
+ Automatically instruments all bot commands with Sentry transactions.
</code_context>
<issue_to_address>
Instrumenting bot commands by replacing callbacks may interfere with other decorators.
Directly replacing the command callback can disrupt existing decorators or attributes. Using a wrapper or middleware would help maintain original metadata.
</issue_to_address>
### Comment 3
<location> `src/tux/core/container.py:244` </location>
<code_context>
+ return descriptor.instance
+
+ # Create new instance
+ self._resolution_stack.add(service_type)
+
+ try:
</code_context>
<issue_to_address>
Using a set for _resolution_stack may not preserve dependency order for error reporting.
Consider replacing the set with a list or stack to maintain resolution order, which will improve error messages for circular dependencies.
Suggested implementation:
```python
# Create new instance
self._resolution_stack.append(service_type)
```
```python
try:
```
```python
except Exception as e:
stack_trace = " -> ".join([t.__name__ for t in self._resolution_stack])
logger.error(f"Failed to resolve {service_type.__name__}: {e}\nResolution stack: {stack_trace}")
error_msg = f"Cannot resolve {service_type.__name__} (resolution stack: {stack_trace})"
raise ServiceResolutionError(error_msg) from e
```
```python
else:
resolution_time = time.perf_counter() - start_time
# Only log if resolution takes longer than expected or fails
if resolution_time > 0.001: # Log if takes more than 1ms
logger.debug(f"Slow resolution: {service_type.__name__} took {resolution_time:.4f}s")
self._resolution_stack.pop()
```
You will also need to:
1. Change the initialization of `self._resolution_stack` from a set to a list (e.g., `self._resolution_stack = []`).
2. Update any other code that checks membership (`in`) or removes items from `_resolution_stack` to use list semantics.
3. If you have circular dependency checks, you can still use `if service_type in self._resolution_stack:` for lists.
</issue_to_address>
### Comment 4
<location> `src/tux/core/container.py:299` </location>
<code_context>
+ # Resolve the dependency
+ dependency = self._resolve_service(param_type)
+
+ if param.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD:
+ if param.default is inspect.Parameter.empty:
+ args.append(dependency)
+ else:
+ kwargs[param.name] = dependency
+ elif param.kind == inspect.Parameter.KEYWORD_ONLY:
+ kwargs[param.name] = dependency
+
</code_context>
<issue_to_address>
Does not handle VAR_POSITIONAL or VAR_KEYWORD constructor parameters.
Currently, *args and **kwargs in service constructors are not processed. Please add explicit handling for VAR_POSITIONAL and VAR_KEYWORD parameters, either by raising an error or documenting their unsupported status.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
# Resolve the dependency
dependency = self._resolve_service(param_type)
if param.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD:
if param.default is inspect.Parameter.empty:
args.append(dependency)
else:
kwargs[param.name] = dependency
elif param.kind == inspect.Parameter.KEYWORD_ONLY:
kwargs[param.name] = dependency
=======
# Resolve the dependency
dependency = self._resolve_service(param_type)
if param.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD:
if param.default is inspect.Parameter.empty:
args.append(dependency)
else:
kwargs[param.name] = dependency
elif param.kind == inspect.Parameter.KEYWORD_ONLY:
kwargs[param.name] = dependency
elif param.kind == inspect.Parameter.VAR_POSITIONAL:
raise ServiceResolutionError(
f"Constructor parameter '*{param.name}' in {impl_type.__name__} is not supported by the DI container"
)
elif param.kind == inspect.Parameter.VAR_KEYWORD:
raise ServiceResolutionError(
f"Constructor parameter '**{param.name}' in {impl_type.__name__} is not supported by the DI container"
)
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `docs/content/dev/local_development.md:37` </location>
<code_context>
The project includes a hot-reloading utility (`tux/utils/hot_reload.py`).
-When the bot is running locally via `poetry run tux --dev start`, this utility watches for changes in the `tux/cogs/` directory. It attempts to automatically reload modified cogs or cogs affected by changes in watched utility files without requiring a full bot restart.
+When the bot is running locally via `uv run tux --dev start`, this utility watches for changes in the `tux/cogs/` directory. It attempts to automatically reload modified cogs or cogs affected by changes in watched utility files without requiring a full bot restart.
-This significantly speeds up development for cog-related changes. Note that changes outside the watched directories (e.g., core bot logic, dependencies) may still require a manual restart (`Ctrl+C` and run the start command again).
</code_context>
<issue_to_address>
Update directory and terminology to match new module structure.
Please update all directory and terminology references to align with the new module structure for consistency.
</issue_to_address>
## Security Issues
### Issue 1
<location> `Dockerfile:259` </location>
<issue_to_address>
**security (dockerfile.security.missing-user):** By not specifying a USER, a program in the container may run as 'root'. This is a security hazard. If an attacker can control a process running as root, they may have control over the container. Ensure that the last USER in a Dockerfile is a USER other than 'root'.
```suggestion
USER non-root
CMD ["sh", "-c", "uv run prisma generate && exec uv run tux --dev start"]
```
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # --- Common Helpers --- | ||
|
|
||
|
|
||
| def safe_set_name(obj: Any, name: str) -> None: |
There was a problem hiding this comment.
suggestion (bug_risk): safe_set_name may fail silently if set_name is not callable.
Add a check to ensure set_name is callable before invoking it to prevent silent failures.
| raise | ||
|
|
||
|
|
||
| def instrument_bot_commands(bot: commands.Bot) -> None: |
There was a problem hiding this comment.
issue: Instrumenting bot commands by replacing callbacks may interfere with other decorators.
Directly replacing the command callback can disrupt existing decorators or attributes. Using a wrapper or middleware would help maintain original metadata.
src/tux/core/container.py
Outdated
| return descriptor.instance | ||
|
|
||
| # Create new instance | ||
| self._resolution_stack.add(service_type) |
There was a problem hiding this comment.
suggestion: Using a set for _resolution_stack may not preserve dependency order for error reporting.
Consider replacing the set with a list or stack to maintain resolution order, which will improve error messages for circular dependencies.
Suggested implementation:
# Create new instance
self._resolution_stack.append(service_type) try: except Exception as e:
stack_trace = " -> ".join([t.__name__ for t in self._resolution_stack])
logger.error(f"Failed to resolve {service_type.__name__}: {e}\nResolution stack: {stack_trace}")
error_msg = f"Cannot resolve {service_type.__name__} (resolution stack: {stack_trace})"
raise ServiceResolutionError(error_msg) from e else:
resolution_time = time.perf_counter() - start_time
# Only log if resolution takes longer than expected or fails
if resolution_time > 0.001: # Log if takes more than 1ms
logger.debug(f"Slow resolution: {service_type.__name__} took {resolution_time:.4f}s")
self._resolution_stack.pop()You will also need to:
- Change the initialization of
self._resolution_stackfrom a set to a list (e.g.,self._resolution_stack = []). - Update any other code that checks membership (
in) or removes items from_resolution_stackto use list semantics. - If you have circular dependency checks, you can still use
if service_type in self._resolution_stack:for lists.
src/tux/core/container.py
Outdated
Comment on lines
296
to
305
| # Resolve the dependency | ||
| dependency = self._resolve_service(param_type) | ||
|
|
||
| if param.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD: | ||
| if param.default is inspect.Parameter.empty: | ||
| args.append(dependency) | ||
| else: | ||
| kwargs[param.name] = dependency | ||
| elif param.kind == inspect.Parameter.KEYWORD_ONLY: | ||
| kwargs[param.name] = dependency |
There was a problem hiding this comment.
suggestion: Does not handle VAR_POSITIONAL or VAR_KEYWORD constructor parameters.
Currently, *args and **kwargs in service constructors are not processed. Please add explicit handling for VAR_POSITIONAL and VAR_KEYWORD parameters, either by raising an error or documenting their unsupported status.
Suggested change
| # Resolve the dependency | |
| dependency = self._resolve_service(param_type) | |
| if param.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD: | |
| if param.default is inspect.Parameter.empty: | |
| args.append(dependency) | |
| else: | |
| kwargs[param.name] = dependency | |
| elif param.kind == inspect.Parameter.KEYWORD_ONLY: | |
| kwargs[param.name] = dependency | |
| # Resolve the dependency | |
| dependency = self._resolve_service(param_type) | |
| if param.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD: | |
| if param.default is inspect.Parameter.empty: | |
| args.append(dependency) | |
| else: | |
| kwargs[param.name] = dependency | |
| elif param.kind == inspect.Parameter.KEYWORD_ONLY: | |
| kwargs[param.name] = dependency | |
| elif param.kind == inspect.Parameter.VAR_POSITIONAL: | |
| raise ServiceResolutionError( | |
| f"Constructor parameter '*{param.name}' in {impl_type.__name__} is not supported by the DI container" | |
| ) | |
| elif param.kind == inspect.Parameter.VAR_KEYWORD: | |
| raise ServiceResolutionError( | |
| f"Constructor parameter '**{param.name}' in {impl_type.__name__} is not supported by the DI container" | |
| ) |
| The project includes a hot-reloading utility (`tux/utils/hot_reload.py`). | ||
|
|
||
| When the bot is running locally via `poetry run tux --dev start`, this utility watches for changes in the `tux/cogs/` directory. It attempts to automatically reload modified cogs or cogs affected by changes in watched utility files without requiring a full bot restart. | ||
| When the bot is running locally via `uv run tux --dev start`, this utility watches for changes in the `tux/cogs/` directory. It attempts to automatically reload modified cogs or cogs affected by changes in watched utility files without requiring a full bot restart. |
There was a problem hiding this comment.
suggestion: Update directory and terminology to match new module structure.
Please update all directory and terminology references to align with the new module structure for consistency.
Dockerfile
Outdated
| # WORKFLOW: Regenerates Prisma client and starts the bot in development mode | ||
| # This ensures the database client is always up-to-date with schema changes | ||
| CMD ["sh", "-c", "poetry run prisma generate && exec poetry run tux --dev start"] | ||
| CMD ["sh", "-c", "uv run prisma generate && exec uv run tux --dev start"] |
There was a problem hiding this comment.
security (dockerfile.security.missing-user): By not specifying a USER, a program in the container may run as 'root'. This is a security hazard. If an attacker can control a process running as root, they may have control over the container. Ensure that the last USER in a Dockerfile is a USER other than 'root'.
Suggested change
| CMD ["sh", "-c", "uv run prisma generate && exec uv run tux --dev start"] | |
| USER non-root | |
| CMD ["sh", "-c", "uv run prisma generate && exec uv run tux --dev start"] |
Source: opengrep
tests/e2e/test_smoke_e2e.py
Outdated
Comment on lines
5
to
6
| # Keep E2E minimal and deterministic; expand with CLI or HTTP flows later | ||
| assert True |
There was a problem hiding this comment.
suggestion (code-quality): Remove assert True statements (remove-assert-true)
Suggested change
| # Keep E2E minimal and deterministic; expand with CLI or HTTP flows later | |
| assert True | |
| pass |
Comment on lines
5
to
6
| # Example of an integration placeholder; expand with real IO later | ||
| assert 1 + 1 == 2 |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Remove
assert Truestatements (remove-assert-true) - Simplify numeric comparison (
simplify-numeric-comparison) - Simplify x == x -> True and x != x -> False (
equality-identity)
Suggested change
| # Example of an integration placeholder; expand with real IO later | |
| assert 1 + 1 == 2 | |
| pass |
tests/unit/test_smoke.py
Outdated
|
|
||
| @pytest.mark.unit | ||
| def test_smoke() -> None: | ||
| assert True |
There was a problem hiding this comment.
suggestion (code-quality): Remove assert True statements (remove-assert-true)
Suggested change
| assert True | |
| pass |
There was a problem hiding this comment.
Hey @kzndotsh - I've reviewed your changes and they look great!
Blocking issues:
- By not specifying a USER, a program in the container may run as 'root'. This is a security hazard. If an attacker can control a process running as root, they may have control over the container. Ensure that the last USER in a Dockerfile is a USER other than 'root'. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/tux/services/sentry_manager.py:582` </location>
<code_context>
+ for key, value in tags.items():
+ scope.set_tag(key, value)
+
+ def set_user_context(self, user: discord.User | discord.Member) -> None:
+ """
+ Sets the user context for the current Sentry scope.
</code_context>
<issue_to_address>
set_user_context may expose sensitive user information.
Review the user and member fields being attached to Sentry to ensure only essential, non-sensitive information is included, particularly for production use.
</issue_to_address>
### Comment 2
<location> `src/tux/core/container.py:57` </location>
<code_context>
+ """Initialize an empty service container."""
+ self._services: dict[type, ServiceDescriptor] = {}
+ self._singleton_instances: dict[type, Any] = {}
+ self._resolution_stack: set[type] = set()
+
+ def register_singleton(self, service_type: type[T], implementation: type[T] | None = None) -> "ServiceContainer":
</code_context>
<issue_to_address>
Using a set for the resolution stack may obscure the actual dependency chain in circular dependency errors.
Using a list for the resolution stack will preserve the order of dependencies, making circular dependency errors easier to trace.
Suggested implementation:
```python
self._resolution_stack: list[type] = []
```
You will need to update all usages of `_resolution_stack` throughout the file:
- Replace `.add(x)` with `.append(x)`
- Replace `.remove(x)` with `.pop()` (if popping the last item) or `del` (if removing by index)
- If checking for membership, `x in self._resolution_stack` still works
- When reporting circular dependencies, you can now print the stack in order for better error messages
</issue_to_address>
### Comment 3
<location> `src/tux/core/container.py:235` </location>
<code_context>
+ descriptor = self._services[service_type]
+
+ # Return existing instance for singletons
+ if descriptor.lifetime == ServiceLifetime.SINGLETON:
+ if service_type in self._singleton_instances:
+ return self._singleton_instances[service_type]
+
+ # If we have a pre-registered instance, return it
+ if descriptor.instance is not None:
+ return descriptor.instance
+
</code_context>
<issue_to_address>
Singleton instance caching logic may be redundant if both instance and _singleton_instances are used.
Consolidate singleton instance handling to ensure a single source of truth and prevent precedence ambiguity.
Suggested implementation:
```python
# Return existing instance for singletons
if descriptor.lifetime == ServiceLifetime.SINGLETON:
if service_type in self._singleton_instances:
return self._singleton_instances[service_type]
```
To fully implement this, you must ensure that any pre-registered singleton instance (previously stored in `descriptor.instance`) is instead stored in `_singleton_instances` at registration time. This may require updating your service registration logic elsewhere in the codebase to move any instance assignment to `_singleton_instances`.
</issue_to_address>
### Comment 4
<location> `src/tux/core/context.py:95` </location>
<code_context>
+ A dictionary with standardized context keys like `user_id`,
+ `command_name`, `guild_id`, `command_type`, etc.
+ """
+ user = source.user if isinstance(source, Interaction) else source.author
+
+ # Base context is common to both types
</code_context>
<issue_to_address>
Assumes that both Context and Interaction have user/author attributes, which may not always be true.
Add error handling or a fallback to prevent AttributeError if the expected attribute is missing.
</issue_to_address>
## Security Issues
### Issue 1
<location> `Dockerfile:259` </location>
<issue_to_address>
**security (dockerfile.security.missing-user):** By not specifying a USER, a program in the container may run as 'root'. This is a security hazard. If an attacker can control a process running as root, they may have control over the container. Ensure that the last USER in a Dockerfile is a USER other than 'root'.
```suggestion
USER non-root
CMD ["sh", "-c", "uv run prisma generate && exec uv run tux --dev start"]
```
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/tux/services/sentry_manager.py
Outdated
| for key, value in tags.items(): | ||
| scope.set_tag(key, value) | ||
|
|
||
| def set_user_context(self, user: discord.User | discord.Member) -> None: |
There was a problem hiding this comment.
🚨 suggestion (security): set_user_context may expose sensitive user information.
Review the user and member fields being attached to Sentry to ensure only essential, non-sensitive information is included, particularly for production use.
src/tux/core/container.py
Outdated
| """Initialize an empty service container.""" | ||
| self._services: dict[type, ServiceDescriptor] = {} | ||
| self._singleton_instances: dict[type, Any] = {} | ||
| self._resolution_stack: set[type] = set() |
There was a problem hiding this comment.
suggestion: Using a set for the resolution stack may obscure the actual dependency chain in circular dependency errors.
Using a list for the resolution stack will preserve the order of dependencies, making circular dependency errors easier to trace.
Suggested implementation:
self._resolution_stack: list[type] = []You will need to update all usages of _resolution_stack throughout the file:
- Replace
.add(x)with.append(x) - Replace
.remove(x)with.pop()(if popping the last item) ordel(if removing by index) - If checking for membership,
x in self._resolution_stackstill works - When reporting circular dependencies, you can now print the stack in order for better error messages
src/tux/core/container.py
Outdated
Comment on lines
235
to
240
| if descriptor.lifetime == ServiceLifetime.SINGLETON: | ||
| if service_type in self._singleton_instances: | ||
| return self._singleton_instances[service_type] | ||
|
|
||
| # If we have a pre-registered instance, return it | ||
| if descriptor.instance is not None: |
There was a problem hiding this comment.
suggestion: Singleton instance caching logic may be redundant if both instance and _singleton_instances are used.
Consolidate singleton instance handling to ensure a single source of truth and prevent precedence ambiguity.
Suggested implementation:
# Return existing instance for singletons
if descriptor.lifetime == ServiceLifetime.SINGLETON:
if service_type in self._singleton_instances:
return self._singleton_instances[service_type]To fully implement this, you must ensure that any pre-registered singleton instance (previously stored in descriptor.instance) is instead stored in _singleton_instances at registration time. This may require updating your service registration logic elsewhere in the codebase to move any instance assignment to _singleton_instances.
src/tux/core/context.py
Outdated
| A dictionary with standardized context keys like `user_id`, | ||
| `command_name`, `guild_id`, `command_type`, etc. | ||
| """ | ||
| user = source.user if isinstance(source, Interaction) else source.author |
There was a problem hiding this comment.
issue: Assumes that both Context and Interaction have user/author attributes, which may not always be true.
Add error handling or a fallback to prevent AttributeError if the expected attribute is missing.
Dockerfile
Outdated
| # WORKFLOW: Regenerates Prisma client and starts the bot in development mode | ||
| # This ensures the database client is always up-to-date with schema changes | ||
| CMD ["sh", "-c", "poetry run prisma generate && exec poetry run tux --dev start"] | ||
| CMD ["sh", "-c", "uv run prisma generate && exec uv run tux --dev start"] |
There was a problem hiding this comment.
security (dockerfile.security.missing-user): By not specifying a USER, a program in the container may run as 'root'. This is a security hazard. If an attacker can control a process running as root, they may have control over the container. Ensure that the last USER in a Dockerfile is a USER other than 'root'.
Suggested change
| CMD ["sh", "-c", "uv run prisma generate && exec uv run tux --dev start"] | |
| USER non-root | |
| CMD ["sh", "-c", "uv run prisma generate && exec uv run tux --dev start"] |
Source: opengrep
tests/e2e/test_smoke_e2e.py
Outdated
Comment on lines
5
to
6
| # Keep E2E minimal and deterministic; expand with CLI or HTTP flows later | ||
| assert True |
There was a problem hiding this comment.
suggestion (code-quality): Remove assert True statements (remove-assert-true)
Suggested change
| # Keep E2E minimal and deterministic; expand with CLI or HTTP flows later | |
| assert True | |
| pass |
Comment on lines
5
to
6
| # Example of an integration placeholder; expand with real IO later | ||
| assert 1 + 1 == 2 |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Remove
assert Truestatements (remove-assert-true) - Simplify numeric comparison (
simplify-numeric-comparison) - Simplify x == x -> True and x != x -> False (
equality-identity)
Suggested change
| # Example of an integration placeholder; expand with real IO later | |
| assert 1 + 1 == 2 | |
| pass |
tests/unit/test_smoke.py
Outdated
|
|
||
| @pytest.mark.unit | ||
| def test_smoke() -> None: | ||
| assert True |
There was a problem hiding this comment.
suggestion (code-quality): Remove assert True statements (remove-assert-true)
Suggested change
| assert True | |
| pass |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/tux/database/migrations/versions/2025_08_19_0437-12574673e637_initial_baseline_migration.py
Show resolved
Hide resolved
src/tux/database/migrations/versions/2025_08_19_0437-12574673e637_initial_baseline_migration.py
Show resolved
Hide resolved
src/tux/database/migrations/versions/2025_08_19_0437-12574673e637_initial_baseline_migration.py
Show resolved
Hide resolved
src/tux/database/migrations/versions/2025_08_19_0437-12574673e637_initial_baseline_migration.py
Show resolved
Hide resolved
src/tux/database/migrations/versions/2025_08_19_0437-12574673e637_initial_baseline_migration.py
Show resolved
Hide resolved
- Changed the import statement for CaseType from tux.database.models.moderation to tux.database.models for improved clarity and organization. - Ensured consistency in import paths across the codebase.
- Changed the import statement for Case from tux.database.models.moderation to tux.database.models for improved clarity and consistency. - Enhanced the organization of import paths in the exceptions module.
- Changed the import statements for CaseType and Case from tux.database.models.moderation to tux.database.models for improved clarity and consistency across moderation modules. - Enhanced organization of import paths in moderation-related files.
- Changed import statements for various models (AFK, Case, GuildConfig, Levels, Reminder, Snippet, Starboard, StarboardMessage) from specific submodules to a unified import from tux.database.models for improved clarity and consistency across the database controllers. - Enhanced organization of import paths in the database controller files.
- Updated the migration scripts in env.py to improve handling of both synchronous and asynchronous database connections, ensuring compatibility with pytest-alembic. - Refactored Alembic configuration setup in runner.py to include additional options for better migration management and logging. - Removed the obsolete initial baseline migration file to streamline the migration history. - Improved organization of import paths for clarity and consistency across migration files.
This commit modifies the alias for the `clearafk` command from `removeafk` to `unafk`, enhancing clarity and consistency in command naming. The change aims to improve user experience by providing a more intuitive command alias.
…se-driven decorators This commit introduces a fully dynamic permission system that replaces the previous hardcoded permission levels with a database-driven approach. The `requires_command_permission` decorator is added to facilitate dynamic permission checks for commands, ensuring that permission requirements are configurable per guild. The legacy `ConditionChecker` and associated permission level enums are removed to streamline the codebase. Additionally, the permission system architecture is updated to utilize numeric ranks (0-100) instead of fixed levels, enhancing flexibility and scalability for server-specific configurations.
…ollers This commit removes the GuildBlacklistController and GuildWhitelistController from the DatabaseCoordinator class, streamlining the code by eliminating unused properties and associated methods. This cleanup enhances code clarity and maintainability by focusing on currently utilized components.
…ture This commit adds a new base model and mixins for enhanced serialization and timestamp management, along with enums for permission and case types. The existing models are updated to utilize these new structures, streamlining the codebase and improving maintainability. Additionally, the old complex permission model is removed in favor of a simpler dynamic permission system, aligning with recent architectural changes.
…cture This commit modifies the imports in the migration environment file to include new permission-related models: GuildCommandPermission, GuildPermissionAssignment, and GuildPermissionRank. This change aligns with the recent updates to the permission system, ensuring that the database schema reflects the current architecture and improves maintainability.
This commit adds functionality to the EventHandler class to register all guilds the bot is in upon startup. It includes logging to track the registration process and handles potential errors if a guild is already registered. This enhancement improves the bot's initialization process and ensures that all guilds are accounted for in the database.
… permission system This commit introduces a suite of integration and unit tests for the newly implemented dynamic permission system. It includes tests for permission rank management, command permission requirements, and error handling. Additionally, utility functions for error detail extraction are tested to ensure robustness. The tests enhance coverage and reliability of the permission system, ensuring it functions correctly across various scenarios.
…ermissionDeniedError This commit modifies the existing permission error messages to use "rank" instead of "level" for clarity. Additionally, it introduces a new exception class, TuxPermissionDeniedError, which provides detailed messages when a user lacks the required permission rank to execute a command, enhancing error handling in the dynamic permission system.
…system This commit adds a new Config cog that allows server administrators to manage guild configurations, including permission ranks, role assignments, command permissions, and log channels. The system features a structured command interface for easy configuration and provides an overview of the current settings. Additionally, it replaces the previous permission management module, streamlining the permission system into a more dynamic and user-friendly format.
…ion system changes This commit revises the README.md to replace references to the `ConditionChecker` with the new `Permission System`, clarifying its role in dynamic permission validation. It updates method decorators from `@require_moderator()` to `@requires_command_permission()` across various moderation commands, ensuring consistency with the new permission architecture. Additionally, documentation issues regarding service count and naming have been resolved, enhancing clarity and accuracy.
…arity and organization This commit significantly restructures the PostgreSQL configuration file, enhancing readability and maintainability. It replaces the previous Tux-specific comments with a more standardized format, providing detailed explanations for various settings. Key parameters such as connection settings, memory configuration, and logging options have been clearly documented, ensuring better understanding for future modifications. Additionally, unnecessary Tux-specific optimizations have been removed, streamlining the configuration for broader use.
…ion assignment tests This commit enhances the integration tests for the guild permission assignment controller by adding assertions to ensure that the created permission ranks have valid IDs. These checks improve the reliability of the tests by confirming that ranks are correctly instantiated before being assigned to roles.
…ding methods This commit renames the GuildPermissionController to GuildPermissionRankController to better reflect its purpose in managing permission ranks. Additionally, it introduces new onboarding-specific methods in the GuildConfigController for updating and retrieving onboarding status, enhancing the guild configuration management capabilities. The onboarding stage is also defined as an enum for better clarity and structure in the database models.
…uration This commit introduces a comprehensive onboarding system for guilds, featuring an interactive setup wizard that guides users through essential configuration steps. The wizard includes functionality for initializing permission ranks, selecting log channels, and assigning roles to permission levels. Additionally, it provides a completion step that summarizes the setup status and ensures a smooth onboarding experience for new guilds. Various UI components and callback handlers have been implemented to facilitate user interactions throughout the setup process.
…rmission ranks This commit introduces a new method, `remove_role_assignment`, to the PermissionSystem class, allowing for the removal of permission rank assignments from roles. The method clears the cache for the specified guild and logs the removal action, enhancing the management of role permissions within the system.
…configuration This commit updates the PostgreSQL image from version 15-alpine to 17-alpine in the Docker Compose configuration. Additionally, it modifies the health check script to reference the updated configuration structure, ensuring that the bot token check aligns with the new environment variable naming convention.
…ontent directly This commit refactors the CogLoader class to check for the presence of a setup function by reading the file content instead of importing the module. This change improves error handling and avoids potential import issues, enhancing the reliability of the cog loading process.
This commit refactors the logging configuration by removing the file logging setup from the `configure_logging` function. The `enable_file_logging` parameter and related file logging methods have been eliminated, simplifying the logging configuration process and focusing on console logging only.
…input This commit refactors the `deepfry` method in the Deepfry class to require an image attachment as input, simplifying the validation process. The image URL extraction logic has been updated to directly use the provided attachment, enhancing clarity and reducing unnecessary checks for image sources. This change improves the overall reliability of the image processing functionality.
This commit adds the GuildOnboardingService to the EventHandler class, initializing onboarding for new guilds when they join. The onboarding process is triggered in the on_guild_join method, enhancing the setup experience for new communities. A TODO comment has also been added to define a data expiration policy for guilds in the future.
| ADMINER_DEFAULT_USERNAME: ${POSTGRES_USER:-tuxuser} | ||
| ADMINER_DEFAULT_PASSWORD: ${POSTGRES_PASSWORD:-ChangeThisToAStrongPassword123!} | ||
| ADMINER_AUTO_LOGIN: "${ADMINER_AUTO_LOGIN:-true}" | ||
| ADMINER_PLUGINS: "backward-keys tables-filter dump-date dump-json dump-xml dump-zip edit-calendar enum-option foreign-system json-column pretty-json-column table-indexes-structure table-structure" |
There was a problem hiding this comment.
[yamllint] reported by reviewdog 🐶
[warning] line too long (202 > 185 characters) (line-length)
…port notifier to individual plugins
…CRUD and Query controllers This commit enhances the CRUD and Query controllers by adding instance expunging after records are retrieved or updated. This change ensures that instances can be reused across different sessions, improving session management and resource handling. The UpsertController is also updated to expunge the merged instance, maintaining consistency across the database interaction methods.
This commit introduces a comprehensive configuration system for guild management, replacing the previous implementation. The new structure includes separate modules for management, overview, and an interactive setup wizard, enhancing organization and maintainability. Key features include commands for managing permission ranks, role assignments, and command permissions, along with an overview command to display the current configuration status. This refactor aims to improve the user experience and streamline configuration processes for guild administrators.
This commit deletes the onboarding service package, including its decorators, utility functions, and the main service class. The onboarding functionality has been streamlined, and the setup wizard has been updated to directly handle permission initialization and onboarding stages without relying on the removed service. This change simplifies the onboarding process and reduces code complexity.
…tics This commit introduces a new command, `membercount`, to the MemberCount cog, which provides the total number of members, humans, and bots in the server. The command is designed to enhance user engagement by offering insights into the server's composition through an embedded message format.
…set onboarding stage This commit modifies the on_guild_join method in the EventHandler class to remove the GuildOnboardingService and instead directly update the onboarding stage for new guilds. This change simplifies the onboarding process by focusing on basic guild data initialization, enhancing the setup experience for new communities.
This commit introduces a new priority level for the "config" cog in the COG_PRIORITIES dictionary, setting its loading priority to 85. This change enhances the organization of cog loading by clearly defining the order of initialization for different components.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change. If this change fixes any issues please put "Fixes #XX" in the description. Please also ensure to add the appropriate labels to the PR.
Guidelines
My code follows the style guidelines of this project (formatted with Ruff)
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation if needed
My changes generate no new warnings
I have tested this change
Any dependent changes have been merged and published in downstream modules
I have added all appropriate labels to this PR
I have followed all of these guidelines.
How Has This Been Tested? (if applicable)
Please describe how you tested your code. e.g describe what commands you ran, what arguments, and any config stuff (if applicable)
Screenshots (if applicable)
Please add screenshots to help explain your changes.
Additional Information
Please add any other information that is important to this PR.
Summary by Sourcery
Migrate project from Poetry to Uv (Hatch) as the dependency and build tool, restructure code under src/, introduce a dependency injection container with BaseCog for service resolution, centralize Sentry integration via a new SentryManager, and overhaul tracing/error handling instrumentation. Update CLI commands, Docker configurations, CI workflows, documentation, and tests to use Uv, add unit/integration/e2e test markers with isolation, and bump version to v0.1.0.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests:
Chores: