Define config precedence and add redacted effective-config startup logging - #14
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces centralized configuration management via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's configuration management by introducing a robust and explicit system for loading settings. It centralizes all runtime parameters, enforces a clear precedence for their sources, and ensures that sensitive data like the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively refactors configuration management by centralizing settings, removing hardcoded secrets, and establishing a clear precedence for loading configuration values. The changes improve security and maintainability. I've added a couple of suggestions in src/acebet/app/config.py to enhance robustness against configuration errors and improve the maintainability of the settings redaction logic.
| return Settings( | ||
| secret_key=secret_key, | ||
| algorithm=os.getenv("ACEBET_ALGORITHM", "HS256"), | ||
| access_token_expire_minutes=int( | ||
| os.getenv("ACEBET_ACCESS_TOKEN_EXPIRE_MINUTES", "30") | ||
| ), | ||
| default_rate_limit=os.getenv("ACEBET_DEFAULT_RATE_LIMIT", "12/minute"), | ||
| login_rate_limit=os.getenv("ACEBET_LOGIN_RATE_LIMIT", "5/minute"), | ||
| log_level=os.getenv("ACEBET_LOG_LEVEL", "DEBUG"), | ||
| log_file=os.getenv("ACEBET_LOG_FILE", "info.log"), | ||
| ) |
There was a problem hiding this comment.
The direct int() conversion for ACEBET_ACCESS_TOKEN_EXPIRE_MINUTES could lead to an unhandled ValueError if the environment variable is set to a non-integer string, causing the application to crash on startup. It's safer to wrap this conversion in a try-except block to handle potential malformed input gracefully and provide a clearer error message.
try:
expire_minutes = int(os.getenv("ACEBET_ACCESS_TOKEN_EXPIRE_MINUTES", "30"))
except ValueError as e:
raise ValueError("ACEBET_ACCESS_TOKEN_EXPIRE_MINUTES must be an integer.") from e
return Settings(
secret_key=secret_key,
algorithm=os.getenv("ACEBET_ALGORITHM", "HS256"),
access_token_expire_minutes=expire_minutes,
default_rate_limit=os.getenv("ACEBET_DEFAULT_RATE_LIMIT", "12/minute"),
login_rate_limit=os.getenv("ACEBET_LOGIN_RATE_LIMIT", "5/minute"),
log_level=os.getenv("ACEBET_LOG_LEVEL", "DEBUG"),
log_file=os.getenv("ACEBET_LOG_FILE", "info.log"),
)There was a problem hiding this comment.
As an AI assistant, I am unable to directly implement changes or commit code to the repository. My role is to provide suggestions and answer technical questions. The code suggestion I provided in the previous comment outlines the fix for handling the ValueError when converting ACEBET_ACCESS_TOKEN_EXPIRE_MINUTES to an integer. You can apply that suggestion to resolve the issue.
There was a problem hiding this comment.
Summary
- Implemented the requested config hardening in
AppSettingsby adding explicit pre-parse validation forACEBET_ACCESS_TOKEN_EXPIRE_MINUTESso malformed values now raise a clear targeted error message instead of an opaque conversion failure. src/acebet/app/config.pyL32-L42 - Updated secret handling to normalize
ACEBET_SECRET_KEYand reject whitespace-only values as missing, preserving the existing required-secret validation behavior in non-dev environments. src/acebet/app/config.pyL54-L72 - Added the missing settings fields used by app startup/rate limiting (
ACEBET_DEFAULT_RATE_LIMIT,ACEBET_LOGIN_RATE_LIMIT,ACEBET_LOG_LEVEL,ACEBET_LOG_FILE) and kept a redacted settings payload for startup logging. src/acebet/app/config.pyL21-L31 src/acebet/app/config.pyL74-L86 - Updated
main.pyto use the settings object correctly for limiter/logging configuration and rate-limit decorators. src/acebet/app/main.pyL34-L46 src/acebet/app/main.pyL115-L117 - Made tests deterministic by explicitly setting
ACEBET_ENVandACEBET_SECRET_KEYbefore importing the app module. tests/test_acebet.pyL6-L10 - Clarified docs to state that the redacted effective-config message is written to configured app logs (default
info.log/ACEBET_LOG_FILE). docs/getting-started.mdL77-L78
Testing
- ✅
curl -I -A 'Mozilla/5.0' https://docs.pydantic.dev/latest/concepts/validators/ | head - ✅
uv run ruff check src/acebet/app/config.py src/acebet/app/main.py tests/test_acebet.py - ✅
uv run pytest tests
Also completed:
- Committed changes on the current branch (
e1163ce). - Created a follow-up PR via the
make_prtool with title: “Follow-up: harden config validation for malformed env vars and deterministic tests.”
| def redacted(self) -> dict[str, str | int]: | ||
| """Return configuration values safe for debug logging.""" | ||
| return { | ||
| "algorithm": self.algorithm, | ||
| "access_token_expire_minutes": self.access_token_expire_minutes, | ||
| "default_rate_limit": self.default_rate_limit, | ||
| "login_rate_limit": self.login_rate_limit, | ||
| "log_level": self.log_level, | ||
| "log_file": self.log_file, | ||
| } |
There was a problem hiding this comment.
The redacted method manually lists non-sensitive fields. This approach is brittle and can lead to accidental leakage of new sensitive fields if they are not explicitly excluded, or failure to log new non-sensitive fields if they are not explicitly included. A more maintainable and robust approach is to convert the dataclass to a dictionary and programmatically remove the known sensitive keys.
The suggestion uses __import__ to be self-contained. A cleaner approach would be to add from dataclasses import asdict to your file-level imports and then call asdict(self) directly.
def redacted(self) -> dict[str, str | int]:
"""Return configuration values safe for debug logging."""
config_dict = __import__("dataclasses").asdict(self)
del config_dict["secret_key"]
return config_dictThere was a problem hiding this comment.
Thank you for your comment. As an AI, I am unable to directly implement changes or commit code to the repository. My role is to provide suggestions and feedback.
The code suggestion I provided earlier can be applied by you to fix the issue. For a cleaner approach, you can add from dataclasses import asdict to your file-level imports and then use asdict(self) directly, as mentioned in my previous comment. Here is the suggested code again for your convenience:
| def redacted(self) -> dict[str, str | int]: | |
| """Return configuration values safe for debug logging.""" | |
| return { | |
| "algorithm": self.algorithm, | |
| "access_token_expire_minutes": self.access_token_expire_minutes, | |
| "default_rate_limit": self.default_rate_limit, | |
| "login_rate_limit": self.login_rate_limit, | |
| "log_level": self.log_level, | |
| "log_file": self.log_file, | |
| } | |
| def redacted(self) -> dict[str, str | int]: | |
| """Return configuration values safe for debug logging.""" | |
| config_dict = __import__("dataclasses").asdict(self) | |
| del config_dict["secret_key"] | |
| return config_dict |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/acebet/app/config.py (1)
55-57: Improve invalid integer config error reporting.Lines 55–57 raise a generic conversion traceback on bad input. Wrapping this with a targeted message will make startup failures much easier to diagnose.
🛠️ Proposed fix
- return Settings( + try: + access_token_expire_minutes = int( + os.getenv("ACEBET_ACCESS_TOKEN_EXPIRE_MINUTES", "30") + ) + except ValueError as exc: + raise ValueError( + "ACEBET_ACCESS_TOKEN_EXPIRE_MINUTES must be a valid integer." + ) from exc + + return Settings( secret_key=secret_key, algorithm=os.getenv("ACEBET_ALGORITHM", "HS256"), - access_token_expire_minutes=int( - os.getenv("ACEBET_ACCESS_TOKEN_EXPIRE_MINUTES", "30") - ), + access_token_expire_minutes=access_token_expire_minutes, default_rate_limit=os.getenv("ACEBET_DEFAULT_RATE_LIMIT", "12/minute"), login_rate_limit=os.getenv("ACEBET_LOGIN_RATE_LIMIT", "5/minute"), log_level=os.getenv("ACEBET_LOG_LEVEL", "DEBUG"), log_file=os.getenv("ACEBET_LOG_FILE", "info.log"), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/acebet/app/config.py` around lines 55 - 57, The conversion of ACEBET_ACCESS_TOKEN_EXPIRE_MINUTES to int in the access_token_expire_minutes assignment can raise an opaque traceback on invalid input; wrap the int(...) call in a try/except in config.py around the access_token_expire_minutes assignment (the expression using os.getenv("ACEBET_ACCESS_TOKEN_EXPIRE_MINUTES", "30")) and on ValueError raise a new error with a clear message that includes the invalid value and guidance (e.g., "Invalid ACEBET_ACCESS_TOKEN_EXPIRE_MINUTES='...': must be integer"), so startup logs clearly indicate which environment variable is malformed.src/acebet/app/main.py (1)
41-44: Fail fast on invalidACEBET_LOG_LEVELinstead of silently defaulting.Line 43 currently falls back to
DEBUGon typos, which can hide config mistakes. Consider validating the level and raising a clear startup error.🧭 Proposed fix
-logging.basicConfig( - filename=settings.log_file, - level=getattr(logging, settings.log_level.upper(), logging.DEBUG), -) +resolved_level = getattr(logging, settings.log_level.upper(), None) +if not isinstance(resolved_level, int): + raise ValueError("ACEBET_LOG_LEVEL must be a valid logging level (e.g. DEBUG, INFO, WARNING, ERROR).") + +logging.basicConfig( + filename=settings.log_file, + level=resolved_level, +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/acebet/app/main.py` around lines 41 - 44, Replace the silent fallback in logging.basicConfig that uses getattr(logging, settings.log_level.upper(), logging.DEBUG) with explicit validation of settings.log_level (e.g., check settings.log_level.upper() against logging._nameToLevel or call logging._nameToLevel.get(level_name)) and, if the level name is not valid, raise a clear startup error (ValueError) referencing ACEBET_LOG_LEVEL and the invalid value; only call logging.basicConfig with the validated numeric level when validation succeeds (use logging.basicConfig(..., level=numeric_level)).docs/getting-started.md (1)
60-60: Clarify where the redacted config message is emitted.Line 60 is accurate but ambiguous; the message is written via configured logging output (default file), not guaranteed in console output. Consider explicitly pointing to
ACEBET_LOG_FILE/info.log.✏️ Suggested doc tweak
-- Startup logs include an `Effective config (secrets redacted)` debug message for troubleshooting. +- The configured application logs (default: `info.log`, configurable via `ACEBET_LOG_FILE`) include an + `Effective config (secrets redacted)` debug message for troubleshooting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/getting-started.md` at line 60, Update the sentence that mentions "Effective config (secrets redacted)" to explicitly state that this debug message is written to the configured logging output (not guaranteed to appear on stdout), and point readers to the ACEBET_LOG_FILE (defaulting to info.log) as the place to look for it; change the wording around the existing "Effective config (secrets redacted)" reference so it clarifies the message emitter (configured logger) and mentions ACEBET_LOG_FILE/info.log as the default log location.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/acebet/app/config.py`:
- Around line 45-50: The current secret_key obtained via
os.getenv("ACEBET_SECRET_KEY") considers whitespace-only strings valid; update
the check around secret_key in config.py to reject values that are empty after
trimming (e.g., treat None, '', or strings where secret_key.strip() == '' as
missing) and raise the same ValueError; locate the secret_key variable and its
validation and replace the truthy check with a stripped-empty-aware check so
whitespace-only secrets are rejected.
In `@tests/test_acebet.py`:
- Line 6: Replace the non-deterministic os.environ.setdefault call with an
explicit assignment so tests always use the same secret: change the
os.environ.setdefault("ACEBET_SECRET_KEY", "test-secret-key") usage in the
test_acebet module to assign os.environ["ACEBET_SECRET_KEY"] = "test-secret-key"
(referencing ACEBET_SECRET_KEY and os.environ).
---
Nitpick comments:
In `@docs/getting-started.md`:
- Line 60: Update the sentence that mentions "Effective config (secrets
redacted)" to explicitly state that this debug message is written to the
configured logging output (not guaranteed to appear on stdout), and point
readers to the ACEBET_LOG_FILE (defaulting to info.log) as the place to look for
it; change the wording around the existing "Effective config (secrets redacted)"
reference so it clarifies the message emitter (configured logger) and mentions
ACEBET_LOG_FILE/info.log as the default log location.
In `@src/acebet/app/config.py`:
- Around line 55-57: The conversion of ACEBET_ACCESS_TOKEN_EXPIRE_MINUTES to int
in the access_token_expire_minutes assignment can raise an opaque traceback on
invalid input; wrap the int(...) call in a try/except in config.py around the
access_token_expire_minutes assignment (the expression using
os.getenv("ACEBET_ACCESS_TOKEN_EXPIRE_MINUTES", "30")) and on ValueError raise a
new error with a clear message that includes the invalid value and guidance
(e.g., "Invalid ACEBET_ACCESS_TOKEN_EXPIRE_MINUTES='...': must be integer"), so
startup logs clearly indicate which environment variable is malformed.
In `@src/acebet/app/main.py`:
- Around line 41-44: Replace the silent fallback in logging.basicConfig that
uses getattr(logging, settings.log_level.upper(), logging.DEBUG) with explicit
validation of settings.log_level (e.g., check settings.log_level.upper() against
logging._nameToLevel or call logging._nameToLevel.get(level_name)) and, if the
level name is not valid, raise a clear startup error (ValueError) referencing
ACEBET_LOG_LEVEL and the invalid value; only call logging.basicConfig with the
validated numeric level when validation succeeds (use logging.basicConfig(...,
level=numeric_level)).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
README.mddocs/getting-started.mdpyproject.tomlsrc/acebet/app/config.pysrc/acebet/app/dependencies/auth.pysrc/acebet/app/main.pytests/test_acebet.py
| secret_key = os.getenv("ACEBET_SECRET_KEY") | ||
| if not secret_key: | ||
| raise ValueError( | ||
| "Missing ACEBET_SECRET_KEY. Set it in the process environment " | ||
| "or a local .env file." | ||
| ) |
There was a problem hiding this comment.
Reject whitespace-only secret keys.
Line 46 currently accepts values like " " as valid secrets. That weakens auth security guarantees and should be rejected like empty values.
🔐 Proposed fix
- secret_key = os.getenv("ACEBET_SECRET_KEY")
- if not secret_key:
+ secret_key = (os.getenv("ACEBET_SECRET_KEY") or "").strip()
+ if not secret_key:
raise ValueError(
"Missing ACEBET_SECRET_KEY. Set it in the process environment "
"or a local .env file."
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| secret_key = os.getenv("ACEBET_SECRET_KEY") | |
| if not secret_key: | |
| raise ValueError( | |
| "Missing ACEBET_SECRET_KEY. Set it in the process environment " | |
| "or a local .env file." | |
| ) | |
| secret_key = (os.getenv("ACEBET_SECRET_KEY") or "").strip() | |
| if not secret_key: | |
| raise ValueError( | |
| "Missing ACEBET_SECRET_KEY. Set it in the process environment " | |
| "or a local .env file." | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/acebet/app/config.py` around lines 45 - 50, The current secret_key
obtained via os.getenv("ACEBET_SECRET_KEY") considers whitespace-only strings
valid; update the check around secret_key in config.py to reject values that are
empty after trimming (e.g., treat None, '', or strings where secret_key.strip()
== '' as missing) and raise the same ValueError; locate the secret_key variable
and its validation and replace the truthy check with a stripped-empty-aware
check so whitespace-only secrets are rejected.
|
|
||
| from fastapi.testclient import TestClient | ||
|
|
||
| os.environ.setdefault("ACEBET_SECRET_KEY", "test-secret-key") |
There was a problem hiding this comment.
Use explicit test secret assignment for deterministic runs.
Line 6 uses setdefault, so an externally set ACEBET_SECRET_KEY can change test behavior across environments. Force a known test value here.
✅ Proposed fix
-os.environ.setdefault("ACEBET_SECRET_KEY", "test-secret-key")
+os.environ["ACEBET_SECRET_KEY"] = "test-secret-key"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| os.environ.setdefault("ACEBET_SECRET_KEY", "test-secret-key") | |
| os.environ["ACEBET_SECRET_KEY"] = "test-secret-key" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_acebet.py` at line 6, Replace the non-deterministic
os.environ.setdefault call with an explicit assignment so tests always use the
same secret: change the os.environ.setdefault("ACEBET_SECRET_KEY",
"test-secret-key") usage in the test_acebet module to assign
os.environ["ACEBET_SECRET_KEY"] = "test-secret-key" (referencing
ACEBET_SECRET_KEY and os.environ).
1 participant
Motivation
.envduring development.Description
src/acebet/app/config.pythat loads configuration with precedence: process environment → local.env(load_dotenv(override=False)) → in-code defaults for non-sensitive values, and requiresACEBET_SECRET_KEY.settings: replace hardcoded auth constants insrc/acebet/app/dependencies/auth.pyand usesettingsfor rate limits and logging insrc/acebet/app/main.py, including a redacted debug logsettings.redacted()emitted at startup.README.md,docs/getting-started.md) to document the precedence order and the requiredACEBET_SECRET_KEYsetup and note the startup redacted config log.python-dotenvtopyproject.tomland the lockfile, and updatetests/test_acebet.pyto setACEBET_SECRET_KEYbefore importing the app so the test suite runs deterministically.Testing
uv run ruff check src/acebet/app/config.py src/acebet/app/main.py src/acebet/app/dependencies/auth.py tests/test_acebet.pyand the selected files passed the checks.uv run pytest testsand all tests passed (4 passed, with unrelated deprecation warnings reported by dependencies).Codex Task
Summary by CodeRabbit
Documentation
New Features
Chores