feat: add FastAPI patterns skill and reviewer agent#1508
feat: add FastAPI patterns skill and reviewer agent#1508DiwanshuG wants to merge 16 commits intoaffaan-m:mainfrom
Conversation
New contributions: - skills/fastapi-patterns/ - Comprehensive FastAPI best practices - agents/fastapi-reviewer.md - FastAPI code review specialist - commands/fastapi-review.md - Slash command for reviews - rules/python/fastapi.md - Project-specific FastAPI rules - Cross-harness: Codex and Cursor skill copies Coverage includes: - Project structure and application factory pattern - Pydantic schemas (request/response separation) - Dependency injection patterns - Async database operations with SQLAlchemy - Custom error handling and exceptions - JWT authentication and OAuth2 flow - Testing with pytest and TestClient - OpenAPI documentation customization - Production deployment (Docker, Gunicorn, config) - Performance optimization (caching, compression)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multiple FastAPI documentation artifacts: production-oriented patterns guides, a FastAPI reviewer agent spec and command, and a FastAPI ruleset. All changes are documentation-only; no runtime code or public API declarations were modified. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
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 |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
Greptile SummaryThis PR adds a FastAPI patterns skill, a reviewer agent, a slash command, and project rules — a solid reference contribution. However, the three Confidence Score: 3/5Not safe to merge until the remaining propagation gaps in the .agents/ and .cursor/ copies are resolved — developers copying those snippets will get ImportError, AttributeError, or silently broken behaviour. The main skills/ file is largely corrected from prior rounds, but the .agents/ and .cursor/ mirror copies still carry multiple P1 bugs (wrong AsyncSession import, un-wired custom_openapi, unguarded update(), CORS credentials mismatch). These are documentation files whose entire value is copy-pasteable correctness, so correctness bugs in the copies block merge. .agents/skills/fastapi-patterns/SKILL.md and .cursor/skills/fastapi-patterns/SKILL.md need all fixes from the main skills/ copy applied before merge. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
PR[PR: FastAPI Patterns Skill]
PR --> MAIN[skills/fastapi-patterns/SKILL.md - Most fixes applied]
PR --> AGENTS[.agents/skills/fastapi-patterns/SKILL.md - Fixes missing]
PR --> CURSOR[.cursor/skills/fastapi-patterns/SKILL.md - Fixes missing]
PR --> REVIEWER[agents/fastapi-reviewer.md - Clean]
PR --> CMD[commands/fastapi-review.md - Clean]
PR --> RULES[rules/python/fastapi.md - Clean]
MAIN -->|Not propagated to| AGENTS
MAIN -->|Not propagated to| CURSOR
AGENTS --> A1[CORS credentials=True + allow_origins=*]
AGENTS --> A2[AsyncSession from wrong module]
AGENTS --> A3[update missing exclude_unset=True]
AGENTS --> A4[custom_openapi called directly]
CURSOR --> C1[AsyncSession from wrong module]
CURSOR --> C2[custom_openapi called directly]
Reviews (12): Last reviewed commit: "fix(fastapi-patterns): register custom O..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".cursor/skills/fastapi-patterns/SKILL.md">
<violation number="1" location=".cursor/skills/fastapi-patterns/SKILL.md:103">
P2: The example CORS config combines allow_origins=['*'] with allow_credentials=True, which Starlette/FastAPI disallow and browsers reject. This is an invalid configuration and can lead to runtime errors or failed credentialed requests.</violation>
</file>
<file name="rules/python/fastapi.md">
<violation number="1" location="rules/python/fastapi.md:53">
P2: FastAPI path parameters must match the function argument name; the example uses `/users/{id}` but `user_id` in the signature, which would cause a 422 if copied.</violation>
<violation number="2" location="rules/python/fastapi.md:120">
P3: Async pytest fixture uses synchronous TestClient, which is misleading; async tests should use AsyncClient or the fixture should be sync.</violation>
</file>
<file name="skills/fastapi-patterns/SKILL.md">
<violation number="1" location="skills/fastapi-patterns/SKILL.md:381">
P2: Test dependency override yields an `AsyncConnection` (`engine.begin`) instead of an `AsyncSession`, conflicting with the documented ORM session usage and risking runtime breakage in CRUD code.</violation>
<violation number="2" location="skills/fastapi-patterns/SKILL.md:384">
P2: Test override targets the wrong dependency: `get_db_session` is overridden, but routes inject `get_db`, which calls `get_db_session` directly, so the override likely won’t apply.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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 (1)
commands/fastapi-review.md (1)
36-36: Minor wording polish: hyphenate compound modifier.At Line 36, consider
rate-limiting configurationfor cleaner grammar in the checklist item.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/fastapi-review.md` at line 36, Update the checklist item string "CORS and rate limiting configuration" to hyphenate the compound modifier by changing it to "CORS and rate-limiting configuration" so the phrase at Line 36 reads grammatically correct; locate and replace the exact checklist entry "CORS and rate limiting configuration" in commands/fastapi-review.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/fastapi-patterns/SKILL.md`:
- Around line 383-385: The test fixture currently sets
app.dependency_overrides[get_db_session] but routes call get_db (which
internally calls get_db_session() as a context manager), so the override is
never used; change the override target to app.dependency_overrides[get_db] =
override_get_db (ensure override_get_db has the same call signature as get_db),
remove or stop relying on get_db_session override, and keep using create_app()
to build the app so all endpoints pick up the get_db override.
- Around line 1135-1155: Update the Redis cache typings and the cached wrapper
signature: import Any from typing and change the signatures in RedisCache.get
and RedisCache.set to use Optional[Any] and value: Any (replace lowercase any),
and remove the default None from the cached wrapper's request parameter so it is
declared as request: Request (not request: Request = None) to avoid an unguarded
null dereference when computing cache_key; keep references to the methods get,
set, delete and the decorator function cached/wrapper when making the edits.
---
Nitpick comments:
In `@commands/fastapi-review.md`:
- Line 36: Update the checklist item string "CORS and rate limiting
configuration" to hyphenate the compound modifier by changing it to "CORS and
rate-limiting configuration" so the phrase at Line 36 reads grammatically
correct; locate and replace the exact checklist entry "CORS and rate limiting
configuration" in commands/fastapi-review.md.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e18c9a39-8b67-4f94-9599-23bbfbfdd0b5
📒 Files selected for processing (6)
.agents/skills/fastapi-patterns/SKILL.md.cursor/skills/fastapi-patterns/SKILL.mdagents/fastapi-reviewer.mdcommands/fastapi-review.mdrules/python/fastapi.mdskills/fastapi-patterns/SKILL.md
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="skills/fastapi-patterns/SKILL.md">
<violation number="1" location="skills/fastapi-patterns/SKILL.md:738">
P2: JWT example references `timezone.utc` without importing `timezone`, causing a runtime NameError in the documented pattern.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| subject: str, | ||
| expires_delta: Optional[timedelta] = None, | ||
| ) -> str: | ||
| expire = datetime.now(timezone.utc) + (expires_delta or timedelta(minutes=30)) |
There was a problem hiding this comment.
P2: JWT example references timezone.utc without importing timezone, causing a runtime NameError in the documented pattern.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/fastapi-patterns/SKILL.md, line 738:
<comment>JWT example references `timezone.utc` without importing `timezone`, causing a runtime NameError in the documented pattern.</comment>
<file context>
@@ -735,7 +735,7 @@ def create_access_token(
expires_delta: Optional[timedelta] = None,
) -> str:
- expire = datetime.utcnow() + (expires_delta or timedelta(minutes=30))
+ expire = datetime.now(timezone.utc) + (expires_delta or timedelta(minutes=30))
to_encode = {"exp": expire, "sub": str(subject)}
return jwt.encode(
</file context>
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
skills/fastapi-patterns/SKILL.md (2)
366-384:⚠️ Potential issue | 🟠 MajorOverride the dependency actually used by
Depends.At Line 383,
app.dependency_overrides[get_db_session] = override_get_dbwon’t affect handlers that depend onget_db(Line 249 / Line 302). Overrideget_dbinstead so tests consistently inject the test session.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/fastapi-patterns/SKILL.md` around lines 366 - 384, The test fixture currently sets app.dependency_overrides[get_db_session] but the routes use get_db, so update the override to target the actual dependency: set app.dependency_overrides[get_db] = override_get_db (keep the override_get_db async generator and AsyncSession logic as-is) so handlers that declare Depends(get_db) receive the in-memory AsyncSession during tests.
1134-1154:⚠️ Potential issue | 🟠 MajorFix invalid
anytyping and nullable request dereference in cache example.Line 1134/1138 should use
Any(not lowercaseany), and Line 1152’srequest: Request = Noneis unsafe because Line 1153 dereferencesrequest.url.pathunguarded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/fastapi-patterns/SKILL.md` around lines 1134 - 1154, The Redis cache methods and decorator use incorrect typing and unsafe request usage: change parameter/type hints from lowercase any to typing.Any (import Any) on RedisCache.get/set/delete and cached signature, and fix the wrapper to avoid dereferencing a nullable request by either requiring request: Request (no default None) or keeping request: Optional[Request] and adding a guard that computes cache_key only when request is present (fall back to func name + serialized args/kwargs when request is None); update references in get/set/setex methods and the cached wrapper accordingly (functions: RedisCache.get, RedisCache.set, RedisCache.delete, cached, wrapper).
🧹 Nitpick comments (1)
skills/fastapi-patterns/SKILL.md (1)
22-1179: Add explicitHow It WorksandExamplessection headers.The guide is comprehensive, but it should include clear top-level sections named
How It WorksandExamplesto match the skill format requirement. As per coding guidelines: “skills/**/*.md: Skill format must be Markdown with clear sections for When to Use, How It Works, and Examples”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/fastapi-patterns/SKILL.md` around lines 22 - 1179, Add two top-level Markdown sections "How It Works" and "Examples" to the SKILL.md document to satisfy the required skill format; insert them as H2 (## How It Works) and H2 (## Examples) headings in the logical spots (for example place "How It Works" after the "Project Structure" / before "Pydantic Schemas", and "Examples" before the "Dependency Injection" or near concrete code blocks) and populate each with a brief paragraph explaining the concept and one short code/example snippet or pointer to existing examples in the doc so the file includes clear "When to Use", "How It Works", and "Examples" top-level sections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/fastapi-patterns/SKILL.md`:
- Around line 714-738: The create_access_token function uses timezone.utc but
timezone isn't imported, causing a NameError; fix it by adding timezone to the
datetime imports (ensure the top import line includes "from datetime import
datetime, timedelta, timezone") so create_access_token (and any other uses of
timezone) can reference timezone.utc correctly.
- Line 765: The import for AsyncSession is wrong: replace the line that imports
AsyncSession from app.db.session with an import from sqlalchemy.ext.asyncio;
update the import statement that references AsyncSession so it reads "from
sqlalchemy.ext.asyncio import AsyncSession" to match the rest of the skill and
the app/db/session.py usage, ensuring any references to AsyncSession in
functions or examples (e.g., dependency providers or route handlers) continue to
use the corrected import.
- Around line 913-946: The custom_openapi function is being executed but not
registered as FastAPI's openapi hook; instead of simply calling
custom_openapi(app), assign the callable to app.openapi so FastAPI will call
your custom_openapi when serving /docs and /redoc. Locate the custom_openapi
function and the app = create_app() instantiation and set app.openapi = lambda:
custom_openapi(app) (or app.openapi = functools.partial(custom_openapi, app)) so
the framework uses your custom schema at runtime.
---
Duplicate comments:
In `@skills/fastapi-patterns/SKILL.md`:
- Around line 366-384: The test fixture currently sets
app.dependency_overrides[get_db_session] but the routes use get_db, so update
the override to target the actual dependency: set
app.dependency_overrides[get_db] = override_get_db (keep the override_get_db
async generator and AsyncSession logic as-is) so handlers that declare
Depends(get_db) receive the in-memory AsyncSession during tests.
- Around line 1134-1154: The Redis cache methods and decorator use incorrect
typing and unsafe request usage: change parameter/type hints from lowercase any
to typing.Any (import Any) on RedisCache.get/set/delete and cached signature,
and fix the wrapper to avoid dereferencing a nullable request by either
requiring request: Request (no default None) or keeping request:
Optional[Request] and adding a guard that computes cache_key only when request
is present (fall back to func name + serialized args/kwargs when request is
None); update references in get/set/setex methods and the cached wrapper
accordingly (functions: RedisCache.get, RedisCache.set, RedisCache.delete,
cached, wrapper).
---
Nitpick comments:
In `@skills/fastapi-patterns/SKILL.md`:
- Around line 22-1179: Add two top-level Markdown sections "How It Works" and
"Examples" to the SKILL.md document to satisfy the required skill format; insert
them as H2 (## How It Works) and H2 (## Examples) headings in the logical spots
(for example place "How It Works" after the "Project Structure" / before
"Pydantic Schemas", and "Examples" before the "Dependency Injection" or near
concrete code blocks) and populate each with a brief paragraph explaining the
concept and one short code/example snippet or pointer to existing examples in
the doc so the file includes clear "When to Use", "How It Works", and "Examples"
top-level sections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2b9e79b-a12b-43ed-a67c-b5c6ac91e3cc
📒 Files selected for processing (2)
rules/python/fastapi.mdskills/fastapi-patterns/SKILL.md
✅ Files skipped from review due to trivial changes (1)
- rules/python/fastapi.md
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="skills/fastapi-patterns/SKILL.md">
<violation number="1" location="skills/fastapi-patterns/SKILL.md:1152">
P1: Caching decorator example requires `request` but does not reliably receive or forward it, making the documented pattern prone to runtime failure.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| """Decorator for caching endpoint responses.""" | ||
| def decorator(func): | ||
| @wraps(func) | ||
| async def wrapper(*args, request: Request, **kwargs): |
There was a problem hiding this comment.
P1: Caching decorator example requires request but does not reliably receive or forward it, making the documented pattern prone to runtime failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/fastapi-patterns/SKILL.md, line 1152:
<comment>Caching decorator example requires `request` but does not reliably receive or forward it, making the documented pattern prone to runtime failure.</comment>
<file context>
@@ -1148,7 +1149,7 @@ def cached(expire: int = 300):
def decorator(func):
@wraps(func)
- async def wrapper(*args, request: Request = None, **kwargs):
+ async def wrapper(*args, request: Request, **kwargs):
cache_key = f"{func.__name__}:{request.url.path}:{request.query_params}"
</file context>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".agents/skills/fastapi-patterns/SKILL.md">
<violation number="1" location=".agents/skills/fastapi-patterns/SKILL.md:380">
P2: Test DB override drops commit/rollback handling, so tests no longer mirror production transaction behavior and may not persist writes that production commits after dependency yield.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…nd remove None guard
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
| from app.dependencies import get_db | ||
| from app.schemas.token import Token | ||
| from app.core.security import verify_password, create_access_token | ||
| from app.db.session import AsyncSession |
There was a problem hiding this comment.
AsyncSession import — ImportError on module load
AsyncSession is exported from sqlalchemy.ext.asyncio, not app.db.session. Any developer who copies this auth router snippet will immediately get ImportError: cannot import name 'AsyncSession' from 'app.db.session' when the module is loaded. The main skills/fastapi-patterns/SKILL.md already has the correct import (from sqlalchemy.ext.asyncio import AsyncSession), but it wasn't carried over to this copy or the .cursor/ copy (line 767 there).
| from app.db.session import AsyncSession | |
| from sqlalchemy.ext.asyncio import AsyncSession |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
|
||
|
|
||
| app = create_app() | ||
| custom_openapi(app) |
There was a problem hiding this comment.
custom_openapi not wired to app.openapi in .cursor/ copy
custom_openapi(app) is called once eagerly to populate app.openapi_schema, but it is never assigned as app.openapi. The main skills/fastapi-patterns/SKILL.md was already corrected to use app.openapi = lambda: custom_openapi(app), making the schema lazily regenerated on each call to /openapi.json. This .cursor/ copy (and the .agents/ copy at line 952) still has the old one-shot call, so if app.openapi_schema is ever cleared the custom schema will not regenerate.
| custom_openapi(app) | |
| app.openapi = lambda: custom_openapi(app) |
The identical fix is also needed at .agents/skills/fastapi-patterns/SKILL.md line 952.
|
Addressed all bot suggestions:
Happy to make any further changes if needed! |
1 participant
New contributions:
Coverage includes:
What Changed
Why This Change
Testing Done
node tests/run-all.js)Type of Change
fix:Bug fixfeat:New featurerefactor:Code refactoringdocs:Documentationtest:Testschore:Maintenance/toolingci:CI/CD changesSecurity & Quality Checklist
Documentation
Summary by CodeRabbit