Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 28 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds a repository versioning and access subsystem (GitHub/GitLab providers), session-backed OAuth/JWT authentication, protected plugin documentation endpoints, extended configuration for repository/docs, new utility modules (cache, config docs, common helpers), and expanded tests covering auth, docs, and repository behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI as App /docs/plugin-config
participant Auth as auth module
participant Repo as repository.versioning
participant Plugin as plugin loader
participant Docs as config_docs builder
Client->>FastAPI: GET /docs/plugin-config?plugin=X (cookie)
FastAPI->>Auth: get_auth_payload(request)
Auth-->>FastAPI: user payload or None
alt OAuth enabled and unauthenticated
FastAPI-->>Client: 403
else
FastAPI->>Plugin: get_plugin(plugin)
Plugin-->>FastAPI: metadata/config or None
FastAPI->>Repo: is_private_repository(repo_url)
Repo-->>FastAPI: True/False/None
alt private repo
FastAPI->>Repo: can_access_repository(repo_url, provider, token)
Repo-->>FastAPI: True/False/None
alt access denied
FastAPI-->>Client: 403
end
end
FastAPI->>Docs: collect_embedded_config_files(config, whitelist)
Docs-->>FastAPI: embedded files
FastAPI->>Docs: build_plugin_config_html(config, embedded_files)
Docs-->>FastAPI: HTMLResponse
FastAPI-->>Client: 200 HTML
end
sequenceDiagram
participant Client
participant FastAPI as App /docs/plugin-release
participant Repo as repository.versioning
participant Provider as GitHub/GitLab provider
Client->>FastAPI: GET /docs/plugin-release?plugin=X
FastAPI->>Repo: get_latest_repository_version(repo_url)
Repo->>Provider: resolve & get_latest_version(parsed_url)
Provider->>Provider: fetch_json(api_endpoint, headers)
Provider-->>Repo: latest_version or None
Repo-->>FastAPI: latest_version
FastAPI->>Repo: has_newer_release_version(current, latest)
Repo-->>FastAPI: boolean
FastAPI-->>Client: 200 JSON {has_update, latest_version}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Codecov Report❌ Patch coverage is 📢 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: 10
🧹 Nitpick comments (2)
src/framex/utils/__init__.py (1)
3-3: Typo in re-exported symbol:StreamEnventType.The symbol
StreamEnventTypeoriginates fromsrc/framex/utils/common.pyand contains a typo ("Envent" instead of "Event"). Consider fixing the class definition toStreamEventTypein a follow-up to prevent further propagation through the public API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framex/utils/__init__.py` at line 3, The public re-export mistakenly uses the misspelled symbol StreamEnventType; update the original class/enum definition in common.py to the correct name StreamEventType and then update the re-export list in __init__.py to export StreamEventType instead of StreamEnventType, ensuring all internal references (e.g., imports, type annotations, and usages) are updated to the new StreamEventType identifier to avoid breaking imports.src/framex/repository/versioning.py (1)
20-25: Consider a TTL or explicit invalidation for release lookups.This cache is process-lifetime, so
/docs/plugin-releasecan keep serving an old latest version indefinitely after an upstream release is published. A short TTL cache or an explicit invalidation hook would make the docs view much less stale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framex/repository/versioning.py` around lines 20 - 25, The get_latest_repository_version function is using functools.lru_cache without expiration so docs can serve stale release data; replace the process-lifetime lru_cache with a TTL-backed cache or add an explicit invalidation hook: either swap the decorator for a TTL cache (e.g., cachetools.TTLCache or an equivalent cached decorator) around get_latest_repository_version(repo_url) with a short TTL (minutes) or keep lru_cache but add an exported invalidation function (e.g., invalidate_latest_repository_version) that calls get_latest_repository_version.cache_clear() or a key-targeted clear implementation so the docs view can refresh after upstream releases. Ensure references to _get_provider_for_url and provider.get_latest_version(parsed_url) remain unchanged.
🤖 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/config.py`:
- Around line 107-115: The current matches method (matches) uses
path.startswith(normalized_prefix) which incorrectly matches sibling segments
(e.g., "/team-alpha" for prefix "/team-a"); update matches to only accept an
empty prefix or exact match or a segment-prefix match by checking path ==
normalized_prefix or path.startswith(normalized_prefix + "/"); keep
normalized_path_prefix and path_prefix logic as-is but reference
normalized_path_prefix in the new checks to ensure correct leading-slash
handling.
- Around line 118-124: The GitLabRepositoryAuthConfig.configured_hosts method
only returns hosts from endpoints so a global token (set on the provider config)
never applies to self-hosted GitLab instances; update configured_hosts in class
GitLabRepositoryAuthConfig to include a wildcard/fallback host (for example an
empty string or "*" entry) when the provider-level token is set (check the
inherited token field on RepositoryProviderAuthConfig) so that the matching
logic in src/framex/repository/providers/gitlab.py will consider the global
token for self-hosted hosts as well.
In `@src/framex/driver/application.py`:
- Around line 173-183: The fallback that reads config from settings.plugins (the
settings.plugins.get(plugin) branch) bypasses repository access checks; update
the handler so that before returning build_plugin_config_html for the
settings-based config you perform the same repository access validation used
when loaded_plugin is present (reuse the same access-check logic/function used
for loaded_plugin), or explicitly gate this path behind the same permission
check and only proceed if the request has access; ensure you still call
collect_embedded_config_files with workspace_root=Path.cwd().resolve() and the
same whitelist, and if access is denied raise
HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=f"Plugin config not
found: {plugin}") as in the other branch so settings-based configs are not
exposed without validation.
In `@src/framex/driver/auth.py`:
- Line 17: The in-memory session store _AUTH_SESSIONS makes auth process-local
and volatile; replace it with a shared persistent store or switch to stateless
tokens: implement a session backend using Redis/DB (e.g., wrap get/set/delete
operations used by the auth functions that reference _AUTH_SESSIONS) and update
all places that read/write _AUTH_SESSIONS to call that backend, or alternatively
encode the complete auth state into the signed token and remove reliance on
_AUTH_SESSIONS; update the functions/methods that currently interact with
_AUTH_SESSIONS (and the related code referenced around lines 46-55) to use the
chosen shared/session-backend API.
- Around line 74-76: The handler currently returns payload when session_id is
missing which lets sessionless JWTs authenticate; change the logic so that if
session_id is not a non-empty str (the check around payload.get("session_id")),
the function rejects the token instead of returning payload—e.g. return an
authentication failure/None or raise the appropriate auth exception so
downstream code treats it as unauthenticated; if you need legacy behavior, add
an explicit compatibility flag to gate accepting sessionless tokens rather than
falling back to returning payload.
In `@src/framex/repository/providers/gitlab.py`:
- Around line 64-74: The current _iter_project_path_candidates uses
GITLAB_RESERVED_PATH_MARKERS to cut off candidate generation early, which
prevents valid repos like "/tree/platform" from being tried; change
_iter_project_path_candidates so it does not treat reserved markers as hard
stops—remove the marker_index-based truncation and set max_length to len(parts)
(or otherwise generate all prefix candidates from len(parts) down to 2),
allowing _resolve_project_path to disambiguate via API probes; keep using
extract_repository_parts(parsed_url) and return the same list comprehension join
logic for candidates.
In `@src/framex/utils/cache.py`:
- Around line 61-74: The current restore_models branch for items with "__type__"
== "dynamic_obj" unsafely imports and instantiates classes from untrusted
payloads; update restore_models to stop dynamic import/instantiation and instead
map allowed (module, class) pairs to safe constructors via an explicit allowlist
(e.g., ALLOWED_DYNAMIC_TYPES) and only call import/getattr or model_validate
when the (item["__module__"], item["__class__"]) tuple is in that allowlist; for
any other values, return the cleaned_data as a plain dict (not SimpleNamespace)
to avoid executing arbitrary code, and ensure cache_decode-related callers
(restore_models) handle these plain dicts correctly.
In `@src/framex/utils/common.py`:
- Around line 17-24: path_to_module_name currently uses rel_path =
path.resolve().relative_to(Path.cwd()) which fails for plugin files outside the
CWD; update path_to_module_name to avoid relative_to(Path.cwd()) and instead
work from the absolute path parts: resolve the path, inspect its parts, detect
if the file is an __init__ and drop the last component, then locate a package
root marker (e.g. "src") in the parts and join the parts after that with dots to
form the module name (if "src" not found, join the parts from the first
directory that should be treated as the package root or use the full dotted path
from the filename without suffix); keep the removeprefix("src.") behavior by
removing a leading "src" segment if present and ensure module_name handles both
module files and package __init__.py correctly in the path_to_module_name
function.
- Around line 32-39: extract_method_params currently returns inspect._empty for
unannotated parameters which breaks downstream validation expecting real types;
update extract_method_params to map param.annotation == inspect._empty to
typing.Any before appending so every tuple uses a concrete type (e.g., change
the value appended in extract_method_params to (param.name, Any) when
param.annotation is inspect._empty), ensure Any is imported from typing if not
already, and keep skipping "self" unchanged.
In `@src/framex/utils/config_docs.py`:
- Around line 230-249: _normalize_display_config_paths currently rewrites any
string-looking path with supported suffixes into an absolute/display path
without checking workspace membership or the whitelist, which leaks blocked
embedded file paths; update _normalize_display_config_paths to apply the same
gating used by collect_embedded_config_files(): verify that the candidate path
is inside the resolved workspace_root and/or present in the allowed/whitelist
before calling _to_display_embedded_config_path (or else leave the original
string untouched), reusing or mirroring collect_embedded_config_files() checks
(and the SUPPORTED_EMBEDDED_CONFIG_SUFFIXES guard) so out-of-workspace or
non-whitelisted files are not converted to display/absolute paths.
---
Nitpick comments:
In `@src/framex/repository/versioning.py`:
- Around line 20-25: The get_latest_repository_version function is using
functools.lru_cache without expiration so docs can serve stale release data;
replace the process-lifetime lru_cache with a TTL-backed cache or add an
explicit invalidation hook: either swap the decorator for a TTL cache (e.g.,
cachetools.TTLCache or an equivalent cached decorator) around
get_latest_repository_version(repo_url) with a short TTL (minutes) or keep
lru_cache but add an exported invalidation function (e.g.,
invalidate_latest_repository_version) that calls
get_latest_repository_version.cache_clear() or a key-targeted clear
implementation so the docs view can refresh after upstream releases. Ensure
references to _get_provider_for_url and provider.get_latest_version(parsed_url)
remain unchanged.
In `@src/framex/utils/__init__.py`:
- Line 3: The public re-export mistakenly uses the misspelled symbol
StreamEnventType; update the original class/enum definition in common.py to the
correct name StreamEventType and then update the re-export list in __init__.py
to export StreamEventType instead of StreamEnventType, ensuring all internal
references (e.g., imports, type annotations, and usages) are updated to the new
StreamEventType identifier to avoid breaking imports.
🪄 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: c56ec41f-e29d-4d83-9bca-d847151dee33
📒 Files selected for processing (23)
README.mdsrc/framex/config.pysrc/framex/driver/application.pysrc/framex/driver/auth.pysrc/framex/plugin/on.pysrc/framex/plugins/proxy/__init__.pysrc/framex/repository/__init__.pysrc/framex/repository/providers/__init__.pysrc/framex/repository/providers/base.pysrc/framex/repository/providers/github.pysrc/framex/repository/providers/gitlab.pysrc/framex/repository/versioning.pysrc/framex/utils/__init__.pysrc/framex/utils/cache.pysrc/framex/utils/common.pysrc/framex/utils/config_docs.pysrc/framex/utils/docs.pytests/api/test_proxy.pytests/conftest.pytests/driver/test_auth.pytests/mock.pytests/test_config.pytests/test_utils.py
Comment on lines
+118
to
+124
| class GitLabRepositoryAuthConfig(RepositoryProviderAuthConfig): | ||
| token_header: str = "PRIVATE-TOKEN" # noqa | ||
| token_scheme: str = "" | ||
| endpoints: list[GitLabRepositoryAuthEndpointConfig] = Field(default_factory=list) | ||
|
|
||
| def configured_hosts(self) -> set[str]: | ||
| return {endpoint.host.lower() for endpoint in self.endpoints} |
There was a problem hiding this comment.
Global GitLab auth cannot target a self-hosted instance by itself.
configured_hosts() only returns hosts from endpoints, and src/framex/repository/providers/gitlab.py only matches self-hosted GitLab URLs when the host appears there. If a deployment sets only the global repository.auth.gitlab.token, version/access checks for gitlab.internal.example never run.
One way to model the missing host mapping
class GitLabRepositoryAuthConfig(RepositoryProviderAuthConfig):
+ hosts: list[str] = Field(default_factory=list)
token_header: str = "PRIVATE-TOKEN" # noqa
token_scheme: str = ""
endpoints: list[GitLabRepositoryAuthEndpointConfig] = Field(default_factory=list)
def configured_hosts(self) -> set[str]:
- return {endpoint.host.lower() for endpoint in self.endpoints}
+ return {host.lower() for host in self.hosts} | {endpoint.host.lower() for endpoint in self.endpoints}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framex/config.py` around lines 118 - 124, The
GitLabRepositoryAuthConfig.configured_hosts method only returns hosts from
endpoints so a global token (set on the provider config) never applies to
self-hosted GitLab instances; update configured_hosts in class
GitLabRepositoryAuthConfig to include a wildcard/fallback host (for example an
empty string or "*" entry) when the provider-level token is set (check the
inherited token field on RepositoryProviderAuthConfig) so that the matching
logic in src/framex/repository/providers/gitlab.py will consider the global
token for self-hosted hosts as well.
|
|
||
| api_key_header = APIKeyHeader(name="Authorization", auto_error=False) | ||
| SESSION_LIFETIME = timedelta(hours=24) | ||
| _AUTH_SESSIONS: dict[str, dict[str, Any]] = {} |
There was a problem hiding this comment.
This session store only works on one process.
_AUTH_SESSIONS lives in module memory, so a login created on worker A is invisible to worker B, and every restart drops all active sessions. That will make docs auth flaky as soon as this runs with multiple workers/instances. Please move the session state to shared storage (for example Redis/DB), or keep the full auth state in the signed token if stateless auth is the goal.
Also applies to: 46-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framex/driver/auth.py` at line 17, The in-memory session store
_AUTH_SESSIONS makes auth process-local and volatile; replace it with a shared
persistent store or switch to stateless tokens: implement a session backend
using Redis/DB (e.g., wrap get/set/delete operations used by the auth functions
that reference _AUTH_SESSIONS) and update all places that read/write
_AUTH_SESSIONS to call that backend, or alternatively encode the complete auth
state into the signed token and remove reliance on _AUTH_SESSIONS; update the
functions/methods that currently interact with _AUTH_SESSIONS (and the related
code referenced around lines 46-55) to use the chosen shared/session-backend
API.
Comment on lines
+64
to
+74
| def _iter_project_path_candidates(self, parsed_url: ParseResult) -> list[str]: | ||
| parts = self.extract_repository_parts(parsed_url) | ||
| if len(parts) < 2: | ||
| return [] | ||
|
|
||
| marker_index = next((index for index, part in enumerate(parts) if part in GITLAB_RESERVED_PATH_MARKERS), None) | ||
| max_length = marker_index if marker_index is not None else len(parts) | ||
| if max_length < 2: | ||
| return [] | ||
|
|
||
| return ["/".join(parts[:length]) for length in range(max_length, 1, -1)] |
There was a problem hiding this comment.
Don’t treat reserved markers as hard stops inside namespace/project names.
This truncates candidates as soon as any segment equals tree, blob, tags, etc. A valid repo like /tree/platform or /group/tags-service can never resolve even though _resolve_project_path() already has the API probe needed to disambiguate.
Safer candidate generation
def _iter_project_path_candidates(self, parsed_url: ParseResult) -> list[str]:
parts = self.extract_repository_parts(parsed_url)
if len(parts) < 2:
return []
- marker_index = next((index for index, part in enumerate(parts) if part in GITLAB_RESERVED_PATH_MARKERS), None)
- max_length = marker_index if marker_index is not None else len(parts)
- if max_length < 2:
- return []
-
- return ["/".join(parts[:length]) for length in range(max_length, 1, -1)]
+ return ["/".join(parts[:length]) for length in range(len(parts), 1, -1)]📝 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
| def _iter_project_path_candidates(self, parsed_url: ParseResult) -> list[str]: | |
| parts = self.extract_repository_parts(parsed_url) | |
| if len(parts) < 2: | |
| return [] | |
| marker_index = next((index for index, part in enumerate(parts) if part in GITLAB_RESERVED_PATH_MARKERS), None) | |
| max_length = marker_index if marker_index is not None else len(parts) | |
| if max_length < 2: | |
| return [] | |
| return ["/".join(parts[:length]) for length in range(max_length, 1, -1)] | |
| def _iter_project_path_candidates(self, parsed_url: ParseResult) -> list[str]: | |
| parts = self.extract_repository_parts(parsed_url) | |
| if len(parts) < 2: | |
| return [] | |
| return ["/".join(parts[:length]) for length in range(len(parts), 1, -1)] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framex/repository/providers/gitlab.py` around lines 64 - 74, The current
_iter_project_path_candidates uses GITLAB_RESERVED_PATH_MARKERS to cut off
candidate generation early, which prevents valid repos like "/tree/platform"
from being tried; change _iter_project_path_candidates so it does not treat
reserved markers as hard stops—remove the marker_index-based truncation and set
max_length to len(parts) (or otherwise generate all prefix candidates from
len(parts) down to 2), allowing _resolve_project_path to disambiguate via API
probes; keep using extract_repository_parts(parsed_url) and return the same list
comprehension join logic for candidates.
Comment on lines
+61
to
+74
| if item.get("__type__") == "dynamic_obj": | ||
| try: | ||
| module = importlib.import_module(item["__module__"]) | ||
| cls = getattr(module, item["__class__"]) | ||
|
|
||
| cleaned_data = {k: restore_models(v) for k, v in item["data"].items()} | ||
|
|
||
| if hasattr(cls, "model_validate"): | ||
| return cls.model_validate(cleaned_data) | ||
| return cls(**cleaned_data) | ||
| except Exception: | ||
| from types import SimpleNamespace | ||
|
|
||
| return SimpleNamespace(**{k: restore_models(v) for k, v in item["data"].items()}) |
There was a problem hiding this comment.
Stop restoring arbitrary classes from decoded payloads.
This branch trusts __module__ / __class__ from the serialized data and then imports + instantiates that type. Because cache_decode() is later used on request-derived proxy payloads, a crafted body can drive arbitrary imports/constructors here. Please restrict restoration to an explicit allowlist of cache-safe types, or leave decoded values as plain dicts.
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 71-71: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framex/utils/cache.py` around lines 61 - 74, The current restore_models
branch for items with "__type__" == "dynamic_obj" unsafely imports and
instantiates classes from untrusted payloads; update restore_models to stop
dynamic import/instantiation and instead map allowed (module, class) pairs to
safe constructors via an explicit allowlist (e.g., ALLOWED_DYNAMIC_TYPES) and
only call import/getattr or model_validate when the (item["__module__"],
item["__class__"]) tuple is in that allowlist; for any other values, return the
cleaned_data as a plain dict (not SimpleNamespace) to avoid executing arbitrary
code, and ensure cache_decode-related callers (restore_models) handle these
plain dicts correctly.
Comment on lines
+17
to
+24
| def path_to_module_name(path: Path) -> str: | ||
| """Convert path to module name.""" | ||
| rel_path = path.resolve().relative_to(Path.cwd().resolve()) | ||
| if rel_path.stem == "__init__": | ||
| module_name = ".".join(rel_path.parts[:-1]) | ||
| else: | ||
| module_name = ".".join([*rel_path.parts[:-1], rel_path.stem]) # type: ignore[arg-type] | ||
| return module_name.removeprefix("src.") |
There was a problem hiding this comment.
Don’t derive plugin module names relative to Path.cwd().
src/framex/plugin/manage.py passes absolute module_spec.origin paths from discovered plugin search roots. If a plugin lives outside the current working directory, relative_to(Path.cwd()) raises and plugin discovery fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framex/utils/common.py` around lines 17 - 24, path_to_module_name
currently uses rel_path = path.resolve().relative_to(Path.cwd()) which fails for
plugin files outside the CWD; update path_to_module_name to avoid
relative_to(Path.cwd()) and instead work from the absolute path parts: resolve
the path, inspect its parts, detect if the file is an __init__ and drop the last
component, then locate a package root marker (e.g. "src") in the parts and join
the parts after that with dots to form the module name (if "src" not found, join
the parts from the first directory that should be treated as the package root or
use the full dotted path from the filename without suffix); keep the
removeprefix("src.") behavior by removing a leading "src" segment if present and
ensure module_name handles both module files and package __init__.py correctly
in the path_to_module_name function.
Comment on lines
+32
to
+39
| def extract_method_params(func: Callable) -> list[tuple[str, Any]]: | ||
| sig = inspect.signature(func) | ||
| params: list[tuple[str, Any]] = [] | ||
| for param in sig.parameters.values(): | ||
| if param.name == "self": | ||
| continue | ||
| params.append((param.name, param.annotation)) | ||
| return params |
There was a problem hiding this comment.
Map missing annotations to a real type before storing params.
For unannotated arguments, inspect.signature() returns inspect._empty, but PluginApi.params expects type[Any] | Callable[..., Any]. Passing the sentinel through here can break model validation or downstream schema generation.
Proposed fix
def extract_method_params(func: Callable) -> list[tuple[str, Any]]:
sig = inspect.signature(func)
params: list[tuple[str, Any]] = []
for param in sig.parameters.values():
if param.name == "self":
continue
- params.append((param.name, param.annotation))
+ annotation = Any if param.annotation is inspect._empty else param.annotation
+ params.append((param.name, annotation))
return params🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framex/utils/common.py` around lines 32 - 39, extract_method_params
currently returns inspect._empty for unannotated parameters which breaks
downstream validation expecting real types; update extract_method_params to map
param.annotation == inspect._empty to typing.Any before appending so every tuple
uses a concrete type (e.g., change the value appended in extract_method_params
to (param.name, Any) when param.annotation is inspect._empty), ensure Any is
imported from typing if not already, and keep skipping "self" unchanged.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/framex/utils/config_docs.py (1)
282-293: Narrow the blindexcept Exception.Ruff flags BLE001 here. The
tryblock only runstomllib.loads,yaml.safe_load, andyaml.safe_dump— catchingtomllib.TOMLDecodeError,yaml.YAMLError, andValueError/UnicodeDecodeErrorkeeps the fallback intentional and avoids swallowing unrelated bugs (e.g., aKeyErrororRecursionErrorinmask_sensitive_config_datawould be silently hidden today).🔧 Suggested change
- try: + try: if suffix == ".toml": parsed = tomllib.loads(content) return _format_plugin_config_toml(mask_sensitive_config_data(parsed)) if suffix in {".yaml", ".yml"}: parsed = yaml.safe_load(content) masked = mask_sensitive_config_data(parsed) return yaml.safe_dump(masked, allow_unicode=True, sort_keys=False).rstrip() - except Exception: + except (tomllib.TOMLDecodeError, yaml.YAMLError, ValueError, UnicodeDecodeError): return mask_sensitive_config_text(content)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framex/utils/config_docs.py` around lines 282 - 293, The current broad except hides unrelated errors; narrow it to only handle parsing/serialization failures by catching tomllib.TOMLDecodeError for toml, yaml.YAMLError for yaml, and also include ValueError/UnicodeDecodeError around yaml.safe_dump/loads; keep the same fallback to mask_sensitive_config_text. Update the try/except around tomllib.loads, yaml.safe_load, and yaml.safe_dump so _format_plugin_config_toml and mask_sensitive_config_data still run normally but only parsing/serialization exceptions are caught and routed to the fallback, referencing tomllib.loads, yaml.safe_load, yaml.safe_dump, _format_plugin_config_toml, and mask_sensitive_config_data.tests/test_utils.py (1)
568-598: Avoidimportlib.reloadin tests — it corrupts module identity across the suite.Reloading
framex.repository.providers.basereplaces the module object insys.modules. Other tests that imported classes from this module before this test runs (e.g., the provider tests above) will still hold references to the old module objects, leading to flaky behavior depending on test ordering.monkeypatch.setattralone onbase_module.httpx.Clientis sufficient and properly restored.🧹 Suggested simplification
def test_repository_fetch_json_follows_redirects(monkeypatch): import framex.repository.providers.base as base_module - base_module = importlib.reload(base_module) captured: dict[str, bool] = {}And drop the
import importlibat line 2 if no longer used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_utils.py` around lines 568 - 598, The test test_repository_fetch_json_follows_redirects should not call importlib.reload; remove the importlib.reload(base_module) call and just import framex.repository.providers.base once into base_module, then use monkeypatch.setattr(base_module.httpx, "Client", FakeClient) as already done; also remove the now-unused import importlib if it exists, and keep assertions that RepositoryVersionProvider.fetch_json(...) and captured["follow_redirects"] are True to verify behavior.tests/api/test_proxy.py (1)
12-43: ConsiderSimpleNamespacefor the OAuth test stub.Creating a class on the fly via
type("OAuthConfig", (), {...})()works but is noisier than the idiomatictypes.SimpleNamespace(**{...})already used intests/driver/test_auth.py::fake_oauth. Reusing the existingfake_oauthhelper (exported or duplicated) would also keep the test stubs consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api/test_proxy.py` around lines 12 - 43, Replace the ad-hoc dynamic class in _set_oauth_session with the idiomatic SimpleNamespace (or reuse the existing fake_oauth helper from tests/driver/test_auth.py) so the test stub is consistent; specifically, create a SimpleNamespace with keys provider, jwt_secret, jwt_algorithm, authorization_url, client_id, call_back_url and then monkeypatch.setattr(settings.auth, "oauth", that SimpleNamespace instead of using type("OAuthConfig", ...), keeping the rest of _set_oauth_session (create_auth_session, create_jwt, client.cookies.set) unchanged.
🤖 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/config.py`:
- Line 58: OauthConfig.provider currently defaults to an empty string which ends
up stored as oauth_provider and can cause can_access_repository to silently
fail; fix by making provider explicit or validating it: change the
OauthConfig.provider field to a stricter type (e.g., Literal["github","gitlab"])
or add a model_post_init on OauthConfig that asserts provider is non-empty and
either sets a detected provider (from redirect/URL) or raises a clear validation
error; alternatively, update oauth_callback to avoid stamping an empty string
into the session/JWT and ensure
can_access_repository(auth_payload.get("oauth_provider"), ...) has a fallback
path when provider is missing.
In `@src/framex/driver/auth.py`:
- Around line 119-122: OAuth scope is hardcoded to GitLab-only values which
breaks multi-provider support; update the code to make scope provider-aware by
adding a scope field to OauthConfig (with provider-specific defaults in
model_post_init) and use that field instead of the hardcoded string in the OAuth
URL construction, and likewise parameterize provider-dependent values: switch
username extraction (e.g., use "username" vs "login" in the token/profile
parsing code around the existing username extraction) and adjust
provider-specific error detail strings (the error messages at/around the current
checks) to depend on settings.auth.oauth.provider or the new
OauthConfig.provider so the flow works for GitLab, GitHub, etc.
In `@src/framex/utils/config_docs.py`:
- Around line 113-118: _mask_sensitive_string currently reveals the first and
last two chars for most values which can expose a large fraction of short
secrets; modify _mask_sensitive_string so that if len(value) < 8 it returns a
complete mask (e.g., '*' repeated len(value)) instead of leaking characters,
otherwise keep the existing partial-mask behavior (first 2 + middle stars + last
2); preserve the current empty-value handling and ensure the star count for long
values is adjusted to exactly len(value)-4.
---
Nitpick comments:
In `@src/framex/utils/config_docs.py`:
- Around line 282-293: The current broad except hides unrelated errors; narrow
it to only handle parsing/serialization failures by catching
tomllib.TOMLDecodeError for toml, yaml.YAMLError for yaml, and also include
ValueError/UnicodeDecodeError around yaml.safe_dump/loads; keep the same
fallback to mask_sensitive_config_text. Update the try/except around
tomllib.loads, yaml.safe_load, and yaml.safe_dump so _format_plugin_config_toml
and mask_sensitive_config_data still run normally but only parsing/serialization
exceptions are caught and routed to the fallback, referencing tomllib.loads,
yaml.safe_load, yaml.safe_dump, _format_plugin_config_toml, and
mask_sensitive_config_data.
In `@tests/api/test_proxy.py`:
- Around line 12-43: Replace the ad-hoc dynamic class in _set_oauth_session with
the idiomatic SimpleNamespace (or reuse the existing fake_oauth helper from
tests/driver/test_auth.py) so the test stub is consistent; specifically, create
a SimpleNamespace with keys provider, jwt_secret, jwt_algorithm,
authorization_url, client_id, call_back_url and then
monkeypatch.setattr(settings.auth, "oauth", that SimpleNamespace instead of
using type("OAuthConfig", ...), keeping the rest of _set_oauth_session
(create_auth_session, create_jwt, client.cookies.set) unchanged.
In `@tests/test_utils.py`:
- Around line 568-598: The test test_repository_fetch_json_follows_redirects
should not call importlib.reload; remove the importlib.reload(base_module) call
and just import framex.repository.providers.base once into base_module, then use
monkeypatch.setattr(base_module.httpx, "Client", FakeClient) as already done;
also remove the now-unused import importlib if it exists, and keep assertions
that RepositoryVersionProvider.fetch_json(...) and captured["follow_redirects"]
are True to verify behavior.
🪄 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: 87f6eea0-4050-4793-9151-cb99964fa0cb
📒 Files selected for processing (8)
src/framex/config.pysrc/framex/driver/application.pysrc/framex/driver/auth.pysrc/framex/utils/config_docs.pytests/api/test_proxy.pytests/driver/test_auth.pytests/mock.pytests/test_utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/mock.py
- src/framex/driver/application.py
|
|
||
|
|
||
| class OauthConfig(BaseModel): | ||
| provider: str = "" |
There was a problem hiding this comment.
provider defaults to empty string — verify downstream tolerance.
OauthConfig.provider defaults to "", which means oauth_callback will stamp an empty string into the session/JWT (oauth_provider). can_access_repository(..., auth_payload.get("oauth_provider"), ...) must handle empty/unknown providers gracefully (e.g., fall back to URL-based detection), otherwise misconfigured deployments silently fail the repo access check. Consider making provider required via Literal["github", "gitlab"] or validating it in model_post_init.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framex/config.py` at line 58, OauthConfig.provider currently defaults to
an empty string which ends up stored as oauth_provider and can cause
can_access_repository to silently fail; fix by making provider explicit or
validating it: change the OauthConfig.provider field to a stricter type (e.g.,
Literal["github","gitlab"]) or add a model_post_init on OauthConfig that asserts
provider is non-empty and either sets a detected provider (from redirect/URL) or
raises a clear validation error; alternatively, update oauth_callback to avoid
stamping an empty string into the session/JWT and ensure
can_access_repository(auth_payload.get("oauth_provider"), ...) has a fallback
path when provider is missing.
Comment on lines
119
to
122
| "&scope=read_user%20read_api%20api%20ai_features" | ||
| ) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Hardcoded GitLab OAuth scope breaks multi-provider support.
read_user, read_api, api, and ai_features are GitLab-only scope names; they're invalid for GitHub (which uses user:email, repo, etc.). Since OauthConfig.provider was introduced in this PR, the scope (and likely the username field extraction at line 154 and the GitLab-specific error strings at lines 144/155) should be provider-aware or driven from config. Otherwise the OAuth flow only works for GitLab despite the new provider field.
🔧 Suggested direction
Add a scope field to OauthConfig (with provider-appropriate defaults via model_post_init) and reference it here, e.g.:
- "&scope=read_user%20read_api%20api%20ai_features"
+ f"&scope={quote(settings.auth.oauth.scope)}"Similarly, parameterize username vs login extraction and the error detail strings based on settings.auth.oauth.provider.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framex/driver/auth.py` around lines 119 - 122, OAuth scope is hardcoded
to GitLab-only values which breaks multi-provider support; update the code to
make scope provider-aware by adding a scope field to OauthConfig (with
provider-specific defaults in model_post_init) and use that field instead of the
hardcoded string in the OAuth URL construction, and likewise parameterize
provider-dependent values: switch username extraction (e.g., use "username" vs
"login" in the token/profile parsing code around the existing username
extraction) and adjust provider-specific error detail strings (the error
messages at/around the current checks) to depend on settings.auth.oauth.provider
or the new OauthConfig.provider so the flow works for GitLab, GitHub, etc.
Comment on lines
+113
to
+118
| def _mask_sensitive_string(value: str) -> str: | ||
| if not value: | ||
| return value | ||
| if len(value) <= 4: | ||
| return "****" | ||
| return f"{value[:2]}{'*' * max(len(value) - 4, 4)}{value[-2:]}" |
There was a problem hiding this comment.
Partial-leak masking preserves the first and last two characters.
f"{value[:2]}{'*' * max(len(value) - 4, 4)}{value[-2:]}" deliberately reveals the first and last 2 characters. That's fine for identifying which token is set, but for very short secrets (≤ ~8 chars) it exposes a substantial fraction. If any of the masked fields here can legitimately hold short secrets, consider a hard floor (e.g., mask entirely when len(value) < 8).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framex/utils/config_docs.py` around lines 113 - 118,
_mask_sensitive_string currently reveals the first and last two chars for most
values which can expose a large fraction of short secrets; modify
_mask_sensitive_string so that if len(value) < 8 it returns a complete mask
(e.g., '*' repeated len(value)) instead of leaking characters, otherwise keep
the existing partial-mask behavior (first 2 + middle stars + last 2); preserve
the current empty-value handling and ensure the star count for long values is
adjusted to exactly len(value)-4.
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
New Features
Bug Fixes / Security
Documentation