[stale] [chore] Switch sessions to connections in reads and more #2902

jp-agenta · 2025-11-11T15:25:50Z

[STALE] [chore] Switch sessions to connections in reads and more

CLAassistant · 2025-11-11T15:25:59Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

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.}

web/oss/src/services/human-evaluations/api/index.ts

+    return await axios
+        .get(`${getAgentaApiUrl()}/human-evaluations/${evaluationId}?project_id=${projectId}`)


web/oss/src/services/testsets/api/index.ts

+    const response = await axios.get(
+        `${getAgentaApiUrl()}/testsets/${testsetId}?project_id=${projectId}`,
+    )


To mitigate the SSRF risk, the code must validate and sanitize the user-provided testsetId before using it to construct internal API request URLs. The best fix would be to ensure that testsetId is in a format expected by the backend (for example, a UUID or a strict alphanumeric string), rejecting anything else before making the request. Specifically, in web/oss/src/services/testsets/api/index.ts, before using testsetId in the axios request, add a strict runtime check (using regex) to ensure its safety. If invalid, throw a controlled error or return a value indicating invalid data (mimicking the case of testsetId === null). This prevents SSRF via path traversal, malicious injection, or unwanted endpoint targeting.

Changes to be made:

In web/oss/src/services/testsets/api/index.ts, inside the fetchTestset function, validate testsetId using a regex for UUID or strict alphanumeric/underscore/hyphen (based on what is allowed).

If the validation fails, either throw an error or return the same object as when !testsetId to maintain frontend stability.

You may use a simple regex, e.g., ^[a-zA-Z0-9\-_]+$ for slugs/IDs, or a UUIDv4 regex if testset IDs are UUIDs.

No new package imports are required—use built-in JS regex.

Copilot

Pull Request Overview

This PR switches terminology from "sessions" to "connections" in database reads and more, involving significant refactoring across evaluation models, database migrations, authentication logic, and service layers. The changes also include cleanup of stale/deprecated code and files.

Key Changes

Renamed evaluation-related entities (e.g., EvaluationResult → EvaluationStep, EvaluationMetrics → EvaluationMetric)
Removed entire applications router and models files
Converted async database operations to sync in migration utilities
Removed multiple evaluation SDK test files and examples

Reviewed Changes

Copilot reviewed 119 out of 2283 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
api/oss/src/apis/fastapi/evaluations/models.py	Renamed evaluation entities and updated field names
api/oss/src/apis/fastapi/applications/*.py	Deleted application router, utils, and models files
api/oss/src/apis/fastapi/annotations/*.py	Major rewrite of annotations router and added utils
api/oss/src/init.py	Simplified blocked email/domain checking logic
api/oss/docker/Dockerfile.*	Removed SDK installation and cron setup
api/oss/databases/postgres/migrations/utils.py	Converted async operations to sync
api/oss/databases/postgres/migrations/tracing/versions/*.py	Updated migration scripts
api/oss/databases/postgres/migrations/core/versions/*.py	Deleted multiple migration files
api/oss/databases/postgres/migrations/runner.py	Deleted migration runner
api/entrypoint.py	Reorganized imports and router mounting
api/ee/tests/manual/evaluations/sdk/*.py	Deleted all SDK test files
api/ee/src/utils/permissions.py	Uncommented API key access check function
api/ee/src/services/*.py	Updated service method signatures and database calls
api/ee/src/routers/*.py	Changed parameter names from `organization_id` to `org_id`
api/ee/src/models/extended/*.py	Renamed deprecated model classes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-11T15:27:46Z

api/oss/databases/postgres/migrations/tracing/versions/fd77265d65dc_fix_spans.py

+    span_type_enum = sa.Enum(SpanType, name="tracetype")
+    trace_type_enum = sa.Enum(TraceType, name="spantype")


The enum names are swapped. span_type_enum should be named 'spantype' and trace_type_enum should be named 'tracetype'. This will cause migration failures when attempting to drop these enums.

Suggested change

span_type_enum = sa.Enum(SpanType, name="tracetype")

trace_type_enum = sa.Enum(TraceType, name="spantype")

span_type_enum = sa.Enum(SpanType, name="spantype")

trace_type_enum = sa.Enum(TraceType, name="tracetype")

Copilot · 2025-11-11T15:27:47Z

api/oss/src/apis/fastapi/annotations/router.py

    AnnotationsResponse,
+    AnnotationQueryRequest,
    AnnotationLinkResponse,
+    Annotation,


The Annotation import is duplicated on lines 33 and 48. Remove the duplicate import to maintain clean code.

Copilot · 2025-11-11T15:27:47Z

api/oss/src/apis/fastapi/annotations/router.py

+from oss.src.apis.fastapi.annotations.utils import (
+    AnnotationFlags,
 )


The utils module is imported twice (lines 35-39 and 56-58). Consolidate these imports into a single import block.

Copilot · 2025-11-11T15:27:47Z

api/oss/databases/postgres/migrations/utils.py

                    ).scalar()

-                    if (count or 0) > 0:
+                    if count > 0:


The expression if count > 0 is less clear than the original if (count or 0) > 0. If count is None, this will raise a TypeError. The original code handled this case safely.

Suggested change

if count > 0:

if (count or 0) > 0:

Copilot · 2025-11-11T15:27:47Z

api/ee/src/services/workspace_manager.py

+        if existing_invitation is not None:
            invitation = existing_invitation
-        elif existing_role:
+        else:
            # Create a new invitation
            invitation = await create_invitation(
-                existing_role, project_id, payload.email
-            )
-        else:
-            raise HTTPException(
-                status_code=404,
-                detail="No existing invitation found for the user",
+                payload.roles[0], project_id, payload.email
            )


When no existing invitation is found, the code creates a new invitation using payload.roles[0], but the original code used existing_role from the check. If there's no existing invitation but there is an existing role assignment, this change loses that information and may create incorrect invitations.

Copilot · 2025-11-11T15:27:48Z

api/ee/src/services/workspace_manager.py

-                status_code=409,
-                detail="User is already a member of the workspace",
-            )
+            raise Exception("User is already a member of the workspace")


Changed from HTTPException with status code 409 to a generic Exception. This breaks the API contract by removing the proper HTTP status code and makes error handling less specific for API consumers.

Copilot · 2025-11-11T15:27:48Z

api/ee/src/services/selectors.py

+        raise NoResultFound(f"User with uid {user_uid} not found")

-        user_org_result = await session.execute(
+    async with engine.core_connection() as connection:


The prepare=True parameter is passed to connection.execute() but this parameter doesn't exist in SQLAlchemy's execute method. This will cause runtime errors.

Copilot · 2025-11-11T15:27:48Z

api/ee/src/services/selectors.py

+            )  # type: ignore
        )
+
+        user_org_result = await connection.execute(stmt=stmt, prepare=True)


The prepare=True parameter is passed to connection.execute() but this parameter doesn't exist in SQLAlchemy's execute method. This will cause runtime errors.

Copilot · 2025-11-11T15:27:48Z

api/oss/src/__init__.py

-
-
-async def _is_blocked(email: str) -> bool:
+def _is_blocked(email: str) -> bool:


Changed from async function to sync function but the callers on lines 104, 149, 182, and 219 still use await _is_blocked(). This will cause runtime errors.

chore/switch-sessions-to-connections

71079ed

Copilot AI review requested due to automatic review settings November 11, 2025 15:25

github-advanced-security bot found potential problems Nov 11, 2025

View reviewed changes

Copilot AI reviewed Nov 11, 2025

View reviewed changes

junaway changed the title ~~[STALE] [chore] Switch sessions to connections in reads and more~~ [stale] [chore] Switch sessions to connections in reads and more Nov 14, 2025

@@ -60,7 +60,13 @@
             }
             export const fetchTestset = async (testsetId: string | null) => {
-                if (!testsetId) {
+                // Accept only non-empty string IDs that are alphanumeric, dash, or underscore
+                const VALID_TESTSETID_REGEX = /^[a-zA-Z0-9\-_]+$/;
+                if (
+                    !testsetId ||
+                    typeof testsetId !== "string" ||
+                    !VALID_TESTSETID_REGEX.test(testsetId)
+                ) {
                     return {
                         id: undefined,
                         name: "No Test Set Associated",

		return await axios
		.get(`${getAgentaApiUrl()}/human-evaluations/${evaluationId}?project_id=${projectId}`)

		span_type_enum = sa.Enum(SpanType, name="tracetype")
		trace_type_enum = sa.Enum(TraceType, name="spantype")



		async def _is_blocked(email: str) -> bool:
		def _is_blocked(email: str) -> bool:

[stale] [chore] Switch sessions to connections in reads and more #2902

Are you sure you want to change the base?

[stale] [chore] Switch sessions to connections in reads and more #2902

Uh oh!

Conversation

jp-agenta commented Nov 11, 2025

Uh oh!

CLAassistant commented Nov 11, 2025

Uh oh!

Check failure

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check failure

Uh oh!

Uh oh!

Copilot Autofix

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants