[stale] [poc] Improve trial user upgrade UX with dedicated continuation flow

[jp-agenta]

[POC] feat: Improve trial user upgrade UX with dedicated continuation flow

[CLAassistant]

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

[Copilot]

Pull Request Overview

This pull request represents a major refactoring and cleanup effort focused on simplifying the evaluation system, removing deprecated code, and improving code organization. The changes involve renaming entities (e.g., "Results/Metrics/Queues" to "Steps/Metrics"), removing unused migration files, simplifying authentication logic, and cleaning up test files.

Key changes:

• Refactored evaluation models to use simplified terminology (Step, Metric instead of Result, Metrics, Queue)
• Removed multiple deprecated database migration files and data migration utilities
• Deleted entire application/query routers and models that appear to be legacy code
• Simplified blocked email/domain checking by removing PostHog feature flag integration
• Cleaned up manual test files and notebooks

Reviewed Changes

Copilot reviewed 114 out of 2313 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
api/oss/src/apis/fastapi/evaluations/models.py	Renamed evaluation entities from Results/Metrics/Queues to Steps/Metrics, updated model names and field references
api/oss/src/apis/fastapi/applications/*	Removed legacy application router, models, and utility files entirely
api/oss/src/apis/fastapi/annotations/*	Major refactor adding new utility functions, expanding router with tracing integration
api/oss/src/init.py	Simplified blocked email/domain checking by removing PostHog integration and async logic
api/oss/docker/Dockerfile.*	Removed SDK installation and cron job setup from Docker builds
api/oss/databases/postgres/migrations/utils.py	Changed async database operations to synchronous
api/oss/databases/postgres/migrations/tracing/versions/fd77265d65dc_fix_spans.py	Simplified migration by removing conditional index operations
api/oss/databases/postgres/migrations/core/versions/*	Removed multiple deprecated migration files
api/entrypoint.py	Reorganized imports and router initialization, simplified lifespan function
api/ee/tests/manual/evaluations/sdk/*	Removed all manual test files and notebooks
api/ee/src/services/*	Updated service imports and renamed parameters (organization_id to org_id)
api/ee/src/models/shared_models.py	Consolidated and renamed permissions (e.g., VIEW_APPLICATIONS to VIEW_APPLICATION)

---

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

[Copilot]

The enum names are swapped: span_type_enum is created with name='tracetype' and trace_type_enum is created with name='spantype'. These should be swapped to match their variable names, or the original names should be 'spantype' and 'tracetype' respectively.

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]

Changed from HTTPException with status code 409 to a generic Exception. This loses important HTTP semantics and will not provide proper status codes to API clients. Should remain as HTTPException(status_code=409, detail='...').

[Copilot]

The default database URI has been changed from 'postgresql+asyncpg://...' to 'postgresql://...', removing the async driver. This is inconsistent with the comment on line 18 that uses .replace('+asyncpg', ''), suggesting asyncpg was expected. Verify this change is intentional and update the logic accordingly.

Suggested change

	or "postgresql://username:password@localhost:5432/agenta_oss"
	or "postgresql+asyncpg://username:password@localhost:5432/agenta_oss"

[Copilot]

URL parameters are not properly encoded. The token, email, organization.id, workspace.id, and project_id should be URL-encoded using urllib.parse.quote to prevent injection vulnerabilities and ensure special characters are handled correctly.

[Copilot]

[nitpick] Changed from 'Default Organization' to 'My Default Organization'. While minor, the added 'My' creates inconsistency with the organization type field which is set to 'default'. Consider keeping the original wording for consistency.

Suggested change

	description="My Default Organization",
	description="Default Organization",

[Copilot]

New file introduces an interface class but the __init__ method unconditionally raises NotImplementedError. Abstract base classes should not implement __init__ in this way - instead, allow subclasses to implement their own initialization or make __init__ abstract using @abstractmethod.

To fix this vulnerability, we must validate and sanitize the evaluationTableId before using it to construct an API route.

Best general approach:

• Allow-list the format/content of evaluationTableId. For instance, if IDs are strictly UUIDs, verify using a regex.
• If not UUIDs, then check they only contain allowed characters (e.g., alphanumeric, dashes/underscores).
• If the value doesn't pass validation, avoid making the request or throw an error.

Implementation in this context:

• In useEvaluationRunData, immediately validate evaluationTableId (before using it in the path).
• If validation fails, throw an error or handle gracefully.
• For a robust pattern, create a helper function, e.g., isValidEvaluationId(), and use it before constructing /preview/evaluations/runs/${evaluationTableId}.
• The helper can be defined adjacent to the affected code, and used both for fetchers and at the call-site if needed.
• Imports from external libraries are unnecessary, but if available, could use e.g., validator's isUUID, otherwise a short regex suffices.

Files to edit:

• web/ee/src/lib/hooks/useEvaluationRunData/index.ts

---

To fix the SSRF vulnerability, the values provided as evaluationId (which ultimately come from the user query string) should be validated before being inserted into the request path. The best way is to ensure that only allowed strings are used — for example, if evaluationIds are UUIDs (or otherwise follow a known format), only allow strings matching that format to be passed to backend APIs. In the affected code:

• Before using any evaluationId in a request path (such as /evaluations/${evaluationId}), check if it matches the expected pattern (e.g. using a regex for UUIDs).
• In the codebase, this means sanitizing the evaluationIds array in EvaluationCompareMode before passing it into fetchAllComparisonResults.
• In the API methods that receive evaluationId (such as fetchEvaluation, fetchAllEvaluationScenarios, etc.), validate the ID before including it in the axios request.
• You may want to add a simple isValidEvaluationId() function (e.g., a regex for UUIDs) and filter/sanitize input everywhere necessary.
• The main changes will be in web/ee/src/components/pages/evaluations/evaluationCompare/EvaluationCompare.tsx (where evaluationIds are used), and in web/ee/src/services/evaluations/api/index.ts (where the API requests are made).

---

To fix this problem, we need to ensure that the evaluationId values used to construct the API route are strictly validated as safe, predictable identifiers. Ideally, the input should be checked against a strict format (e.g., UUID or a list of known IDs) before it is used to assemble a URL. The best approach is to validate each element in evaluationIds before calling fetchAllEvaluationScenarios in fetchAllComparisonResults. We can implement a util function (e.g., isValidEvaluationId) that ensures the format is a valid UUID v4 (since uuidv4 is used in imports, suggesting that evaluation IDs are UUIDs). Before proceeding to fetch scenarios, we filter and/or throw on any invalid evaluation IDs.

Update fetchAllComparisonResults in web/ee/src/services/evaluations/api/index.ts to validate evaluationIds. Additionally, restrict usage of these IDs in all downstream API-calling functions. Only allow those that match a safe, expected pattern—here, UUID v4.

We’ll add a helper function for validation (UUID v4 regex), filter/throw on bad values, and edit usage accordingly.

---

To mitigate SSRF, we must ensure that untrusted user input (here, scenarioId) is validated before being interpolated into any remote URL. The safest way is to enforce a strict format on scenario IDs—ideally, they should be UUIDs.

The best fix in this context is to validate scenarioId against the expected pattern (e.g., using a UUID v4 regex) right at the start of updateScenarioStatusRemote, rejecting/ignoring invalid input. If scenarioId fails validation, do not proceed with the fetch. This is defensive programming: even if client/UI controls help, it ensures only well-formed IDs are used in outgoing requests.

Step-by-step changes:

1. In web/ee/src/services/evaluations/workerUtils.ts's updateScenarioStatusRemote:

   • Before using scenarioId, validate it (use regex for UUID v4).
   • If invalid, early return or throw.

2. Add the required regex and validation helper at the top of the file.
   No additional dependencies are needed—native JS regex suffices.

All changes should be within the shown scope of workerUtils.ts; do not alter upstream (other files) logic.

---

To mitigate this vulnerability, user-controlled input should never directly form URL segments for outgoing server requests without validation. The best approach is to allow only expected, safe values for evaluationId. Since evaluationId appears to be a database/resource identifier (presumably a MongoDB ObjectId or similar), we should enforce a validation check prior to interpolating it into the URL.

• If you know the expected format (e.g., a 24-character hex string for MongoDB ObjectId), validate accordingly using a regex, and either reject or sanitize invalid input.
• If the format is not known, restrict it to a conservative character whitelist (e.g., alphanumeric plus -/_).
• Return null or throw an error if validation fails, preventing the unsafe HTTP request from being issued.
• Make the change in fetchLoadEvaluation (in web/ee/src/services/human-evaluations/api/index.ts): validate evaluationId before using it, and fail safely if validation does not pass.
• Optionally, implement the validator inline or just above fetchLoadEvaluation.

To fix the problem in web/ee/src/services/human-evaluations/api/index.ts, you need to stop interpolating the user-provided value (evaluationId) directly into the template string of console.error. Instead, use a static string for the log message and pass any untrusted variable (here, evaluationId) as a separate argument to console.error. This prevents the possibility of user-controlled data being parsed as part of a format string.

How to fix:
On line 93, change

console.error(`Error fetching evaluation ${evaluationId}:`, error)

to

console.error("Error fetching evaluation:", evaluationId, error)

No additional imports or changes are necessary, as you only need to restructure the arguments to console.error.

---

The correct way to mitigate this SSRF vulnerability is to restrict the evaluationTableId so only valid IDs are allowed to be interpolated into the API URL's path. Since it appears to be an identifier for an evaluation (likely a database ID), you should:

• Validate that evaluationTableId conforms to the type/format of a safe evaluation identifier (e.g., a strict alphanumeric string, UUID, MongoDB ObjectId, etc.).
• Reject (or ignore) any value that does not match this pattern, either by returning early or raising an error/logging.
• Ideally, this validation should occur as soon as possible—i.e., in fetchAllLoadEvaluationsScenarios, on the input parameter.
• For the context shown, a simple regex check (e.g., /^[a-fA-F0-9]{24}$/ for Mongo ObjectId, or similar for UUID) should suffice.
• If you prefer, you can define a helper function isValidEvaluationId and use it to guard the axios call.

You only need to modify web/ee/src/services/human-evaluations/api/index.ts, where you should add input validation for evaluationTableId.

The best way to fix this problem is to validate and sanitize all user-provided data before using it to construct URLs for outgoing requests on the server. Specifically:

• Restrict evaluationScenarioId to only permitted values, ideally to those present in the set of known scenario IDs for the current evaluation (coming from the backend).
• In the updateEvaluationScenario API function, before using evaluationScenarioId in the URL, verify it against an allow-list (e.g., check that it matches a strict regex for ID format such as UUID or that it exists in a locally-cached set of valid IDs for the current evaluation).
• Throw or return an error if validation fails, preventing construction and execution of requests with untrusted path data.

The required edit is to add input validation for evaluationScenarioId at the beginning of the updateEvaluationScenario function in web/ee/src/services/human-evaluations/api/index.ts. This would involve:

• Adding a helper function to validate scenario IDs (e.g., check for non-empty strings and valid format).
• Using that function to validate user input before constructing the URL.

No external dependencies are needed, as basic validation can be written inline or with immediate regex.

---