feat(security): add API rate limiting with slowapi#327
Conversation
Add comprehensive rate limiting to protect API endpoints from abuse: - Add slowapi dependency for FastAPI-compatible rate limiting - Create RateLimitConfig with environment variable support: - RATE_LIMIT_ENABLED: Enable/disable rate limiting (default: true) - RATE_LIMIT_AUTH: Auth endpoints limit (default: 10/minute) - RATE_LIMIT_STANDARD: Standard API limit (default: 100/minute) - RATE_LIMIT_AI: AI operations limit (default: 20/minute) - RATE_LIMIT_STORAGE: memory or redis (default: memory) - Create rate limiter middleware with: - Key extraction from user ID (authenticated) or IP (anonymous) - X-Forwarded-For/X-Real-IP header support for proxied requests - Custom 429 response with Retry-After header - Apply rate limits to critical endpoints: - AI rate limits on chat and agent start/resume endpoints - Standard rate limits on CRUD operations - Add rate limit audit logging events - Add 23 TDD tests for rate limiting functionality Rate limit categories: - auth: 10/minute (authentication endpoints) - standard: 100/minute (CRUD operations) - ai: 20/minute (LLM-powered operations) - websocket: 30/minute (connection rate limiting)
WalkthroughAdds environment-driven rate limiting: new RateLimitConfig and GlobalConfig fields, a slowapi-based Limiter (memory or Redis), per-category decorators applied across many routes, FastAPI middleware/handler wiring, audit events for rate limits, and tests plus test updates. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App as "FastAPI App"
participant Middleware as "SlowAPI Middleware"
participant Limiter as "Limiter (memory/Redis)"
participant Route as "Route Handler"
participant Audit as "Audit Logger"
Client->>App: HTTP request + headers
App->>Middleware: forward request
Middleware->>Limiter: check/increment key (IP or user)
Limiter-->>Middleware: allowed? / Retry-After
alt allowed
Middleware-->>App: continue
App->>Route: execute handler
Route-->>App: response
App-->>Client: 2xx response
else exceeded
Middleware-->>App: raise RateLimitExceeded
App->>Audit: log_rate_limit_event(endpoint, category)
Audit-->>App: logged
App-->>Client: 429 JSON + Retry-After
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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)
Comment |
Add API rate limiting with
|
Code Review - API Rate Limiting ImplementationThis is a well-structured implementation of rate limiting for the FastAPI server. The code is clean, well-tested, and follows good practices. StrengthsArchitecture & Design
Code Quality
Testing
Issues & Concerns1. Decorator Implementation Pattern (Medium Priority)The _create_rate_limit_decorator at lines 165-201 in codeframe/lib/rate_limiter.py has an unused wrapped function that is defined but never called when rate limiting is enabled (line 180-181). This is dead code that should be removed for clarity. The configuration check happens at decorator application time (module load), not at request time. This means toggling RATE_LIMIT_ENABLED requires a restart. This is acceptable but worth documenting. 2. Architecture Concern: v1 vs v2 Alignment (High Priority)According to CLAUDE.md, this codebase is in a v2 refactor phase where the goal is CLI-first, headless functionality. This PR adds rate limiting to the v1 FastAPI server, which is marked as optional adapter and legacy. Questions:
Recommendation: Get explicit sign-off that rate limiting is an acceptable v1 enhancement during the v2 refactor. (continued in next comment...) |
3. Configuration DuplicationRate limit configuration exists in TWO places:
Recommendation: Either unify into GlobalConfig (add all 7 fields) OR remove from GlobalConfig entirely and only use RateLimitConfig. Document why both are needed if there is a good reason. 4. Missing Redis Dependencypyproject.toml adds slowapi but not redis. If RATE_LIMIT_STORAGE=redis, the code will fail with ImportError. While the code handles this gracefully (falls back to memory at line 112), it would be better to:
5. X-Forwarded-For Security Risk (High Priority)Lines 49-52 in codeframe/lib/rate_limiter.py trust X-Forwarded-For unconditionally. This is a security risk. Security concern: X-Forwarded-For can be spoofed by clients. An attacker could set this header to bypass rate limits by using different IPs for each request. Recommendations:
This is especially important given the deployment mode system already in place (is_hosted_mode()). Minor Issues6. Test Assertion Could Be Stronger 7. Missing Rate Limit Headers on Success 8. No Distributed Rate Limiting Test 9. Missing Manual Testing Checklist |
Code Review - API Rate Limiting ImplementationI've reviewed this PR and overall the implementation is solid and well-structured. The rate limiting functionality is comprehensive, properly tested, and follows good security practices. However, there is one critical architectural concern that needs to be addressed based on the v2 refactor guidelines. Critical: v2 Architecture AlignmentAccording to CLAUDE.md, CodeFRAME is undergoing an in-place v2 refactor with a headless, CLI-first architecture. The current implementation adds rate limiting exclusively to the FastAPI server layer (codeframe/ui/routers/* and server.py), which is described as an optional adapter in the v2 architecture. The issue: Rate limiting is a security feature that should protect the entire system, not just the legacy API layer. If the v2 CLI or future adapters need rate limiting, this implementation won't help. RecommendationSince this is a v1/legacy feature (FastAPI server), I suggest accepting it as v1-only infrastructure. Acknowledge this is legacy code and may need refactoring in future phases. Add a TODO comment noting this should be reconsidered when Phase 2 (Server Layer) or Phase 3 (Web UI Rebuild) begins. For now, recommend option 1 with a clear comment in the code, since the v2 roadmap shows Phase 1 (CLI Completion) is still in progress. Strengths1. Excellent Test Coverage
2. Security Best Practices
3. Clean Abstractions
4. Deployment Flexibility
Issues and Suggestions1. Decorator Implementation Quirk (Minor)In rate_limiter.py:175-200, the decorator factory has an unnecessary nesting level. The function is called with an extra () which works but feels awkward. Impact: Low - it works fine, just unusual API design. 2. Missing Import Validation (Minor)rate_limiter.py:111 catches ImportError for Redis, but slowapi itself might not handle missing Redis gracefully. Consider checking if redis is installed before attempting to initialize with storage_uri. Impact: Low - but prevents potential runtime errors in production. 3. Duplicate Limiter Initialization (Code Smell)In server.py, the limiter is initialized in two places: inside lifespan() startup (lines 212-224) and at module level before lifespan runs (lines 263-268). Verify whether the duplicate initialization is actually needed. 4. Test Isolation Concerns (Medium)Several tests use reset functions to clean up global state. Not all tests reset state after running, and global state can leak between tests in parallel execution. Use pytest fixtures to ensure cleanup with an autouse fixture. 5. Missing Rate Limit Headers on Success (Info)Per RFC 6585 and common API practices, rate limit headers should be included on all responses, not just 429s (X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset). Consider adding middleware to inject these headers. Impact: Low - not critical for MVP, but improves API developer experience. Testing Suggestions
Summary
Approval StatusConditionally approve pending:
The implementation itself is solid. The main question is whether this fits the v2 roadmap direction or should be tagged as legacy infrastructure. Great work on the TDD approach and comprehensive coverage! |
Recommendations SummaryMust Address Before Merge:
Should Fix Before Merge: Nice to Have: Overall AssessmentThis is a solid, well-tested implementation with good architecture and comprehensive test coverage (23 tests). The code demonstrates good engineering practices with TDD, clear separation of concerns, and flexible configuration. Main concerns:
Test Coverage: Excellent - 23 tests covering unit, integration, and edge cases Recommendation: Request clarification on v2 alignment (Issue #2) and fixes for security issue (Issue #5), then approve after addressing feedback. Great work overall! The implementation is clean and well-thought-out. Once the strategic alignment is confirmed and the security concern is addressed, this will be a valuable addition to the codebase. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codeframe/ui/routers/tasks.py (1)
294-301:⚠️ Potential issue | 🔴 CriticalBug: Wrong variable used -
request.approvedshould bebody.approved.Line 294 accesses
request.approved, butrequestis the FastAPIRequestobject, not the Pydantic body model. This will cause anAttributeErrorat runtime sinceRequesthas noapprovedattribute.🐛 Fix the variable reference
# Check if user is rejecting - if not request.approved: + if not body.approved: return TaskApprovalResponse( success=False, phase=project.get("phase", "planning"),
🤖 Fix all issues with AI agents
In `@codeframe/config/rate_limits.py`:
- Around line 118-123: The current logger.info in the rate limit initialization
(referencing _rate_limit_config and logger.info) must not emit sensitive
credentials; ensure you do not log _rate_limit_config.redis_url or, if you need
to log the host, mask credentials first (e.g., strip or replace user:password@
from the URL) before including it in logger.info; audit any future additions
near the RateLimitConfig/_rate_limit_config initialization to either omit
redis_url entirely or use a sanitized value when constructing the log message.
In `@codeframe/lib/rate_limiter.py`:
- Around line 48-52: The X-Forwarded-For handling in the get_client_ip (or
equivalent) function currently trusts request.headers["X-Forwarded-For"]
unconditionally (variable forwarded_for), enabling header spoofing; change this
to respect a configuration flag (e.g., TRUST_X_FORWARDED_FOR or
trust_x_forwarded_for) so the code only parses forwarded_for when that flag is
true and the app is deployed behind a trusted proxy, otherwise ignore the header
and use the actual remote address (e.g., request.client.host or equivalent);
also add a short docstring or comment on the function noting the assumption and
how to disable header trust for direct deployments.
- Around line 175-197: The decorator currently reads get_rate_limiter() and
get_rate_limit_config() at decoration time inside decorator()/wrapper(), causing
config to be frozen at import; fix this by moving retrieval of limiter, config,
and limit_value (get_rate_limiter(), get_rate_limit_config(), and
getattr(config, limit_key, ...)) into the inner async wrapped(*args, **kwargs)
so the limit is evaluated at request time and then apply limiter.limit(...)
there (or if you prefer keeping decoration-time semantics, update tests to call
reset_rate_limiter() to clear the cached _limiter before re-importing);
reference decorator(), wrapped(), get_rate_limiter(), get_rate_limit_config(),
limit_key, _limiter, and reset_rate_limiter().
- Around line 143-146: The Retry-After header is currently set to exc.detail (a
human-readable string) which violates RFC 7231; update the headers construction
in the rate limit handler (where headers is built, and exc is the
RateLimitExceeded exception) to use a numeric delay-seconds value instead—either
a fixed string like "60" or, if the exception exposes a retry delay (e.g.,
exc.retry_after or exc.reset_time), convert that to an integer string and use it
for "Retry-After" so the header contains a numeric value.
In `@tests/lib/test_rate_limiter.py`:
- Around line 249-258: The test currently contains a tautological assertion;
replace it with a meaningful check: after calling _reset_rate_limit_config() and
get_rate_limiter(), assert that either limiter is None OR (if limiter is not
None) that its no-op behavior is verifiable by exercising its public
rate-checking API (e.g., call limiter.allow("test") or limiter.acquire("test")
as appropriate) and assert the call returns True / does not block / does not
raise and does not decrement any real quota; keep references to get_rate_limiter
and _reset_rate_limit_config so the test verifies the disabled case
deterministically.
🧹 Nitpick comments (4)
tests/config/test_rate_limits.py (1)
102-111: Consider adding a test for invalid storage values.The test validates that
"memory"and"redis"are accepted, but doesn't verify behavior when an invalid value is provided. Based on the relevant code snippet,RateLimitConfig.from_environment()logs a warning and falls back to"memory"for invalid storage values.🧪 Suggested test for invalid storage handling
def test_storage_validation_invalid_fallback(self): """Invalid storage should fall back to memory with warning.""" from codeframe.config.rate_limits import RateLimitConfig with patch.dict(os.environ, {"RATE_LIMIT_STORAGE": "invalid"}, clear=True): config = RateLimitConfig.from_environment() # Should fall back to memory assert config.storage == "memory"codeframe/ui/server.py (1)
212-224: Duplicate rate limiter initialization.The rate limiter is initialized twice:
- At module level (lines 263-268) - runs at import time
- In lifespan (lines 212-222) - runs when app starts
The module-level initialization is necessary for middleware setup, but the lifespan block is redundant. This causes duplicate log messages and unnecessary work.
♻️ Suggested cleanup
Remove the redundant initialization in lifespan since the module-level code already handles it:
# Log that authentication is now always required logger.info("🔒 Authentication: ENABLED (always required)") - # Initialize rate limiting - rate_limit_config = get_rate_limit_config() - if rate_limit_config.enabled: - limiter = get_rate_limiter() - if limiter: - app.state.limiter = limiter - logger.info( - f"🚦 Rate limiting: ENABLED " - f"(storage={rate_limit_config.storage}, " - f"standard={rate_limit_config.standard_limit})" - ) - else: - logger.info("🚦 Rate limiting: DISABLED") + # Log rate limiting status (already initialized at module level) + rate_limit_config = get_rate_limit_config() + if rate_limit_config.enabled: + logger.info( + f"🚦 Rate limiting: ENABLED " + f"(storage={rate_limit_config.storage}, " + f"standard={rate_limit_config.standard_limit})" + ) + else: + logger.info("🚦 Rate limiting: DISABLED") # Start background session cleanup taskAlso applies to: 263-268
codeframe/config/rate_limits.py (1)
69-72: Consider validating rate limit format strings.The rate limit strings (e.g., "10/minute") are passed directly from environment variables without format validation. Invalid formats like "abc/xyz" will cause runtime errors in slowapi.
This is optional since slowapi provides clear error messages, but early validation would fail fast with a helpful message:
import re def _validate_rate_limit(value: str, name: str) -> str: """Validate rate limit format (e.g., '10/minute').""" if not re.match(r'^\d+/(second|minute|hour|day)$', value): logger.warning(f"Invalid {name}: {value}. Using default.") return None # Return None to use dataclass default return valuetests/lib/test_rate_limiter.py (1)
144-171: Test fixture should reset both config and limiter.The
app_with_low_limitfixture resets the config but doesn't reset the global_limiter. This could cause test interference since the limiter is a global singleton.♻️ Add limiter reset to fixture
`@pytest.fixture` def app_with_low_limit(self): """Create a test app with very low rate limit for testing.""" from codeframe.lib.rate_limiter import ( rate_limit_exceeded_handler, + reset_rate_limiter, ) from codeframe.config.rate_limits import _reset_rate_limit_config from slowapi import Limiter from slowapi.util import get_remote_address from slowapi.errors import RateLimitExceeded # Reset config to apply new limits _reset_rate_limit_config() + reset_rate_limiter()
| # Build response headers | ||
| headers = { | ||
| "Retry-After": str(exc.detail) if hasattr(exc, "detail") else "60", | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
slowapi RateLimitExceeded exception detail attribute format
💡 Result:
In slowapi, RateLimitExceeded.detail is a plain string (not a dict / Pydantic model).
- The built-in
_rate_limit_exceeded_handlerformats its response using an f-string likef"Rate limit exceeded: {exc.detail}", soexc.detailis expected to be string-like. [1] - That string ultimately comes from the limit’s configured
error_message(which can be a static string or a callable that returns a string). [2]
So in practice you should treat exc.detail as “the human-readable message for the limit that was hit” (whatever you configured via error_message, or the library’s default message). [1], [2]
Sources: [1] [2]
🏁 Script executed:
# Search for the rate_limiter.py file and its context
fd "rate_limiter.py" -type fRepository: frankbria/codeframe
Length of output: 233
🏁 Script executed:
# Look for slowapi imports and RateLimitExceeded usage in the codebase
rg "RateLimitExceeded|from slowapi" -A 3 -B 1Repository: frankbria/codeframe
Length of output: 4056
🏁 Script executed:
# Check the specific file to understand the full context around lines 143-146
fd "rate_limiter.py" -type f -exec wc -l {} \;Repository: frankbria/codeframe
Length of output: 233
🏁 Script executed:
# Get the rate_limiter.py file and view the full context around lines 143-146
cat -n codeframe/lib/rate_limiter.py | sed -n '130,160p'Repository: frankbria/codeframe
Length of output: 1247
🏁 Script executed:
# Check if there are any other references to how exc.detail is used or what it contains
rg "exc\.detail" -B 2 -A 2Repository: frankbria/codeframe
Length of output: 340
Use numeric value for Retry-After header instead of exc.detail.
The Retry-After header at line 145 incorrectly uses exc.detail, which slowapi populates with a human-readable rate limit message (e.g., "10 per 1 minute"), not a numeric retry delay. RFC 7231 requires Retry-After to be either delay-seconds (integer) or an HTTP date. The fallback "60" is unreachable since RateLimitExceeded always has a detail attribute.
Replace with a fixed numeric value like "60" or extract the actual retry delay from the exception if available.
🤖 Prompt for AI Agents
In `@codeframe/lib/rate_limiter.py` around lines 143 - 146, The Retry-After header
is currently set to exc.detail (a human-readable string) which violates RFC
7231; update the headers construction in the rate limit handler (where headers
is built, and exc is the RateLimitExceeded exception) to use a numeric
delay-seconds value instead—either a fixed string like "60" or, if the exception
exposes a retry delay (e.g., exc.retry_after or exc.reset_time), convert that to
an integer string and use it for "Retry-After" so the header contains a numeric
value.
The rate limiting decorators require a `request: Request` parameter, which changed function signatures. This commit: - Fixes tasks.py to use `body.approved` instead of `request.approved` - Adds mock_request fixture to test files that call endpoints directly - Updates all direct endpoint calls to pass mock_request as first arg - Updates TaskApprovalRequest parameter from `request=` to `body=` - Updates function signature tests to expect `request` parameter This resolves the 30 failing tests caused by the rate limiting changes.
Code Review - PR #327: API Rate LimitingThank you for this comprehensive rate limiting implementation! The overall architecture is solid and follows good patterns. Here's my detailed analysis: 🟢 Strengths
🔴 Critical Issues (Must Fix Before Merge)1. Dead Code in Rate Limiter DecoratorFile: The def wrapper(func: Callable) -> Callable:
@wraps(func)
async def wrapped(*args, **kwargs): # ← Dead code
return await func(*args, **kwargs)
limiter = get_rate_limiter()
# ... code that returns limited_func, not wrapped
return limited_func # ← wrapped() is never calledFix: Simplify the decorator to remove unused code: def wrapper(func: Callable) -> Callable:
limiter = get_rate_limiter()
config = get_rate_limit_config()
if limiter is None or not config.enabled:
return func
limit_value = getattr(config, limit_key, "100/minute")
return limiter.limit(limit_value)(func)2. Tautological Test AssertionFile: assert limiter is None or limiter is not None # Always True!This test provides no value. Fix: Actually test that disabled rate limiting allows unlimited requests: # Test that limiter is None when disabled
config = get_rate_limit_config()
assert config.enabled is False
limiter = get_rate_limiter()
assert limiter is None🟡 High Priority Issues (Should Fix Soon)3. Thread-Safety in Config SingletonFile: Race condition possible during initialization: _rate_limit_config: Optional[RateLimitConfig] = None
def get_rate_limit_config() -> RateLimitConfig:
global _rate_limit_config
if _rate_limit_config is None: # ← Not thread-safe
_rate_limit_config = RateLimitConfig.from_environment()Fix: Use from functools import lru_cache
@lru_cache(maxsize=1)
def get_rate_limit_config() -> RateLimitConfig:
return RateLimitConfig.from_environment()4. User ID Null HandlingFile: If if user and hasattr(user, "id"):
return f"user:{user.id}" # ← What if user.id is None?Fix: Add null check: if user and hasattr(user, "id") and user.id:
return f"user:{user.id}"5. Redis Import Error Silently CaughtFile: ImportError for Redis is caught but only logged as info: except ImportError:
logger.info("Redis not available") # ← Should be ERROR levelFix: Log at ERROR level so users know Redis storage is unavailable: except ImportError as e:
logger.error(f"Redis storage requested but redis module not available: {e}")🔵 Test Coverage GapsMissing Test Scenarios:
Example test to add: async def test_authenticated_user_rate_limiting():
"""Test that authenticated users are rate limited by user ID."""
limiter = RateLimiter()
# Mock authenticated request
request = MagicMock()
request.state.user.id = "user123"
key = limiter._get_rate_limit_key(request)
assert key == "user:user123"
# Test that multiple requests from same user are blocked
# ... implementation📊 Architecture AssessmentFollowing CLAUDE.md Guidelines?✅ YES - This is v1 legacy code (FastAPI server), not v2 core Note: Per CLAUDE.md, this PR enhances the optional FastAPI adapter layer, which is appropriate for v1 legacy maintenance. 🎯 Overall VerdictRating: 7.5/10 The architecture is sound and the implementation is production-ready after addressing the dead code and test assertion. The thread-safety and user ID handling issues are lower priority but should be addressed in a follow-up PR. 📝 Action ItemsBefore merge:
Post-merge (follow-up PR):
Great work on this feature! The rate limiting system will significantly improve API resilience. 🚀 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codeframe/ui/routers/tasks.py (1)
265-269:⚠️ Potential issue | 🟡 MinorDocstring mismatch for
approve_tasksparameters.
The Args section still describesrequestas the approval payload; that should bebodynow.✏️ Suggested docstring fix
- request: Approval request with approved flag and optional exclusions + request: FastAPI request (rate limiter context) + body: Approval request with approved flag and optional exclusions
🧹 Nitpick comments (1)
tests/ui/test_assign_pending_tasks.py (1)
17-26: Consider centralizing the mock Request fixture.
This fixture is duplicated in other UI test modules; a sharedconftest.pyfixture would reduce drift and simplify updates.
Critical fixes: - Remove dead code: unused wrapped() function in decorator - Fix tautological test assertion (always true) with meaningful test High priority fixes: - Use lru_cache for thread-safe config singleton - Add null check for user.id in rate limit key generation - Change Redis import error log from WARNING to ERROR level - Remove unused functools.wraps import
Code Review: API Rate Limiting ImplementationI have reviewed PR #327. Here is my detailed feedback on the rate limiting implementation. Strengths1. Solid Architecture
2. Comprehensive Key Extraction
3. Proper HTTP Semantics
4. Good Test Coverage
5. Configuration Flexibility
Issues and Concerns1. CRITICAL: Audit Logging Not ImplementedThe PR description claims "Added audit logging events for rate limit exceeded" but this is NOT implemented:
Recommendation: Either implement the audit logging integration in rate_limit_exceeded_handler(), remove the audit logging claim from the PR description, or remove the unused audit logging code from audit_logger.py 2. V2 Architecture ConcernsAccording to CLAUDE.md, the v2 architecture principle is core-first with server/UI as optional adapters. This PR adds rate limiting only to the FastAPI layer. While this makes sense for HTTP-based rate limiting, consider adding a comment clarifying this is an HTTP-specific concern and does not apply to CLI/core operations. 3. Potential Race Condition in Limiter Initializationserver.py lines 263-268 runs at module import time, but the lifespan function (lines 213-224) also initializes the limiter. This could cause duplicate initialization or the limiter being created before environment variables are loaded. Recommendation: Move all rate limiting initialization into the lifespan function after load_environment() 4. Decorator Pattern IssuesThe decorator factory in _create_rate_limit_decorator() gets the config at module import time when decorators are applied, not at runtime. If config changes (e.g., in tests), the decorators will not see the new values. Recommendation: Move config fetching to request time, or document that config is frozen at startup. 5. Missing Integration PointsThe PR description mentions applying rate limits to chat, projects, tasks, and agents routers, but:
Recommendation: Apply rate_limit_auth() to authentication endpoints and document which endpoints are intentionally NOT rate limited and why. 6. Security: IP Spoofing Riskget_client_ip() trusts X-Forwarded-For and X-Real-IP headers without validation. In a self-hosted deployment without a trusted proxy, clients can spoof these headers to bypass rate limits. Recommendation:
Suggestions1. Documentation Needed
2. Observability Improvements
3. Testing Gaps
SummaryThis is a solid foundation for rate limiting with good architecture and test coverage. However, there are several gaps between the PR description and implementation:
Recommended Actions:
Verdict:The code quality is good, but the PR feels incomplete. The claimed features should either be fully implemented or removed from the description. I recommend addressing the audit logging discrepancy and initialization issues before merging. Great work on the core implementation! The rate limiting logic itself is solid. Just needs some polish to match the description and handle edge cases. |
The PR description claimed audit logging was implemented but it wasn't actually being called. This commit integrates the audit logger into the rate_limit_exceeded_handler to log security-relevant events: - Imports AuditLogger and AuditEventType when database is available - Logs rate limit exceeded events with user_id, IP, endpoint, and metadata - Gracefully handles audit logging failures without affecting response - Consistent with existing audit logging patterns in the codebase
Code Review - PR #327: API Rate Limiting with slowapi🚨 Critical Architecture ConcernAccording to This PR invests significant effort in the v1 FastAPI layer which conflicts with the roadmap:
From
|
| Aspect | Assessment |
|---|---|
| Architecture fit | ❌ Conflicts with v2 roadmap |
| Code quality | ✅ Clean, well-documented |
| Security | ✅ Proper audit logging, key extraction |
| Test coverage | ✅ Comprehensive |
| Recommendation | Defer to Phase 2/3 or use infrastructure-level solution |
🤔 Questions for Author
- Is rate limiting a critical security requirement right now, or can it wait for Phase 2?
- Have you considered infrastructure-level rate limiting (nginx, Cloudflare, API Gateway)?
- If proceeding: How do you plan to integrate this with the upcoming v2 server redesign?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@codeframe/lib/rate_limiter.py`:
- Around line 97-99: The repeated "Rate limiting is disabled" info log in
get_rate_limiter() causes duplicate startup messages; change behavior so the
message is emitted only once or downgraded to debug. Modify get_rate_limiter
(and use get_rate_limit_config/_limiter as needed) to either call
logger.debug("Rate limiting is disabled") instead of logger.info, or add a
module-level boolean (e.g., _logged_disabled) and set it the first time you log
so subsequent calls skip logging; ensure you mark the variable global in
get_rate_limiter before updating it.
🧹 Nitpick comments (1)
codeframe/lib/rate_limiter.py (1)
93-117: Consider thread-safe initialization for multi-worker deployments.The global
_limiterinitialization inget_rate_limiter()has a potential race condition if multiple threads call it simultaneously before_limiteris set. While typical FastAPI async deployments run on a single thread per worker (making this a non-issue), if the app runs with threading or multiple workers access shared state, duplicateLimiterinstances could be created.Since slowapi's Limiter state lives in the storage backend (Redis/memory), this is unlikely to cause data corruption—just unnecessary object creation and duplicate log messages. This is acceptable for current use but worth noting for future consideration.
🔒 Thread-safe initialization pattern (optional)
import threading _limiter: Optional[Limiter] = None _limiter_lock = threading.Lock() def get_rate_limiter() -> Optional[Limiter]: global _limiter config = get_rate_limit_config() if not config.enabled: return None if _limiter is None: with _limiter_lock: if _limiter is None: # Double-check locking # ... initialization code ... return _limiter
Add module-level _logged_disabled flag to track if we've already logged the disabled message. This prevents duplicate INFO logs on every call to get_rate_limiter() when rate limiting is disabled. Also reset the flag in reset_rate_limiter() for test isolation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@codeframe/lib/rate_limiter.py`:
- Around line 108-120: The current Limiter initialization only catches
ImportError and doesn't handle runtime Redis failures; update the code that
constructs _limiter (the Limiter call using key_func=get_rate_limit_key and
storage_uri=config.redis_url) to enable SlowAPI's built-in fallback by passing
in_memory_fallback_enabled=True and optionally swallow_errors=True, and consider
adding storage_options (e.g., short socket_connect_timeout/socket_timeout) so
Redis connection problems automatically fall back to in-memory and limiter
errors are gracefully suppressed.
| if config.storage == "redis" and config.redis_url: | ||
| try: | ||
| _limiter = Limiter( | ||
| key_func=get_rate_limit_key, | ||
| storage_uri=config.redis_url, | ||
| ) | ||
| logger.info("Rate limiter initialized with Redis storage") | ||
| except ImportError as e: | ||
| logger.error(f"Redis storage requested but redis module not available: {e}. Falling back to memory.") | ||
| _limiter = Limiter(key_func=get_rate_limit_key) | ||
| else: | ||
| _limiter = Limiter(key_func=get_rate_limit_key) | ||
| logger.info("Rate limiter initialized with in-memory storage") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual file to confirm the context
find . -name "rate_limiter.py" -type fRepository: frankbria/codeframe
Length of output: 94
🏁 Script executed:
# Let's also search for imports and usage of slowapi/Limiter
rg -i "from slowapi|import.*limiter|slowapi" --type py -A 2 -B 2Repository: frankbria/codeframe
Length of output: 5149
🌐 Web query:
slowapi Limiter Redis storage connection error handling
💡 Result:
To handle Redis connection problems when using SlowAPI’s Limiter with Redis storage, you generally want to (1) fail open with a controlled fallback (so your API doesn’t 500 when Redis blips) and (2) set sane Redis timeouts/pooling so failures happen fast and recover cleanly.
1) Enable SlowAPI’s built-in in-memory fallback (recommended)
SlowAPI’s Limiter supports automatically falling back to in-memory storage if the primary backend (Redis) is down via in_memory_fallback_enabled=True, and you can optionally define separate fallback limits with in_memory_fallback=[...]. This is explicitly part of the Limiter API. [1]
from slowapi import Limiter
from slowapi.util import get_remote_address
limiter = Limiter(
key_func=get_remote_address,
storage_uri="redis://redis:6379/0",
# fall back to in-memory when Redis is unavailable
in_memory_fallback_enabled=True,
# optional: different (usually more permissive) limits while degraded
in_memory_fallback=["60/minute"],
)This keeps requests flowing during Redis outages (with the tradeoff that limits won’t be shared across instances while degraded).
2) Decide how you want errors treated: swallow vs fail
SlowAPI exposes swallow_errors on the Limiter. If enabled, storage/ratelimit errors are swallowed (logged) instead of bubbling up as request failures. [1]
limiter = Limiter(
key_func=get_remote_address,
storage_uri="redis://redis:6379/0",
in_memory_fallback_enabled=True,
swallow_errors=True, # “fail open” if limiter/storage errors occur
)3) Set Redis client timeouts/pool limits via storage_options
SlowAPI passes storage_options through to the underlying storage implementation. [1] Use this to ensure Redis connection failures are fast (connect timeout, socket timeout) and to control pooling (max connections), depending on your redis client/version.
limiter = Limiter(
key_func=get_remote_address,
storage_uri="redis://redis:6379/0",
storage_options={
"socket_connect_timeout": 1,
"socket_timeout": 1,
# optionally:
# "max_connections": 50,
},
in_memory_fallback_enabled=True,
)4) What to catch if you implement your own handling
If you’re interacting with Redis directly (or diagnosing failures), Redis documents that connection failures show up as ConnectionError / TimeoutError / AuthenticationError and are typically recoverable with retry/backoff or fallback logic. [2]
Sources: SlowAPI Limiter parameters (storage_options, swallow_errors, in_memory_fallback, in_memory_fallback_enabled). [1] Redis error-handling guidance for connection errors and fallback/retry patterns. [2]
🏁 Script executed:
# Let's examine the actual rate_limiter.py file, especially around lines 108-120
cat -n ./codeframe/lib/rate_limiter.pyRepository: frankbria/codeframe
Length of output: 10270
Use SlowAPI's built-in fallback mechanisms for Redis connection resilience.
The code catches ImportError when the Redis module is unavailable, but runtime connection failures (e.g., Redis server unreachable, timeouts) are not handled. These errors would occur during limit checks, not during Limiter initialization, and could cause request failures if Redis becomes unavailable.
SlowAPI's Limiter provides explicit fallback support for this scenario. Configure it with in_memory_fallback_enabled=True to automatically fall back to in-memory storage on connection failures, and optionally swallow_errors=True to gracefully suppress limiter errors:
Example configuration
_limiter = Limiter(
key_func=get_rate_limit_key,
storage_uri=config.redis_url,
in_memory_fallback_enabled=True,
swallow_errors=True,
)You can also set connection timeouts via storage_options to ensure failures occur quickly:
_limiter = Limiter(
key_func=get_rate_limit_key,
storage_uri=config.redis_url,
storage_options={"socket_connect_timeout": 1, "socket_timeout": 1},
in_memory_fallback_enabled=True,
)🤖 Prompt for AI Agents
In `@codeframe/lib/rate_limiter.py` around lines 108 - 120, The current Limiter
initialization only catches ImportError and doesn't handle runtime Redis
failures; update the code that constructs _limiter (the Limiter call using
key_func=get_rate_limit_key and storage_uri=config.redis_url) to enable
SlowAPI's built-in fallback by passing in_memory_fallback_enabled=True and
optionally swallow_errors=True, and consider adding storage_options (e.g., short
socket_connect_timeout/socket_timeout) so Redis connection problems
automatically fall back to in-memory and limiter errors are gracefully
suppressed.
Apply rate limiting decorators to all 16 v2 API routers: - @rate_limit_standard() (100/min) for CRUD operations - @rate_limit_ai() (20/min) for AI-heavy operations (diagnose, discovery, execute) Renamed body request parameters from 'request' to 'body' to avoid conflict with FastAPI's Request object required by rate limiting.
Code Review - Rate Limiting ImplementationThis PR adds comprehensive API rate limiting to the FastAPI server layer. Overall, this is a well-implemented, production-ready feature with good test coverage and proper architectural alignment. ✅ Strengths
🔍 Issues & Recommendations1. CRITICAL: Rate limiting only applies to legacy v1 routesLooking at the Strategic Roadmap ( Problem: The v2 API routes ( Recommendation:
2. Decorator pattern adds implicit
|
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 (16)
codeframe/ui/routers/gates_v2.py (1)
104-105:⚠️ Potential issue | 🟡 MinorStale docstring references old parameter name.
The docstring still refers to
requestbut the parameter was renamed tobody. Update for accuracy.📝 Proposed fix
Args: - request: Gate run options + body: Gate run options (RunGatesRequest) workspace: v2 Workspacecodeframe/ui/routers/blockers_v2.py (2)
194-202:⚠️ Potential issue | 🟡 MinorDocstring parameter name is outdated.
The docstring refers to
request: Blocker creation requestbut the parameter was renamed tobody. Update for consistency.📝 Proposed fix
"""Create a new blocker. Args: - request: Blocker creation request + body: Blocker creation request workspace: v2 Workspace Returns: Created blocker """
227-244:⚠️ Potential issue | 🟡 MinorDocstring parameter name is outdated.
The docstring refers to
request: Answer requestbut the parameter was renamed tobody. Update for consistency.📝 Proposed fix
"""Answer a blocker. Answering a blocker also resets the associated task to READY status, so it can be restarted with `cf work start <task-id> --execute`. Args: blocker_id: Blocker to answer (can be partial ID) - request: Answer request + body: Answer request workspace: v2 Workspace Returns: Updated blockercodeframe/ui/routers/checkpoints_v2.py (1)
80-99:⚠️ Potential issue | 🟡 MinorDocstring parameter name mismatch.
Line 94 documents
request: Checkpoint creation requestbut the parameter was renamed tobody. Update the docstring to match.📝 Proposed fix
Args: - request: Checkpoint creation request + body: Checkpoint creation request workspace: v2 Workspacecodeframe/ui/routers/discovery_v2.py (2)
206-209:⚠️ Potential issue | 🟡 MinorDocstring references outdated parameter name.
The docstring still refers to
requestbut the parameter was renamed tobodyto avoid conflict with the FastAPIRequestobject.📝 Proposed fix
Args: session_id: Discovery session ID - request: Answer request with answer text + body: Answer request with answer text workspace: v2 Workspace
252-255:⚠️ Potential issue | 🟡 MinorDocstring references outdated parameter name.
The docstring still refers to
requestbut the parameter was renamed tobody.📝 Proposed fix
Args: session_id: Discovery session ID (must be complete) - request: Optional template selection + body: Optional template selection workspace: v2 Workspacecodeframe/ui/routers/batches_v2.py (2)
177-208:⚠️ Potential issue | 🟡 MinorDocstring references outdated parameter name.
The docstring at line 197 still documents
request: Stop optionsbut the parameter was renamed tobody. This should be updated for consistency.📝 Proposed fix for docstring
Args: batch_id: Batch to stop - request: Stop options + body: Stop options workspace: v2 Workspace
227-250:⚠️ Potential issue | 🟡 MinorDocstring references outdated parameter name.
Same issue as
stop_batch- the docstring at line 239 documentsrequest: Resume optionsbut the parameter is nowbody.📝 Proposed fix for docstring
Args: batch_id: Batch to resume - request: Resume options + body: Resume options workspace: v2 Workspacecodeframe/ui/routers/prd_v2.py (1)
244-249:⚠️ Potential issue | 🟠 MajorAvoid leaking internal error details in 500 responses.
Returningstr(e)exposes internal state to clients. Keep details in logs and return a generic error message instead.🔧 Suggested fix
- raise HTTPException( - status_code=500, - detail=api_error("Failed to create PRD", ErrorCodes.EXECUTION_FAILED, str(e)), - ) + raise HTTPException( + status_code=500, + detail=api_error( + "Failed to create PRD", + ErrorCodes.EXECUTION_FAILED, + "An unexpected error occurred", + ), + )- raise HTTPException( - status_code=500, - detail=api_error("Failed to create version", ErrorCodes.EXECUTION_FAILED, str(e)), - ) + raise HTTPException( + status_code=500, + detail=api_error( + "Failed to create version", + ErrorCodes.EXECUTION_FAILED, + "An unexpected error occurred", + ), + )Also applies to: 376-381
codeframe/ui/routers/templates_v2.py (1)
195-219:⚠️ Potential issue | 🟡 MinorUpdate apply_template docstring to match new parameters.
The docstring still describes
requestas the payload and omitsbody. This can mislead readers now that the payload is inbody.✏️ Proposed docstring fix
- Args: - request: Template application request - workspace: v2 Workspace + Args: + request: FastAPI Request (needed for rate limiting) + body: Template application request + workspace: v2 Workspacecodeframe/ui/routers/git_v2.py (1)
163-176:⚠️ Potential issue | 🟡 MinorFix the
create_commitdocstring parameter description.
The docstring still describesrequestas the commit payload; it should documentbodyas theCommitRequestandrequestas the FastAPIRequest.✏️ Suggested docstring update
Args: - request: CommitRequest with files and message + request: FastAPI Request (required for rate limiting) + body: CommitRequest with files and message workspace: v2 WorkspaceAlso applies to: 182-186
codeframe/ui/routers/projects_v2.py (1)
79-93:⚠️ Potential issue | 🟡 MinorUpdate docstrings to include the new
requestparameter.The signature includes
request, but the Args section still lists onlyworkspace.✏️ Suggested docstring update
Args: + request: Request for the current HTTP call. workspace: v2 Workspacecodeframe/ui/routers/workspace_v2.py (1)
224-252:⚠️ Potential issue | 🟡 MinorAllow explicit null to clear
tech_stack.The current code
if body.tech_stack is not None:prevents callingupdate_workspace_tech_stackwhen a client explicitly sendsnull, making it impossible to clear the tech stack. The function signature explicitly documents support for None to clear the value (tech_stack: Optional[str]with comment "or None to clear"), but the conditional blocks this. Usemodel_dump(exclude_unset=True)to distinguish between "field not provided" and "field provided as null":Suggested fix (Pydantic v2)
- if body.tech_stack is not None: - updated = ws.update_workspace_tech_stack(workspace.repo_path, body.tech_stack) + data = body.model_dump(exclude_unset=True) + if "tech_stack" in data: + updated = ws.update_workspace_tech_stack(workspace.repo_path, data.get("tech_stack"))codeframe/ui/routers/pr_v2.py (1)
211-231:⚠️ Potential issue | 🟡 MinorUpdate Args docs to reflect the
bodypayload rename.Both create and merge docstrings still describe the payload as
request, which now refers to a different parameter. This makes the endpoint docs misleading.📝 Suggested docstring updates
@@ - Args: - request: PR creation request - workspace: v2 Workspace (for context) + Args: + request: Request context (rate limiting) + body: PR creation request payload + workspace: v2 Workspace (for context) @@ - Args: - pr_number: PR number to merge - request: Merge options - workspace: v2 Workspace (for context) + Args: + pr_number: PR number to merge + request: Request context (rate limiting) + body: Merge options + workspace: v2 Workspace (for context)Also applies to: 253-269
codeframe/ui/routers/tasks_v2.py (2)
235-288:⚠️ Potential issue | 🟡 MinorReturn 404 when status updates target a missing task.
tasks.update_statuscan raise a not-found error, but the inner handler currently maps it to “Invalid status transition” (400). That makes status-only updates on missing tasks return the wrong status code.🔧 Proposed fix
- except ValueError as e: - if "Invalid status" in str(e) or "not a valid" in str(e).lower(): + except ValueError as e: + error_msg = str(e) + if "not found" in error_msg.lower(): + raise HTTPException( + status_code=404, + detail=api_error("Task not found", ErrorCodes.NOT_FOUND, error_msg), + ) + if "Invalid status" in error_msg or "not a valid" in error_msg.lower(): raise HTTPException( status_code=400, detail=api_error( f"Invalid status: {body.status}", ErrorCodes.VALIDATION_ERROR, f"Valid values: {[s.value for s in TaskStatus]}", ), ) # Status transition error raise HTTPException( status_code=400, - detail=api_error("Invalid status transition", ErrorCodes.INVALID_STATE, str(e)), + detail=api_error("Invalid status transition", ErrorCodes.INVALID_STATE, error_msg), )
465-526:⚠️ Potential issue | 🟠 MajorAvoid treating an explicit empty task list as “run all”.
Using
body.task_ids or ...treats[]as falsy and falls back to READY tasks. If a client passes an empty list (e.g., filtered out), this would start tasks unexpectedly. Prefer checking forNoneso an empty list results in the existing “No tasks to execute” error.🔧 Proposed fix
- task_ids = body.task_ids or runtime.get_ready_task_ids(workspace) + task_ids = ( + runtime.get_ready_task_ids(workspace) + if body.task_ids is None + else body.task_ids + )
🤖 Fix all issues with AI agents
In `@codeframe/ui/routers/diagnose_v2.py`:
- Around line 106-112: The diagnose_task endpoint's docstring is out of date:
the function diagnose_task now accepts a FastAPI Request parameter and a
separate body parameter of type DiagnoseRequest, but the docstring still claims
request contains diagnosis options and omits body; update the docstring for
diagnose_task to describe both parameters (request: FastAPI Request for
HTTP/context info and body: DiagnoseRequest containing diagnosis
options/payload) and adjust any wording that references request as the sole
source of options; apply the same docstring correction to the other
diagnose_task variant/duplicate in this module so both reflect the new
signature.
- Around line 181-185: The docstring for get_diagnostic_report lacks
documentation for the new request: Request parameter; update the function's
docstring Args section to include a line for request (type Request) describing
that it carries the incoming HTTP request/context (used by rate
limiting/middleware or to access headers/session), and ensure the parameter list
and description match the signature of get_diagnostic_report so docs and static
analyzers stay consistent.
🧹 Nitpick comments (2)
codeframe/ui/routers/environment_v2.py (1)
125-127: Docstrings are now slightly out of sync with the new parameters.The Args sections don’t mention
request: Request, andinstall_toolstill describes the tool name as coming fromrequestinstead ofbody. Consider updating for accuracy and clarity.Also applies to: 156-158, 194-196
codeframe/ui/routers/discovery_v2.py (1)
244-244: Consider explicitOptionaltype hint for clarity.While
body: GeneratePrdRequest = Noneworks, addingOptionalmakes the nullable intent clearer to readers and static analysis tools.📝 Suggested improvement
- body: GeneratePrdRequest = None, + body: Optional[GeneratePrdRequest] = None,
Update Args sections in v2 router endpoint docstrings: - Add 'request: HTTP request for rate limiting' documentation - Rename 'request' body references to 'body' for consistency
Code Review: Rate Limiting ImplementationThank you for this comprehensive rate limiting implementation! The code quality is excellent with strong test coverage. However, I've identified some critical architectural concerns and security issues that need to be addressed. 🚨 Critical Issues1. Architecture Misalignment with v2 RoadmapSeverity: CRITICAL This PR implements server-side rate limiting, which is Phase 2 work according to our v2 roadmap. Per
From
Recommendation:
Reference: 2. Duplicate Configuration SystemsSeverity: IMPORTANT You've created Existing code ( # Rate Limiting Configuration
rate_limit_enabled: bool = Field(True, alias="RATE_LIMIT_ENABLED")
rate_limit_storage: str = Field("memory", alias="RATE_LIMIT_STORAGE")
redis_url: Optional[str] = Field(None, alias="REDIS_URL")New code ( enabled_str = os.getenv("RATE_LIMIT_ENABLED", "true")
storage = os.getenv("RATE_LIMIT_STORAGE", "memory")
redis_url = os.getenv("REDIS_URL")Problems:
Recommendation:
|
| Category | Score | Notes |
|---|---|---|
| Code Quality | 8/10 | Excellent tests, clean structure |
| Security | 6/10 | Good baseline, needs hardening |
| Architecture | 2/10 | Violates v2 CLI-first principles |
| Test Coverage | 8/10 | Comprehensive, missing security tests |
Overall: NEEDS REVISION
This is high-quality code that will be valuable for Phase 2, but it needs:
- Proper roadmap alignment (Phase 2 target)
- Security hardening (X-Forwarded-For)
- Configuration consolidation
Once these issues are addressed, this will be an excellent addition to the Phase 2 server layer! 🚀
Files Reviewed: 25+ files, ~1000 lines of changes
- New:
config/rate_limits.py,lib/rate_limiter.py, test files - Modified: 20 routers,
audit_logger.py,.env.example,server.py
cc: Per CLAUDE.md guidelines and v2 architectural principles
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
codeframe/ui/routers/gates_v2.py (2)
119-124:⚠️ Potential issue | 🟠 MajorAvoid returning raw exception messages to clients.
str(e)can expose internal paths/commands or sensitive error details. Log the exception, but return a redacted/constant message in the API payload.Suggested fix
except Exception as e: logger.error(f"Failed to run gates: {e}", exc_info=True) raise HTTPException( status_code=500, - detail=api_error("Gate execution failed", ErrorCodes.EXECUTION_FAILED, str(e)), + detail=api_error( + "Gate execution failed", + ErrorCodes.EXECUTION_FAILED, + "Internal error. See server logs for details.", + ), )
133-139:⚠️ Potential issue | 🟡 MinorDocstring missing
requestargument.The endpoint now accepts
request: Request, but the Args list omits it.Suggested fix
Args: + request: HTTP request for rate limiting workspace: v2 Workspacecodeframe/ui/routers/workspace_v2.py (3)
176-189:⚠️ Potential issue | 🟡 MinorAllow explicit empty tech_stack updates for existing workspaces.
Line 187 usesif tech_stack:which skips empty strings, so an explicit empty value can’t be applied (unlikeupdate_current_workspace, which usesis not None). Consider allowing empty strings here as well.🛠️ Suggested fix
- if tech_stack: - workspace = ws.update_workspace_tech_stack(repo_path, tech_stack) + if tech_stack is not None: + workspace = ws.update_workspace_tech_stack(repo_path, tech_stack)
204-217:⚠️ Potential issue | 🟡 MinorDocstring missing the new
requestparameter.
get_current_workspacenow acceptsrequest: Requestbut the Args list doesn’t mention it.📝 Suggested doc update
Args: + request: HTTP request for rate limiting workspace: v2 Workspace (resolved by dependency)
263-283:⚠️ Potential issue | 🟡 MinorAvoid double
workspace_existscalls.
ws.workspace_exists(path)is called twice, which adds extra I/O and allows a small TOCTOU window.♻️ Suggested refactor
- return { - "exists": ws.workspace_exists(path), - "path": str(path), - "state_dir": str(path / ".codeframe") if ws.workspace_exists(path) else None, - } + exists = ws.workspace_exists(path) + return { + "exists": exists, + "path": str(path), + "state_dir": str(path / ".codeframe") if exists else None, + }codeframe/ui/routers/projects_v2.py (1)
129-141:⚠️ Potential issue | 🟡 MinorUpdate docstrings to include the new
requestparameter.
These endpoints now acceptrequest: Request, but their Args sections omit it, which makes the docs inconsistent.Also applies to: 160-171, 192-206, 227-239
codeframe/ui/routers/tasks_v2.py (1)
466-529:⚠️ Potential issue | 🟠 MajorAvoid executing all READY tasks when an empty list is supplied.
body.task_ids or ...treats[]as falsy, so an explicit empty list will execute all READY tasks instead of triggering the “No tasks to execute” error. Use an explicitNonecheck to preserve the default only when the field is omitted.🛠️ Proposed fix
- task_ids = body.task_ids or runtime.get_ready_task_ids(workspace) + task_ids = body.task_ids if body.task_ids is not None else runtime.get_ready_task_ids(workspace)
🧹 Nitpick comments (4)
codeframe/ui/routers/discovery_v2.py (1)
245-245: UseOptionaltype annotation for nullable body parameter.The type annotation
body: GeneratePrdRequest = Noneis inconsistent—the type saysGeneratePrdRequestbut the default isNone. This will cause type checker warnings and is not idiomatic FastAPI.♻️ Proposed fix
async def generate_prd( request: Request, session_id: str, - body: GeneratePrdRequest = None, + body: Optional[GeneratePrdRequest] = None, workspace: Workspace = Depends(get_v2_workspace), ) -> GeneratePrdResponse:codeframe/ui/routers/git_v2.py (1)
89-97: Minor docstring inconsistency (optional nit).The
create_commitendpoint documents therequestparameter in its docstring (lines 175-176), but the GET endpoints likeget_git_statusdon't. Consider adding a brief note about therequestparameter to other docstrings for consistency, or removing it fromcreate_commitsince it's a technical implementation detail for rate limiting rather than a meaningful API parameter.codeframe/ui/routers/prd_v2.py (1)
346-358: Docstring references outdated parameter name.The docstring at line 350 refers to
request: Version creation requestbut the parameter was renamed tobody. This should be updated for consistency."""Create a new version of a PRD. Args: prd_id: ID of the parent PRD - request: Version creation request + body: Version creation request workspace: v2 Workspace Returns:codeframe/ui/routers/batches_v2.py (1)
182-183: Make body params explicitly optional for typing clarity.Defaulting to
Noneis fine, but the type hints should reflect optionality to keep static typing and OpenAPI expectations aligned.♻️ Suggested tweak
- body: StopBatchRequest = None, + body: Optional[StopBatchRequest] = None, @@ - body: ResumeBatchRequest = None, + body: Optional[ResumeBatchRequest] = None,Also applies to: 233-234
Security fixes: - Add trusted proxy validation to prevent X-Forwarded-For spoofing - Only trust proxy headers when direct connection is from configured proxy - Make "unknown" IP rate limit keys unique to prevent shared bucket DoS - Add warning logs for proxy header spoofing attempts Configuration consolidation: - Add rate limit config fields to GlobalConfig (single source of truth) - Update RateLimitConfig.from_global_config() to delegate to GlobalConfig - Remove duplicate environment variable parsing - Add get_global_config() and reset_global_config() module-level functions Test improvements: - Add reset_caches fixture to all rate limiter test classes - Add TestTrustedProxyValidation with exact IP and CIDR matching tests - Add test_key_for_unknown_ip_is_unique for DoS mitigation - Update tests to use from_global_config() instead of from_environment()
Pull Request Review: Rate Limiting ImplementationI've completed a comprehensive review of the rate limiting implementation. Overall, this is well-architected and follows security best practices. The code quality is high with good test coverage. However, there are important architectural concerns regarding v1 vs v2 strategy. 🏗️ CRITICAL: V1 vs V2 Architecture AlignmentAccording to
The IssueThis PR adds significant functionality to the v1 server layer (
Questions for Discussion
RecommendationIf rate limiting is needed now:
If it can wait:
✅ Code Quality - ExcellentStrengths
🐛 Potential Issues1. Decorator Pattern Complexity (Medium Priority)Location: The def decorator():
def wrapper(func: Callable) -> Callable:
limiter = get_rate_limiter() # Called every time
config = get_rate_limit_config() # Cached, but still checked
if limiter is None or not config.enabled:
return func
limit_value = getattr(config, limit_key, "100/minute")
return limiter.limit(limit_value)(func)
return wrapperIssue: If rate limiting is disabled, the original function is returned without slowapi's decorator applied. This means:
Recommendation: def wrapper(func: Callable) -> Callable:
limiter = get_rate_limiter()
if limiter is None:
return func # OK - rate limiting globally disabled
limit_value = getattr(config, limit_key, "100/minute")
# Always apply the decorator, let slowapi handle the logic
return limiter.limit(limit_value)(func)2. Error Handling in Audit Logging (Low Priority)Location: The audit logging swallows all exceptions with a debug log: except Exception as e:
logger.debug(f"Failed to log rate limit event to audit log: {e}")Issue: This is correct behavior (don't fail the request), but Recommendation: Use 3. Missing Rate Limit Headers (Medium Priority)Location: The exception handler returns: headers = {
"Retry-After": str(exc.detail) if hasattr(exc, "detail") else "60",
}
if hasattr(exc, "limit"):
headers["X-RateLimit-Limit"] = str(exc.limit)Missing: Standard rate limit headers:
Recommendation: Add these headers for better client behavior. Clients can implement exponential backoff or smart retry logic. 4. Lack of Rate Limit Bypass for Tests (Low Priority)Location: N/A Issue: Integration tests will hit rate limits when running large test suites. Recommendation: Add a test fixture or environment variable ( # In conftest.py
@pytest.fixture(autouse=True)
def disable_rate_limiting_in_tests():
with patch.dict(os.environ, {"RATE_LIMIT_ENABLED": "false"}):
yield🚀 Performance Considerations1. In-Memory Storage for Single Instance ✅Good choice for default. In-memory is fast and sufficient for self-hosted single-instance deployments. 2. Redis for Distributed Deployments ✅Redis support is present but requires manual configuration. Consider documenting:
3. Cache Usage ✅
🔒 Security AssessmentStrengths ✅
Recommendations
📋 Test CoverageWhat's Tested ✅
What's Missing
RecommendationAdd integration test: def test_rate_limit_with_real_endpoint(client):
"""Test rate limiting on actual endpoint."""
# Make 11 requests to auth endpoint (limit: 10/minute)
for i in range(11):
response = client.post("/auth/login", json={...})
if i < 10:
assert response.status_code in [200, 401] # Normal response
else:
assert response.status_code == 429
assert "Retry-After" in response.headers
assert response.json()["error"] == "rate_limit_exceeded"📝 DocumentationStrengths ✅
Recommendations
🎯 SummaryMust Address Before Merge
Should Address Before Merge
Nice to Have (Future PRs)
🎉 ConclusionThis is high-quality, production-ready code with excellent security practices and test coverage. The main concern is architectural fit within the v1→v2 migration strategy. Once the v2 alignment question is resolved, this PR is very close to mergeable. Great work on the implementation! 👏 |
Integration tests: - Add TestRateLimitingIntegration class with 5 tests - Verify rate limiting allows requests within limit - Verify 429 response when limit exceeded - Verify Retry-After header and response body format - Test rate limiting on real v2 endpoint Decorator pattern fix: - Remove redundant config.enabled check (get_rate_limiter already returns None when disabled) - Simplify to: if limiter is None, return func; otherwise apply decorator - This ensures consistent function identity regardless of config state
| if client_ip == "unknown": | ||
| # Use a unique key per request for unknown IPs | ||
| # This effectively gives each "unknown" request its own bucket | ||
| # preventing the shared bucket DoS attack | ||
| request_id = id(request) | ||
| return f"ip:unknown:{request_id}" |
There was a problem hiding this comment.
🟡 Medium
codeframe/lib/rate_limiter.py:122
Using id(request) creates a unique bucket per request, which completely bypasses rate limiting for unknown IPs. Consider using a shared key like "ip:unknown" instead—a shared bucket is safer than no rate limiting at all.
| if client_ip == "unknown": | |
| # Use a unique key per request for unknown IPs | |
| # This effectively gives each "unknown" request its own bucket | |
| # preventing the shared bucket DoS attack | |
| request_id = id(request) | |
| return f"ip:unknown:{request_id}" | |
| if client_ip == "unknown": | |
| return "ip:unknown" |
🚀 Want me to fix this? Reply ex: "fix it for me".
Code Review - API Rate Limiting ImplementationI've reviewed PR #327. The implementation is well-structured with good security practices, but there are some important considerations: Strengths
Critical Issue: Architecture AlignmentAccording to CLAUDE.md, this codebase is in v2 refactor phase (CLI-first, headless). The stated principle is:
This PR adds significant server/UI functionality (2055 additions across 20+ router files), which appears to conflict with v2 architecture that states FastAPI is optional and CLI must not require a server. Question: Is this PR targeting v1 maintenance or v2? If v1, that's fine. If v2, rate limiting may need to wait for Phase 2 (Server Layer) per the strategic roadmap. Other Issues
SummaryStrong implementation overall. Main concern is architectural alignment with v2 direction. Please clarify target version. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/ui/test_v2_routers_integration.py`:
- Line 890: Remove the unused import "patch" from the unittest.mock import
statement in tests/ui/test_v2_routers_integration.py (the line currently reading
"from unittest.mock import patch"); locate any references to "patch" (none
expected) and delete only the unused symbol so the file still imports any other
needed names or nothing if not required, then run lint/tests to confirm F401 is
resolved.
🧹 Nitpick comments (2)
codeframe/lib/rate_limiter.py (1)
122-128: Clarify the security tradeoff for "unknown" IP handling.Using
id(request)as a unique key per request effectively disables rate limiting for requests with undeterminable IPs—each gets its own bucket. The comment explains this prevents shared-bucket DoS, but the tradeoff is that requests from misconfigured proxies or edge cases bypass rate limits entirely.Consider adding a comment documenting this tradeoff, or alternatively applying a very aggressive shared limit for unknown IPs (e.g.,
1/second) rather than no limit at all.💡 Alternative approach with aggressive shared limit
# Mark "unknown" IPs specially for potential stricter handling if client_ip == "unknown": - # Use a unique key per request for unknown IPs - # This effectively gives each "unknown" request its own bucket - # preventing the shared bucket DoS attack - request_id = id(request) - return f"ip:unknown:{request_id}" + # Use shared bucket for unknown IPs with aggressive limit + # This is a tradeoff: attackers could fill this bucket, but + # leaving unknown IPs unlimited is worse for DoS protection. + # If this causes issues, ensure trusted proxies are configured. + return "ip:unknown"This would require a separate, more restrictive limit for unknown IPs configured elsewhere.
tests/ui/test_v2_routers_integration.py (1)
926-929: Use a context manager withyieldin the fixture to ensure proper TestClient cleanup.TestClient leaves background threads and lifespan shutdown handlers running without proper cleanup. The fixture should use
with TestClient(app) as client:andyield clientto ensure deterministic cleanup when the test finishes. This pattern is already used elsewhere in the test suite and is a FastAPI testing best practice.♻️ Proposed refactor
- client = TestClient(app) - client.workspace = test_workspace - - return client + with TestClient(app) as client: + client.workspace = test_workspace + yield client
Code Review: API Rate Limiting ImplementationI've reviewed PR #327 which adds comprehensive API rate limiting using slowapi. Overall, this is a well-designed and thoroughly tested implementation with strong security considerations. Strengths
Critical Issues1. Redis Connection Handling (rate_limiter.py:152-160)Only catches ImportError when initializing Redis. Connection failures will crash the app instead of falling back to memory. Recommendation: Catch broader exceptions for graceful fallback. 2. Rate Limiter Initialization Race (server.py:263-268)Initialized twice - module level before env vars are loaded (line 263) and in lifespan (line 213). This causes configuration inconsistencies. Recommendation: Remove module-level init, move middleware addition into lifespan function. 3. Missing Rate Limit Format Validation (config.py:402-411)Invalid formats like '10/week' or 'abc/minute' only fail at runtime. Recommendation: Add Pydantic field validator for rate limit format strings. Medium-Priority
TestingComplete manual testing checklist:
SummarySolid production-ready implementation! Must fix issues #1 and #2 before merge. Great work on security and test coverage! |
- Update status badge to Phase 2 In Progress (4285 tests) - Add API key authentication section with CLI commands - Add rate limiting configuration documentation - Document new V2 API endpoints - Add Security & API section to Key Features - Update roadmap to show Phase 2 at 90% complete - Mark #322, #325, #326, #327 as complete - Update current focus to remaining items (WebSocket, OpenAPI, pagination)
1 participant
Summary
Add comprehensive API rate limiting to protect endpoints from abuse using the
slowapilibrary.Retry-Afterheader and JSON error bodyChanges
New Files
codeframe/config/rate_limits.py- Configuration with environment variable supportcodeframe/lib/rate_limiter.py- Middleware with slowapi integrationtests/config/test_rate_limits.py- 8 configuration teststests/lib/test_rate_limiter.py- 15 middleware testsModified Files
.env.examplewith rate limiting configurationConfiguration
RATE_LIMIT_ENABLED=true RATE_LIMIT_AUTH=10/minute RATE_LIMIT_STANDARD=100/minute RATE_LIMIT_AI=20/minute RATE_LIMIT_STORAGE=memory # or "redis"Test plan
Summary by CodeRabbit
New Features
Documentation
Audit
Tests