Merged
Conversation
WalkthroughThis change introduces configurable authentication cookie naming, extends API registration to support OpenAPI metadata (tags and descriptions), adds plugin description field support, and creates a utility function for formatting plugin metadata. Changes are coordinated across authentication, ingress routing, and plugin systems to enable enhanced API documentation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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 (2)
tests/driver/test_auth.py (1)
14-14: UseAUTH_COOKIE_NAMEin the callback Set-Cookie assertion too.Line 186 still checks
"token=", which is a loose substring check. Prefer exact key matching with the constant.Proposed test tightening
- assert "token=" in res.headers.get("set-cookie", "") + assert f"{AUTH_COOKIE_NAME}=" in res.headers.get("set-cookie", "")Also applies to: 222-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/driver/test_auth.py` at line 14, The Set-Cookie assertions in the callback tests use the loose substring "token="; update those assertions to use the AUTH_COOKIE_NAME constant instead so the cookie key is matched exactly. Find the assertions that inspect response.headers["Set-Cookie"] (currently checking for "token=" at the callback test locations) and replace the literal with a check for f"{AUTH_COOKIE_NAME}=" (or otherwise ensure the header contains the AUTH_COOKIE_NAME key) so the tests assert the exact cookie name.src/framex/driver/ingress.py (1)
219-227: Consider updating existing tag metadata when description becomes available.If a tag is first registered without description, later routes cannot enrich it. Updating existing entries (when empty) would make tag docs stable regardless of registration order.
♻️ Suggested refinement
if include_in_schema and tags: - names = list({tag["name"] for tag in app.state.tags_metadata_map}) - for tag in tags: - if tag not in names: - app.state.tags_metadata_map.append( - { - "name": tag, - "description": description, - }, - ) + metadata_by_name = {item["name"]: item for item in app.state.tags_metadata_map} + for tag in tags: + entry = metadata_by_name.get(tag) + if entry is None: + entry = {"name": tag, "description": description} + app.state.tags_metadata_map.append(entry) + metadata_by_name[tag] = entry + elif description and not entry.get("description"): + entry["description"] = description🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framex/driver/ingress.py` around lines 219 - 227, The current logic in the include_in_schema branch appends a new tag dict unconditionally when a tag name isn't in names, which prevents enriching an existing entry's description later; update the code that manipulates app.state.tags_metadata_map so that for each tag in tags you first search for an existing entry by name (using the same identity as names), and if found and its "description" is missing/empty, set/overwrite it with the provided description; only append a new dict when no existing entry exists; reference the include_in_schema condition, the tags loop, names set, and app.state.tags_metadata_map when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framex/driver/ingress.py`:
- Around line 210-217: The route registration call in app.add_api_route (inside
the ingress setup) drops the operation-level docs because the description
argument is not forwarded; update the app.add_api_route invocation that
currently passes path, endpoint, methods=list(method_set), tags=tags,
include_in_schema=include_in_schema, **kwargs to also include
description=description so the route’s OpenAPI/operation description is
preserved (ensure the local variable description is defined/available where
add_api_route is called).
In `@src/framex/plugins/proxy/__init__.py`:
- Around line 173-178: The code always prefixes the version with "v" when
calling build_plugin_description which can yield duplicated "v" if
__plugin_meta__.version already starts with one; modify the call site to
normalize the version first (e.g., read __plugin_meta__.version, strip any
leading "v" or "V", then format with a single "v" prefix) and pass that
normalized string to build_plugin_description (affecting the description
variable and the call that references __plugin_meta__.version and
build_plugin_description).
---
Nitpick comments:
In `@src/framex/driver/ingress.py`:
- Around line 219-227: The current logic in the include_in_schema branch appends
a new tag dict unconditionally when a tag name isn't in names, which prevents
enriching an existing entry's description later; update the code that
manipulates app.state.tags_metadata_map so that for each tag in tags you first
search for an existing entry by name (using the same identity as names), and if
found and its "description" is missing/empty, set/overwrite it with the provided
description; only append a new dict when no existing entry exists; reference the
include_in_schema condition, the tags loop, names set, and
app.state.tags_metadata_map when making this change.
In `@tests/driver/test_auth.py`:
- Line 14: The Set-Cookie assertions in the callback tests use the loose
substring "token="; update those assertions to use the AUTH_COOKIE_NAME constant
instead so the cookie key is matched exactly. Find the assertions that inspect
response.headers["Set-Cookie"] (currently checking for "token=" at the callback
test locations) and replace the literal with a check for f"{AUTH_COOKIE_NAME}="
(or otherwise ensure the header contains the AUTH_COOKIE_NAME key) so the tests
assert the exact cookie name.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de594b2a-c13c-4df0-bc67-86d39c249e38
📒 Files selected for processing (11)
src/framex/consts.pysrc/framex/driver/application.pysrc/framex/driver/auth.pysrc/framex/driver/ingress.pysrc/framex/plugin/base.pysrc/framex/plugin/model.pysrc/framex/plugin/on.pysrc/framex/plugins/proxy/__init__.pysrc/framex/plugins/proxy/config.pytests/driver/test_auth.pytests/driver/test_ingress.py
Comment on lines
+210
to
+217
| app.add_api_route( | ||
| path, | ||
| endpoint, | ||
| methods=list(method_set), # type: ignore | ||
| tags=tags, # type: ignore | ||
| include_in_schema=include_in_schema, | ||
| **kwargs, | ||
| ) |
There was a problem hiding this comment.
description is dropped before FastAPI route registration.
Line 210-217 calls app.add_api_route(...) without passing the description argument, so operation-level docs metadata is never applied.
✅ Proposed fix
app.add_api_route(
path,
endpoint,
methods=list(method_set), # type: ignore
tags=tags, # type: ignore
+ description=description,
include_in_schema=include_in_schema,
**kwargs,
)📝 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.
Suggested change
| app.add_api_route( | |
| path, | |
| endpoint, | |
| methods=list(method_set), # type: ignore | |
| tags=tags, # type: ignore | |
| include_in_schema=include_in_schema, | |
| **kwargs, | |
| ) | |
| app.add_api_route( | |
| path, | |
| endpoint, | |
| methods=list(method_set), # type: ignore | |
| tags=tags, # type: ignore | |
| description=description, | |
| include_in_schema=include_in_schema, | |
| **kwargs, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framex/driver/ingress.py` around lines 210 - 217, The route registration
call in app.add_api_route (inside the ingress setup) drops the operation-level
docs because the description argument is not forwarded; update the
app.add_api_route invocation that currently passes path, endpoint,
methods=list(method_set), tags=tags, include_in_schema=include_in_schema,
**kwargs to also include description=description so the route’s
OpenAPI/operation description is preserved (ensure the local variable
description is defined/available where add_api_route is called).
Comment on lines
+173
to
+178
| description = build_plugin_description( | ||
| __plugin_meta__.author, | ||
| f"v{__plugin_meta__.version}", | ||
| __plugin_meta__.description, | ||
| __plugin_meta__.url, | ||
| ) |
There was a problem hiding this comment.
Normalize version prefix before building description.
Line 175 always prepends "v", which can produce vv... if the configured version already includes a prefix. Normalize once before passing it to build_plugin_description.
💡 Proposed fix
- description = build_plugin_description(
+ version = __plugin_meta__.version
+ version = version if version.startswith("v") else f"v{version}"
+ description = build_plugin_description(
__plugin_meta__.author,
- f"v{__plugin_meta__.version}",
+ version,
__plugin_meta__.description,
__plugin_meta__.url,
)📝 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.
Suggested change
| description = build_plugin_description( | |
| __plugin_meta__.author, | |
| f"v{__plugin_meta__.version}", | |
| __plugin_meta__.description, | |
| __plugin_meta__.url, | |
| ) | |
| version = __plugin_meta__.version | |
| version = version if version.startswith("v") else f"v{version}" | |
| description = build_plugin_description( | |
| __plugin_meta__.author, | |
| version, | |
| __plugin_meta__.description, | |
| __plugin_meta__.url, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framex/plugins/proxy/__init__.py` around lines 173 - 178, The code always
prefixes the version with "v" when calling build_plugin_description which can
yield duplicated "v" if __plugin_meta__.version already starts with one; modify
the call site to normalize the version first (e.g., read
__plugin_meta__.version, strip any leading "v" or "V", then format with a single
"v" prefix) and pass that normalized string to build_plugin_description
(affecting the description variable and the call that references
__plugin_meta__.version and build_plugin_description).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Updates