Implement Redis Caching for Agent API Calls #71
Conversation
…ng, and type hints.
WalkthroughAdds an async Redis-backed cache utility and integrates it into the code audit agent to cache serialized httpx responses for GitHub/GitLab calls; also includes FastAPI dependency adjustments, import cleanups, reduced package re-exports, and small test/import tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as CodeAuditAgent
participant Lim as RateLimiter
participant Cache as Redis / cache_request
participant API as External API (GitHub/GitLab)
Agent->>Lim: check_rate_limit(repo, endpoint)
Lim-->>Agent: allowed / denied
alt allowed
Agent->>Cache: cache_request(url, params, external_api_call)
note right of Cache: generate_cache_key(url, params)\nGET cached value
Cache-->>Agent: cached response (hit) / miss
alt cache miss
Cache->>API: external_api_call() (httpx request)
API-->>Cache: HTTP response
Cache->>Cache: serialize_httpx_response & SETEX(key, TTL)
Cache-->>Agent: response (deserialized)
end
else denied
Lim-->>Agent: rate limit error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/services/agents/code_audit_agent.py(1 hunks)backend/app/utils/cache_utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/services/agents/code_audit_agent.py (1)
backend/app/utils/cache_utils.py (1)
cache_request(15-30)
🪛 Ruff (0.14.6)
backend/app/utils/cache_utils.py
15-15: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/app/services/agents/code_audit_agent.py (1)
178-180: Same serialization issue applies to GitLab API calls.GitLab API calls (lines 178, 186, 194, 204, 212) have the same httpx.Response serialization problem described in the GitHub section. The
int(commits_resp.headers.get('x-total', 0))call at line 180 will fail on cache hits whencommits_respis a deserialized dict instead of a Response object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/app/api/v1/routes.py(3 hunks)backend/app/db/database.py(0 hunks)backend/app/db/migrations/env.py(1 hunks)backend/app/db/tests/test_connection.py(0 hunks)backend/app/services/agents/code_audit_agent.py(8 hunks)backend/app/services/agents/tests/test_code_audit_agent.py(1 hunks)backend/app/services/nlg/__init__.py(0 hunks)backend/app/services/nlg/tests/test_nlg_engine.py(0 hunks)backend/app/services/nlg/tests/test_report_nlg_engine.py(0 hunks)backend/app/services/report_processor.py(1 hunks)backend/app/services/summary/__init__.py(0 hunks)backend/app/services/summary/summary_engine.py(1 hunks)backend/app/services/validation/tests/test_validation_engine.py(0 hunks)backend/app/services/validation/validation_engine.py(0 hunks)backend/app/utils/cache_utils.py(1 hunks)backend/tests/test_orchestrator_config.py(1 hunks)
💤 Files with no reviewable changes (8)
- backend/app/services/nlg/tests/test_report_nlg_engine.py
- backend/app/db/tests/test_connection.py
- backend/app/services/validation/tests/test_validation_engine.py
- backend/app/services/validation/validation_engine.py
- backend/app/services/nlg/tests/test_nlg_engine.py
- backend/app/db/database.py
- backend/app/services/nlg/init.py
- backend/app/services/summary/init.py
🧰 Additional context used
🧬 Code graph analysis (4)
backend/app/services/agents/tests/test_code_audit_agent.py (1)
backend/app/services/agents/code_audit_agent.py (3)
CodeAuditAgent(46-368)CodeMetrics(25-33)AuditSummary(35-40)
backend/app/api/v1/routes.py (2)
backend/app/models/report_models.py (1)
ReportRequest(4-6)backend/app/db/database.py (1)
get_db(25-27)
backend/app/services/agents/code_audit_agent.py (2)
backend/app/utils/cache_utils.py (1)
cache_request(20-44)backend/app/security/rate_limiter.py (1)
check_rate_limit(29-47)
backend/app/services/report_processor.py (1)
backend/app/core/storage.py (1)
try_set_processing(20-32)
🪛 Ruff (0.14.6)
backend/app/api/v1/routes.py
33-33: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
42-42: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
52-52: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
backend/app/utils/cache_utils.py
30-30: Do not catch blind exception: Exception
(BLE001)
41-41: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (8)
backend/app/services/summary/summary_engine.py (1)
6-6: LGTM! Import cleanup.Removing the unused
Listimport is appropriate since it's not referenced in any method signatures or the file body.backend/tests/test_orchestrator_config.py (1)
111-112: LGTM! Test improvement.Adding explicit verification that
agents == {}strengthens the test by confirming the expected behavior (no agents registered) rather than relying solely on log message assertions.backend/app/services/report_processor.py (1)
7-7: LGTM! Import cleanup.Removing unused
save_report_dataandset_report_statusimports is appropriate. The file correctly retainstry_set_processing, which is used at line 30 for atomic status checking.backend/app/db/migrations/env.py (1)
1-5: LGTM! Import organization.Consolidating the SQLAlchemy imports and explicitly adding
osandsysat the top improves readability. These imports are used at line 20 for path manipulation.backend/app/api/v1/routes.py (1)
33-33: LGTM! Dependency injection standardization.The change to use
Depends(get_db)across all three endpoints is appropriate and follows standard FastAPI patterns. The static analysis warning (B008) is a false positive—usingDepends()in default parameter values is the idiomatic way to declare dependencies in FastAPI.Also applies to: 42-42, 52-52
backend/app/services/agents/tests/test_code_audit_agent.py (1)
5-5: LGTM! Import cleanup.Removing
CodeAuditResultfrom the test imports is appropriate since tests interact with the dict representation returned byfetch_data()(line 111), not the Pydantic model directly.backend/app/utils/cache_utils.py (2)
13-17: LGTM! Cache key generation is robust.The use of
json.dumps(params, sort_keys=True, default=str)properly handles nested dictionaries and lists, ensuring deterministic cache keys. This addresses the past review concern about sorted(params.items()) failing on complex structures.
26-31: LGTM! Appropriate error handling for cache layer.The broad exception handling at lines 30 and 41 is appropriate for a caching layer—Redis unavailability or serialization failures should degrade gracefully rather than break the application. The warnings are logged for observability. The static analysis BLE001 warnings are false positives in this context.
Also applies to: 35-42
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/app/services/agents/code_audit_agent.py (1)
22-47: MockResponse implementation is functional but has limitations.The MockResponse class duck-types httpx.Response for the specific methods used in this codebase (.raise_for_status(), .json(), .headers). This is a pragmatic solution for caching.
However, note:
- Lines 34-37: Returning
{}on JSONDecodeError silently masks invalid JSON responses. This might hide issues if the API returns non-JSON error pages.- Line 42: The dummy request URL
"http://mock.url/error"is a workaround. Consider making it more descriptive, e.g.,"http://cached-response/error".Regarding the static analysis hint (TRY003) for lines 43-45: The error message could be moved to a custom exception class, but given this is an internal mock, the current approach is acceptable for this use case.
backend/app/utils/cache_utils.py (2)
12-12: Consider making CACHE_TTL configurable via environment variable.The hardcoded 1-hour TTL is reasonable for development, but production deployments might need different values based on API rate limits and data freshness requirements.
Consider:
CACHE_TTL = int(os.getenv("REDIS_CACHE_TTL", "3600")) # Default 1 hour
37-47: Cache read error handling is well-designed.The function properly handles both Redis failures and deserialization errors, treating both as cache misses. This ensures graceful degradation when cache is unavailable.
However, consider the static analysis hint for line 43:
- logger.error(f"Failed to deserialize cached response for key {cache_key}: {e}") + logger.exception(f"Failed to deserialize cached response for key {cache_key}")Using
logger.exceptionautomatically includes the traceback, which is helpful for debugging deserialization issues without manually interpolating the exception.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
backend/app/cache/__pycache__/redis_client.cpython-313.pycis excluded by!**/*.pycbackend/app/core/__pycache__/config.cpython-313.pycis excluded by!**/*.pycbackend/app/security/__pycache__/__init__.cpython-313.pycis excluded by!**/*.pycbackend/app/security/__pycache__/rate_limiter.cpython-313.pycis excluded by!**/*.pycbackend/app/services/agents/__pycache__/code_audit_agent.cpython-313.pycis excluded by!**/*.pycbackend/app/services/agents/tests/__pycache__/test_code_audit_agent.cpython-313-pytest-8.2.0.pycis excluded by!**/*.pycbackend/app/utils/__pycache__/cache_utils.cpython-313.pycis excluded by!**/*.pyc
📒 Files selected for processing (2)
backend/app/services/agents/code_audit_agent.py(8 hunks)backend/app/utils/cache_utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/services/agents/code_audit_agent.py (3)
backend/app/utils/cache_utils.py (1)
cache_request(22-61)backend/tests/test_routes.py (1)
client(16-18)backend/app/security/rate_limiter.py (1)
check_rate_limit(29-47)
backend/app/utils/cache_utils.py (2)
backend/app/services/agents/code_audit_agent.py (1)
json(32-37)backend/app/cache/redis_client.py (2)
get_cache(51-63)set_cache(37-49)
🪛 Ruff (0.14.6)
backend/app/services/agents/code_audit_agent.py
43-45: Avoid specifying long messages outside the exception class
(TRY003)
backend/app/utils/cache_utils.py
43-43: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (4)
backend/app/services/agents/code_audit_agent.py (2)
14-20: LGTM: Serialization helper is well-designed.The function correctly extracts the essential attributes from httpx.Response (status code, headers, and text) for caching. Converting headers to a dict is appropriate for JSON serialization.
142-147: Verify that caching without Authorization header in cache key is safe.The cache key is generated from URL and params only (line 143), but doesn't include the Authorization header. This means:
- If tokens are service-level (from environment variables, same for all users): ✅ Safe - all requests share the same credentials.
- If tokens could be user-specific or rotated:
⚠️ Risk - different users/tokens would share cached responses, potentially leaking data or serving stale results.Since
github_tokenis fromos.getenv("GITHUB_TOKEN")(line 94), it appears to be service-level. However, verify that:
- Tokens will never become user-specific in the future
- Token rotation scenarios are handled (cached data from old token continues to be served until TTL expires)
Same consideration applies to all other cache_request calls (lines 156-161, 170-175, 187-192, 201-206 for GitHub; lines 239-244, 252-257, 265-270, 280-285, 293-298 for GitLab).
If tokens could vary per request, consider passing them as part of the
paramsdict to include them in the cache key:cache_params = {"token_hash": hashlib.sha256(self.github_token.encode()).hexdigest()[:8]} if self.github_token else {} commits_resp = await cache_request( url=f"{base_url}/commits?per_page=1", params=cache_params, external_api_call=lambda: self.client.get(f"{base_url}/commits?per_page=1", headers=headers), serializer=serialize_httpx_response, deserializer=deserialize_httpx_response )backend/app/utils/cache_utils.py (2)
15-19: LGTM: Cache key generation is robust.The use of
json.dumpswithsort_keys=Trueanddefault=strensures deterministic cache keys even with nested dictionaries and non-serializable types. SHA-256 hashing is appropriate for Redis keys.
49-61: LGTM: Cache write logic with transparent failure handling.The function correctly:
- Executes the external API call
- Attempts to serialize and cache the response
- Logs failures without raising exceptions
- Returns the original response regardless of caching success
This design ensures that cache failures don't break the application flow.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/services/agents/code_audit_agent.py(8 hunks)backend/app/utils/cache_utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/utils/cache_utils.py (2)
backend/app/services/agents/code_audit_agent.py (1)
json(33-35)backend/app/cache/redis_client.py (2)
get_cache(51-63)set_cache(37-49)
backend/app/services/agents/code_audit_agent.py (2)
backend/app/utils/cache_utils.py (1)
cache_request(23-62)backend/app/security/rate_limiter.py (1)
check_rate_limit(29-47)
🪛 Ruff (0.14.6)
backend/app/utils/cache_utils.py
44-44: Redundant exception object included in logging.exception call
(TRY401)
backend/app/services/agents/code_audit_agent.py
41-43: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
backend/app/services/agents/code_audit_agent.py (3)
15-45: Well-designed serialization/deserialization for httpx.Response objects.The
MockResponseclass correctly implements the interface needed by the calling code (status_code,headers,text,json(),raise_for_status()). This addresses the previous review concern about httpx.Response not being JSON-serializable.One minor note: the
raise_for_status()implementation passesself(the MockResponse) as theresponseparameter toHTTPStatusError, which expects anhttpx.Response. This works at runtime but could cause issues if any error handler inspects the response object expecting full httpx.Response attributes.
140-146: Cache integration pattern looks correct.The cache wrapping correctly:
- Passes the full URL including query params for unique cache key generation
- Includes a token hash in
paramsto differentiate cache entries by authentication context- Provides appropriate serializer/deserializer for httpx responses
This pattern is consistently applied across all GitHub and GitLab API calls.
242-248: GitLab caching follows the same correct pattern.Consistent with the GitHub implementation—uses token hash differentiation and proper serializer/deserializer callbacks.
backend/app/utils/cache_utils.py (2)
16-20: Deterministic cache key generation looks good.Using
json.dumps(params, sort_keys=True, default=str)correctly handles nested structures and ensures consistent key generation, addressing the previous review concern.
23-62: Clean implementation with proper graceful degradation.The function correctly:
- Falls back to cache miss on Redis errors
- Skips caching on serialization failures while still returning the response
- Accepts custom serializer/deserializer for complex types like
httpx.ResponseThis addresses all the concerns from previous reviews.
| try: | ||
| return deserializer(cached_response) | ||
| except json.JSONDecodeError as e: | ||
| logger.exception(f"Failed to deserialize cached response for key {cache_key}: {e}") | ||
| # If deserialization fails, treat as a cache miss | ||
| pass |
There was a problem hiding this comment.
Broaden exception handling for custom deserializers.
The catch block only handles json.JSONDecodeError, but custom deserializers may raise other exceptions (e.g., KeyError, TypeError, ValueError). A failed deserialization should fall back to a cache miss, not propagate the exception.
try:
return deserializer(cached_response)
- except json.JSONDecodeError as e:
- logger.exception(f"Failed to deserialize cached response for key {cache_key}: {e}")
+ except Exception as e:
+ logger.warning(f"Failed to deserialize cached response for key {cache_key}: {e}")
# If deserialization fails, treat as a cache miss
passAlso, logger.exception already includes the exception traceback, so including {e} in the message is redundant (per static analysis hint TRY401). Using logger.warning with the exception message is sufficient here since we're intentionally catching and recovering.
📝 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.
| try: | |
| return deserializer(cached_response) | |
| except json.JSONDecodeError as e: | |
| logger.exception(f"Failed to deserialize cached response for key {cache_key}: {e}") | |
| # If deserialization fails, treat as a cache miss | |
| pass | |
| try: | |
| return deserializer(cached_response) | |
| except Exception as e: | |
| logger.warning(f"Failed to deserialize cached response for key {cache_key}: {e}") | |
| # If deserialization fails, treat as a cache miss | |
| pass |
🧰 Tools
🪛 Ruff (0.14.6)
44-44: Redundant exception object included in logging.exception call
(TRY401)
🤖 Prompt for AI Agents
In backend/app/utils/cache_utils.py around lines 41 to 46, the deserialization
catch is too narrow and uses logger.exception redundantly; change the except
clause to catch broad exceptions from custom deserializers (e.g., except
Exception:) so any KeyError/TypeError/ValueError will be treated as a cache
miss, and replace logger.exception(...) with logger.warning(...) using a concise
message that names the cache key (do not interpolate the exception into the
string); then allow the function to continue treating the entry as a miss.
This PR introduces Redis caching for external API calls made by agents, improving performance and reducing redundant requests.
Changes
backend/app/utils/cache_utils.pywith acache_request(url, params)function.cache_requestinto all agent services to check Redis before making external API calls.backend/app/services/agents/and introduces a new utility file.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.