[feature] Add usage limits to agenta-provided keys #2905
Conversation
release/v0.60.2
release/v0.61.0
release/v0.61.1
release/v0.61.2
release/v0.62.0
There was a problem hiding this comment.
Pull Request Overview
This pull request appears to be focused on code cleanup and refactoring, primarily removing deprecated features and simplifying type definitions. The main changes involve removing "custom" evaluation types and eliminating unused code related to testsets, evaluations, and various SDK managers.
Key Changes:
- Removed support for "custom" evaluation type, consolidating to "auto", "human", and "online" types only
- Eliminated entire modules for testsets, evaluations, applications, evaluators management
- Cleaned up SDK dependencies and removed unused imports
- Simplified workflow interfaces by removing configuration-related code
- Reorganized import paths and removed commented-out code
Reviewed Changes
Copilot reviewed 139 out of 546 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| web/ee/src/components/EvalRunDetails/* | Removed "custom" eval type references throughout evaluation components |
| sdk/pyproject.toml | Downgraded version from 0.62.0 to 0.60.0 and removed python-jsonpath dependency |
| sdk/agenta/sdk/workflows/* | Refactored parameter handling, removed template expansion logic |
| sdk/agenta/sdk/managers/* | Deleted entire manager modules for testsets, evaluators, applications |
| sdk/agenta/sdk/models/* | Removed model definitions for testsets, evaluations, git entities |
| sdk/agenta/sdk/evaluations/* | Deleted complete evaluation utilities and preview functionality |
| sdk/agenta/sdk/decorators/* | Simplified workflow decorators, removed invoke/inspect helper functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| structlog = "^25.2.0" | ||
| huggingface-hub = "<0.31.0" | ||
| restrictedpython = { version = "^8.0", python = ">=3.11,<3.14" } | ||
| restrictedpython = { version = "^8.0", python = ">=3.11,<3.12" } |
There was a problem hiding this comment.
The Python version constraint has been narrowed from '>=3.11,<3.14' to '>=3.11,<3.12', which reduces compatibility. This may break installations for Python 3.12 and 3.13 users. Consider if this restriction is intentional.
| restrictedpython = { version = "^8.0", python = ">=3.11,<3.12" } | |
| restrictedpython = { version = "^8.0", python = ">=3.11,<3.14" } |
| if not uri: | ||
| return True | ||
|
|
||
| return False |
There was a problem hiding this comment.
The return value changed from 'True' to 'False' when uri is None in is_custom_uri(). This logic change may break existing functionality that expects None URIs to be treated as custom. Verify this is the intended behavior.
| return False | |
| return True |
| def echo( | ||
| *, | ||
| slug: Optional[str] = None, | ||
| slug: str, |
There was a problem hiding this comment.
The 'slug' parameter has been changed from Optional[str] to required (str) across all workflow functions. This is a breaking API change that will require all callers to provide a slug value. Ensure this breaking change is documented and necessary.
|
|
||
| try: | ||
| if not request.url.path in _ALWAYS_ALLOW_LIST: | ||
| await self._allow_local_secrets(credentials) |
There was a problem hiding this comment.
A new authentication check has been added that validates credentials before allowing access to secrets. However, the exception handling at line 126-128 catches DenyException but only prints the error without taking action. Consider whether secrets should still be allowed when authentication fails.
| self, | ||
| # | ||
| slug: Optional[str] = None, | ||
| slug: str, |
There was a problem hiding this comment.
The 'slug' parameter in the application class has changed from Optional[str] to required (str). This is a breaking change that requires all application instantiations to provide a slug. Document this breaking change in migration notes.
| self, | ||
| # | ||
| slug: Optional[str] = None, | ||
| slug: str, |
There was a problem hiding this comment.
The 'slug' parameter in the evaluator class has changed from Optional[str] to required (str). This is a breaking change that requires all evaluator instantiations to provide a slug. Document this breaking change in migration notes.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -84,7 +84,7 @@ | |||
|
|
|||
| return {scenarioCount: scenarioIds.length} | |||
| } catch (error) { | |||
| console.error("[refreshLiveEvaluationRun] Failed to refresh run %s", runId, error) | |||
| console.error(`[refreshLiveEvaluationRun] Failed to refresh run ${runId}`, error) | |||
Check failure
Code scanning / CodeQL
Use of externally-controlled format string High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 months ago
To prevent possible exploitation from untrusted input in logging functions, avoid placing user-controlled data directly inside template literals used as the "format" string of console.error or similar methods. Instead, use a constant message as the format string and pass the untrusted data as separate arguments, which ensures they're rendered as literals, and prevents unexpected formatter injection by the user.
In web/ee/src/lib/hooks/useEvaluationRunData/refreshLiveRun.ts, update the problematic console.error call on line 87. Rather than interpolating runId, supply it as a second argument (e.g., "Failed to refresh run", runId, error). There is no need to add any imports. Only a single line needs editing; the rest of the functionality remains unchanged.
| @@ -84,7 +84,7 @@ | ||
|
|
||
| return {scenarioCount: scenarioIds.length} | ||
| } catch (error) { | ||
| console.error(`[refreshLiveEvaluationRun] Failed to refresh run ${runId}`, error) | ||
| console.error("[refreshLiveEvaluationRun] Failed to refresh run %s", runId, error) | ||
| throw error | ||
| } | ||
| } |
|
GitHub CI seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
This is stale. I reopened a clean PR here: #2957 |
[feature] Add usage limits to agenta-provided keys