[chore] Clean up and unify (most) git-based routes#4470
Conversation
Rename the wrapper field on revision commit/log request bodies so each matches the snake_case form of its enclosing request class. Outliers fixed: - /workflows/revisions/commit: workflow_revision -> workflow_revision_commit - /workflows/revisions/log: workflow -> workflow_revisions_log - /applications/revisions/log: application -> application_revisions_log - /evaluators/revisions/log: evaluator -> evaluator_revisions_log - /environments/revisions/log: environment -> environment_revisions_log - /queries/revisions/log: query_revisions -> query_revisions_log - /testsets/revisions/log: testset_revision -> testset_revisions_log Updates the FastAPI request models, the routers that read those fields, and the handwritten callers in: - web/packages/agenta-entities/src/workflow/api/api.ts (commit bodies only; query/response uses of workflow_revision remain unchanged) - sdks/python/agenta/sdk/managers/shared.py (history/ahistory log bodies) - 11 workflow acceptance tests + 1 SDK workflow acceptance test (commit + log bodies) Fern-generated clients regenerated separately.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
size:M
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR unifies request body field naming and fork request shapes across six git-backed entity types (workflows, applications, evaluators, queries, testsets, environments). It renames revision commit/log request wrapper fields, adds inline-resolution support to revision resolve endpoints, implements fork service methods for environments and testsets, and updates tests and SDKs to use the new request shapes. ChangesGit-backed entity fork and request field standardization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
junaway
changed the title
Railway Preview Environment
Updated at 2026-06-04T08:13:48.900Z |
…nify-revision-request-fields
…nify-revision-request-fields
There was a problem hiding this comment.
Pull request overview
This PR standardizes the top-level wrapper field names in several revision commit/log request bodies so they match the snake_case form of their enclosing FastAPI request model names, and updates a subset of in-repo callers/tests accordingly.
Changes:
- Renamed request wrapper fields for
/workflows/revisions/commitand multiple*/revisions/logendpoints (workflows, applications, evaluators, environments, queries, testsets). - Updated FastAPI request models + routers to read the renamed wrapper fields.
- Updated handwritten callers (TS frontend workflow API, Python SDK history/ahistory) and acceptance tests to send the new wrapper keys.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| web/packages/agenta-entities/src/workflow/api/api.ts | Updates workflow revision commit request bodies to use workflow_revision_commit. |
| sdks/python/oss/tests/pytest/acceptance/workflows/test_embeds_integration.py | Updates workflow commit acceptance-test payloads to use workflow_revision_commit. |
| sdks/python/agenta/sdk/managers/shared.py | Updates application revisions log payload wrapper to application_revisions_log (sync + async). |
| api/oss/tests/pytest/acceptance/workflows/test_workflow_revisions_queries.py | Updates workflow commit payload wrappers in revision query acceptance tests. |
| api/oss/tests/pytest/acceptance/workflows/test_workflow_revisions_basics.py | Updates workflow commit payload wrapper in basic revisions test. |
| api/oss/tests/pytest/acceptance/workflows/test_workflow_retrieval_info.py | Updates workflow commit payload wrappers in retrieval-info acceptance tests. |
| api/oss/tests/pytest/acceptance/workflows/test_workflow_lineage.py | Updates workflow revisions log payload wrapper to workflow_revisions_log. |
| api/oss/tests/pytest/acceptance/workflows/test_workflow_embeds.py | Updates workflow commit payload wrappers used by embed resolution tests. |
| api/oss/tests/pytest/acceptance/workflows/test_workflow_embeds_string.py | Updates workflow commit payload wrappers used by string-embed tests. |
| api/oss/tests/pytest/acceptance/workflows/test_workflow_embeds_security.py | Updates workflow commit payload wrappers used by embed security tests. |
| api/oss/tests/pytest/acceptance/workflows/test_workflow_embeds_retrieve_resolve.py | Updates workflow commit payload wrappers used by retrieve/resolve embed tests. |
| api/oss/tests/pytest/acceptance/workflows/test_workflow_embeds_legacy.py | Updates workflow commit payload wrappers used by legacy embed tests. |
| api/oss/tests/pytest/acceptance/workflows/test_workflow_embeds_errors.py | Updates workflow commit payload wrappers used by embed error-handling tests. |
| api/oss/tests/pytest/acceptance/workflows/test_workflow_embeds_cross_entity.py | Updates workflow commit payload wrappers used by cross-entity embed tests. |
| api/oss/src/apis/fastapi/workflows/router.py | Reads workflow_revision_commit and workflow_revisions_log from request bodies. |
| api/oss/src/apis/fastapi/workflows/models.py | Renames request model fields to workflow_revision_commit and workflow_revisions_log. |
| api/oss/src/apis/fastapi/testsets/router.py | Reads testset_revisions_log from request body. |
| api/oss/src/apis/fastapi/testsets/models.py | Renames log request wrapper field to testset_revisions_log. |
| api/oss/src/apis/fastapi/queries/router.py | Reads query_revisions_log from request body. |
| api/oss/src/apis/fastapi/queries/models.py | Renames log request wrapper field to query_revisions_log. |
| api/oss/src/apis/fastapi/evaluators/router.py | Reads evaluator_revisions_log from request body. |
| api/oss/src/apis/fastapi/evaluators/models.py | Renames log request wrapper field to evaluator_revisions_log. |
| api/oss/src/apis/fastapi/environments/router.py | Reads environment_revisions_log from request body (also minor formatting change). |
| api/oss/src/apis/fastapi/environments/models.py | Renames log request wrapper field to environment_revisions_log. |
| api/oss/src/apis/fastapi/applications/router.py | Reads application_revisions_log from request body (also minor formatting change). |
| api/oss/src/apis/fastapi/applications/models.py | Renames log request wrapper field to application_revisions_log. |
Comments suppressed due to low confidence (8)
api/oss/src/apis/fastapi/workflows/models.py:251
- This rename makes
POST /workflows/revisions/commitreject the legacy wrapper keyworkflow_revision, which is still used by existing clients in this repo (e.g. generated TS client). To avoid an immediate breaking change, consider accepting the old key as a validation alias while keeping the new field name in the OpenAPI schema.
class WorkflowRevisionCommitRequest(BaseModel):
workflow_revision_commit: WorkflowRevisionCommit = Field(
description=(
"Revision to append to a variant's history. Requires `workflow_variant_id` "
"and optional `message`; `data` carries the new configuration."
),
)
api/oss/src/apis/fastapi/workflows/models.py:369
- This rename makes
POST /workflows/revisions/logreject the legacy wrapper keyworkflow, which is still used by existing clients. If you want a non-breaking transition, accept the old key via a Pydanticvalidation_aliaswhile documenting/serializing the new name.
class WorkflowRevisionsLogRequest(BaseModel):
workflow_revisions_log: WorkflowRevisionsLog = Field(
description=(
"Log query. Supply `workflow_id`, `workflow_variant_id`, or "
"`workflow_revision_id` to scope the log, and an optional `depth`."
),
)
api/oss/src/apis/fastapi/applications/models.py:174
POST /applications/revisions/logwill now reject the legacy wrapper keyapplication. Since existing clients still sendapplication, consider accepting it as avalidation_aliasto prevent a hard break while transitioning toapplication_revisions_log.
class ApplicationRevisionsLogRequest(BaseModel):
"""Request body for `POST /applications/revisions/log`.
Returns the ordered list of revisions committed to a variant, newest first.
Each entry carries commit metadata and the full revision record.
"""
application_revisions_log: ApplicationRevisionsLog = Field(
description=(
"Filter for the log. Typically set `application_variant_id` to list "
"the revision history of a single variant; optionally set "
"`application_revision_id` + `depth` to walk back a bounded number "
"of commits from a specific revision."
),
)
api/oss/src/apis/fastapi/testsets/models.py:280
POST /testsets/revisions/logwill now reject the legacy wrapper keytestset_revision. If backwards compatibility is desired, accept the old key as avalidation_aliaswhile keeping the new field name for documentation.
class TestsetRevisionsLogRequest(BaseModel):
testset_revisions_log: TestsetRevisionsLog = Field(
description="Scope for the log: one of `testset_id`, `testset_variant_id`, or `testset_revision_id`. Optional `depth` limits how far back to walk.",
)
include_testcases: Optional[bool] = Field(
default=None,
description="Include full testcase objects for each returned revision.",
)
api/oss/src/apis/fastapi/workflows/models.py:251
- This rename makes
POST /workflows/revisions/commitreject the legacy wrapper keyworkflow_revision, which is still used by existing clients in this repo (e.g. generated TS client). To avoid an immediate breaking change, consider accepting the old key as a validation alias while keeping the new field name in the OpenAPI schema.
class WorkflowRevisionCommitRequest(BaseModel):
workflow_revision_commit: WorkflowRevisionCommit = Field(
description=(
"Revision to append to a variant's history. Requires `workflow_variant_id` "
"and optional `message`; `data` carries the new configuration."
),
)
api/oss/src/apis/fastapi/workflows/models.py:369
- This rename makes
POST /workflows/revisions/logreject the legacy wrapper keyworkflow, which is still used by existing clients. If you want a non-breaking transition, accept the old key via a Pydanticvalidation_aliaswhile documenting/serializing the new name.
class WorkflowRevisionsLogRequest(BaseModel):
workflow_revisions_log: WorkflowRevisionsLog = Field(
description=(
"Log query. Supply `workflow_id`, `workflow_variant_id`, or "
"`workflow_revision_id` to scope the log, and an optional `depth`."
),
)
api/oss/src/apis/fastapi/applications/models.py:174
POST /applications/revisions/logwill now reject the legacy wrapper keyapplication. Since existing clients still sendapplication, consider accepting it as avalidation_aliasto prevent a hard break while transitioning toapplication_revisions_log.
class ApplicationRevisionsLogRequest(BaseModel):
"""Request body for `POST /applications/revisions/log`.
Returns the ordered list of revisions committed to a variant, newest first.
Each entry carries commit metadata and the full revision record.
"""
application_revisions_log: ApplicationRevisionsLog = Field(
description=(
"Filter for the log. Typically set `application_variant_id` to list "
"the revision history of a single variant; optionally set "
"`application_revision_id` + `depth` to walk back a bounded number "
"of commits from a specific revision."
),
)
api/oss/src/apis/fastapi/testsets/models.py:280
POST /testsets/revisions/logwill now reject the legacy wrapper keytestset_revision. If backwards compatibility is desired, accept the old key as avalidation_aliaswhile keeping the new field name for documentation.
class TestsetRevisionsLogRequest(BaseModel):
testset_revisions_log: TestsetRevisionsLog = Field(
description="Scope for the log: one of `testset_id`, `testset_variant_id`, or `testset_revision_id`. Optional `depth` limits how far back to walk.",
)
include_testcases: Optional[bool] = Field(
default=None,
description="Include full testcase objects for each returned revision.",
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nify-revision-request-fields
junaway
changed the base branch from
feat/add-retrieval-info-to-api-and-sdk
to
release/v0.100.8
junaway
changed the title
dosubot
Bot
added
size:XL
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 44 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
sdks/python/agenta/sdk/managers/shared.py:946
- The request body wrapper key for
POST /applications/revisions/logshould match the FastAPI model (ApplicationRevisionsLogRequest.application_revisions). Usingapplication_revisions_logwill cause a 422 validation error.
sdks/python/agenta/sdk/managers/shared.py:990 - Same issue as the sync
history()method: the request wrapper key should beapplication_revisions(perApplicationRevisionsLogRequest) rather thanapplication_revisions_log, otherwise the async call will 422.
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 46 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
sdks/python/agenta/sdk/managers/shared.py:946
- The
/applications/revisions/logrequest body wrapper was changed toapplication_revisions_log, but the FastAPI model expectsapplication_revisions(seeapi/oss/src/apis/fastapi/applications/models.py), so the SDK call will send an unexpected field and likely return 422.
sdks/python/agenta/sdk/managers/shared.py:990 - Same issue as the sync
history()method: the request body wrapper key should beapplication_revisions, notapplication_revisions_log, to match the API model forPOST /applications/revisions/log.
| | `POST /<entities>/variants/` | `<entity>_variant: <Entity>VariantCreate` | | ||
| | `PUT /<entities>/variants/{id}` | `<entity>_variant: <Entity>VariantEdit` | | ||
| | `POST /<entities>/variants/query` | `<entity>_variant: <Entity>VariantQuery` + `<entity>_refs: List[Reference]` + `<entity>_variant_refs: List[Reference]` | | ||
| | `POST /<entities>/variants/fork` *(all six entities)* | `<entity>_variant: <Entity>VariantFork` + `<entity>_variant_ref: Reference` + `<entity>_revision_ref: Reference` | | ||
|
|
| | `POST /<entities>/` | `<entity>: <Entity>Create` | | ||
| | `PUT /<entities>/{id}` | `<entity>: <Entity>Edit` | | ||
| | `POST /<entities>/query` | `<entity>: <Entity>Query` + `<entity>_refs: List[Reference]` | | ||
| | `POST /<entities>/{id}/fork` *(all six entities)* | `<entity>: <Entity>Fork` | | ||
|
|
| environment_revisions = await self.environments_service.query_environment_revisions( | ||
| project_id=UUID(request.state.project_id), | ||
| # | ||
| environment_revision_query=environment_revision_query_request.environment_revision, | ||
| # | ||
| environment_refs=environment_revision_query_request.environment_refs, | ||
| environment_variant_refs=environment_revision_query_request.environment_variant_refs, | ||
| environment_revision_refs=environment_revision_query_request.environment_revision_refs, | ||
| # | ||
| application_refs=environment_revision_query_request.application_refs, | ||
| references=environment_revision_query_request.references, | ||
| # |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
api/oss/src/apis/fastapi/applications/router.py (1)
1750-1779:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInline resolve should not populate
retrieval_info.When
application_revision_resolve_request.application_revisionis present, the service resolves onlydataand ignores the revision identifiers. This handler still callsbuild_retrieval_info(...), so the response can claim the API retrieved refs that actually just came from the request body.Suggested fix
application_revision, resolution_info = result + retrieval_info = None + if application_revision_resolve_request.application_revision is None: + retrieval_info = build_retrieval_info( + revision=application_revision, + entity_type="application", + ) return ApplicationRevisionResolveResponse( count=1, application_revision=application_revision, resolution_info=resolution_info, - retrieval_info=build_retrieval_info( - revision=application_revision, - entity_type="application", - ), + retrieval_info=retrieval_info, )web/packages/agenta-entities/src/environment/api/api.ts (1)
99-107: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMove this request onto the Fern client instead of extending the raw axios path.
This payload rename is being added inside a direct
axios.post(...)call with axios-styleparams. The repository rule for frontend API code is to go through the Fern-generated client and pass query params viaqueryParams, so this change should land in that wrapper instead of growing the raw HTTP surface.As per coding guidelines, "All new frontend API code must go through the Fern-generated client, not raw axios" and "Pass query params via
{queryParams: {...}}, NOT axios's{params: {...}}."api/oss/src/apis/fastapi/environments/router.py (1)
865-902:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSkip
retrieval_infowhen resolving an inline environment payload.When
environment_revision_resolve_request.environment_revisionis supplied, nothing is fetched from storage, but this response still callsbuild_retrieval_info(...)on the returned model. That makesretrieval_inforeflect caller-supplied ids/slugs rather than an actual retrieval path. Leave it unset for inline mode, or have the service tell the router whether the revision was fetched.api/oss/src/apis/fastapi/evaluators/router.py (1)
1753-1781:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't synthesize
retrieval_infofor inline evaluator resolves.When the request passes
evaluator_revisiondirectly, the service resolves that payload without fetching a stored revision. Buildingretrieval_infofrom the returned model makes the response report a retrieval path that never happened and can echo arbitrary ids/slugs from the request body. Returnretrieval_info=Nonein inline mode.api/oss/src/apis/fastapi/testsets/router.py (1)
1405-1413:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the renamed wrapper field when building the internal commit request.
This helper still instantiates
TestsetRevisionCommitRequestwithtestset_revision_commit=..., but Line 1481 now consumestestset_revision_commit_request.testset_revision. That breaks/revisions/{testset_revision_id}/uploadbefore it reachescommit_testset_revision.Suggested fix
testset_revision_commit_request = TestsetRevisionCommitRequest( - testset_revision_commit=TestsetRevisionCommit( + testset_revision=TestsetRevisionCommit( testset_id=base_revision.testset_id, testset_variant_id=base_revision.testset_variant_id, testset_revision_id=testset_revision_id, data=testset_revision_data, ), include_testcases=include_testcases, )api/oss/src/dbs/postgres/git/dao.py (1)
1469-1538:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis adapter breaks cursor pagination across pages.
apply_windowing()trims the raw revision stream before this application-diff filter runs, and Line 1496 resetsprev_app_revision_idson every call. That makes the first matching revision on each page look like a new deployment change fromNone, so paginated history can return false positives at page boundaries and unstable page sizes. Apply the diff filter before windowing, or persist the last-seen application revision ids in the cursor state. As per coding guidelines, "Use cursor pagination viaWindowing, not page-number pagination."
🧹 Nitpick comments (1)
api/oss/src/apis/fastapi/evaluators/models.py (1)
496-502: ⚡ Quick winUse a request-specific inline resolve payload.
This field advertises the full persisted
EvaluatorRevisionshape even though the endpoint only consumesdata. That couples the public request contract to server-managed revision fields that are ignored at runtime and can force callers to fabricate ids/version metadata just to resolve an ad-hoc payload. A small request DTO with a requireddatafield would keep the wire contract aligned with the implementation. Based on learnings: "Define explicit request/response models inmodels.py."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0db92667-5c80-4fad-bce0-6d64361b9dca
📒 Files selected for processing (46)
api/oss/src/apis/fastapi/applications/models.pyapi/oss/src/apis/fastapi/applications/router.pyapi/oss/src/apis/fastapi/environments/models.pyapi/oss/src/apis/fastapi/environments/router.pyapi/oss/src/apis/fastapi/evaluators/models.pyapi/oss/src/apis/fastapi/evaluators/router.pyapi/oss/src/apis/fastapi/queries/models.pyapi/oss/src/apis/fastapi/queries/router.pyapi/oss/src/apis/fastapi/testsets/models.pyapi/oss/src/apis/fastapi/testsets/router.pyapi/oss/src/apis/fastapi/workflows/models.pyapi/oss/src/apis/fastapi/workflows/router.pyapi/oss/src/core/applications/service.pyapi/oss/src/core/environments/dtos.pyapi/oss/src/core/environments/service.pyapi/oss/src/core/evaluators/service.pyapi/oss/src/core/queries/service.pyapi/oss/src/core/testsets/dtos.pyapi/oss/src/core/testsets/service.pyapi/oss/src/core/workflows/service.pyapi/oss/src/dbs/postgres/git/dao.pyapi/oss/tests/pytest/acceptance/applications/test_application_retrieval_info.pyapi/oss/tests/pytest/acceptance/applications/test_application_variants_and_revisions.pyapi/oss/tests/pytest/acceptance/environments/test_environment_retrieval_info.pyapi/oss/tests/pytest/acceptance/evaluators/test_evaluator_retrieval_info.pyapi/oss/tests/pytest/acceptance/legacy_variants/test_legacy_configs_fetch_acceptance.pyapi/oss/tests/pytest/acceptance/loadables/test_loadable_strategies.pyapi/oss/tests/pytest/acceptance/queries/test_queries_basics.pyapi/oss/tests/pytest/acceptance/queries/test_query_retrieval_info.pyapi/oss/tests/pytest/acceptance/test_revision_commit_extra_forbid.pyapi/oss/tests/pytest/acceptance/test_revision_logs.pyapi/oss/tests/pytest/acceptance/test_revision_retrieve_ambiguous_400.pyapi/oss/tests/pytest/acceptance/test_revision_retrieve_env_path.pyapi/oss/tests/pytest/acceptance/test_revision_retrieve_inconsistent_400.pyapi/oss/tests/pytest/acceptance/test_revision_retrieve_positive.pyapi/oss/tests/pytest/acceptance/testsets/test_testset_retrieval_info.pyapi/oss/tests/pytest/acceptance/tracing/test_traces_preview.pyapi/oss/tests/pytest/acceptance/workflows/test_workflow_embeds_cross_entity.pyapi/oss/tests/pytest/acceptance/workflows/test_workflow_lineage.pyapi/oss/tests/pytest/acceptance/workflows/test_workflow_retrieval_info.pyapi/oss/tests/pytest/unit/environments/test_commit_validation.pydocs/designs/git-entities/request-body-fields.mddocs/designs/git-entities/tasks.mdsdks/python/agenta/sdk/managers/shared.pyweb/oss/src/components/DeploymentsDashboard/store/deploymentStore.tsweb/packages/agenta-entities/src/environment/api/api.ts
| if application_variant_id: | ||
| if ( | ||
| fork_request.application_variant_id | ||
| and fork_request.application_variant_id != application_variant_id | ||
| application_variant_ref.id | ||
| and application_variant_ref.id != application_variant_id | ||
| ): | ||
| return ApplicationVariantResponse() | ||
|
|
||
| if not fork_request.application_variant_id: | ||
| fork_request.application_variant_id = application_variant_id | ||
| if not application_variant_ref.id: | ||
| application_variant_ref = Reference(id=application_variant_id) |
There was a problem hiding this comment.
Return 400 for conflicting source-variant IDs.
Line 1072 turns contradictory inputs into 200 {count: 0}. That makes a malformed request look like an empty result set instead of a client error.
Suggested fix
if application_variant_id:
if (
application_variant_ref.id
and application_variant_ref.id != application_variant_id
):
- return ApplicationVariantResponse()
+ raise HTTPException(
+ status_code=status.HTTP_400_BAD_REQUEST,
+ detail="application_variant_id does not match application_variant_ref.id.",
+ )
if not application_variant_ref.id:
application_variant_ref = Reference(id=application_variant_id)📝 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.
| if application_variant_id: | |
| if ( | |
| fork_request.application_variant_id | |
| and fork_request.application_variant_id != application_variant_id | |
| application_variant_ref.id | |
| and application_variant_ref.id != application_variant_id | |
| ): | |
| return ApplicationVariantResponse() | |
| if not fork_request.application_variant_id: | |
| fork_request.application_variant_id = application_variant_id | |
| if not application_variant_ref.id: | |
| application_variant_ref = Reference(id=application_variant_id) | |
| if application_variant_id: | |
| if ( | |
| application_variant_ref.id | |
| and application_variant_ref.id != application_variant_id | |
| ): | |
| raise HTTPException( | |
| status_code=status.HTTP_400_BAD_REQUEST, | |
| detail="application_variant_id does not match application_variant_ref.id.", | |
| ) | |
| if not application_variant_ref.id: | |
| application_variant_ref = Reference(id=application_variant_id) |
| class TestsetRevisionCommitRequest(BaseModel): | ||
| testset_revision_commit: TestsetRevisionCommit = Field( | ||
| testset_revision: TestsetRevisionCommit = Field( | ||
| description="New revision to commit. Pass either `data` (full replacement of the testcase list) or `delta` (add/remove/replace operations against the base revision) — not both.", |
There was a problem hiding this comment.
Restore the standardized wrapper keys for testset commit/log bodies.
Line 239 and Line 295 rename these payload keys to testset_revision / testset_revisions, but the contract this PR is standardizing is the snake_case form of the enclosing request type. Leaving testsets on the short names will make this surface reject callers that send testset_revision_commit / testset_revisions_log.
Suggested fix
class TestsetRevisionCommitRequest(BaseModel):
- testset_revision: TestsetRevisionCommit = Field(
+ testset_revision_commit: TestsetRevisionCommit = Field(
description="New revision to commit. Pass either `data` (full replacement of the testcase list) or `delta` (add/remove/replace operations against the base revision) — not both.",
)
@@
class TestsetRevisionsLogRequest(BaseModel):
- testset_revisions: TestsetRevisionsLog = Field(
+ testset_revisions_log: TestsetRevisionsLog = Field(
description="Scope for the log: one of `testset_id`, `testset_variant_id`, or `testset_revision_id`. Optional `depth` limits how far back to walk.",
)Also applies to: 294-296
| async def fork_environment_variant( | ||
| self, | ||
| *, | ||
| project_id: UUID, | ||
| user_id: UUID, | ||
| # | ||
| environment_variant_fork: EnvironmentVariantFork, | ||
| environment_variant_ref: Reference, | ||
| environment_revision_ref: Optional[Reference] = None, | ||
| ) -> Optional[EnvironmentVariant]: | ||
| _artifact_fork = ArtifactFork( | ||
| variant_id=environment_variant_ref.id, | ||
| revision_id=environment_revision_ref.id | ||
| if environment_revision_ref | ||
| else None, | ||
| variant=environment_variant_fork, | ||
| ) | ||
|
|
||
| variant = await self.environments_dao.fork_variant( | ||
| project_id=project_id, | ||
| user_id=user_id, | ||
| # | ||
| artifact_fork=_artifact_fork, | ||
| ) |
There was a problem hiding this comment.
Preserve slug/version-based source refs in the fork path.
Line 664 drops environment_variant_ref and environment_revision_ref down to .id only. That breaks valid requests that identify the source variant or revision by slug/version, because the DAO now receives None for those cases instead of a resolvable source.
Suggested fix
async def fork_environment_variant(
self,
*,
project_id: UUID,
user_id: UUID,
#
environment_variant_fork: EnvironmentVariantFork,
environment_variant_ref: Reference,
environment_revision_ref: Optional[Reference] = None,
) -> Optional[EnvironmentVariant]:
+ source_variant = await self.fetch_environment_variant(
+ project_id=project_id,
+ environment_variant_ref=environment_variant_ref,
+ )
+ if not source_variant:
+ return None
+
+ source_revision_id: Optional[UUID] = None
+ if environment_revision_ref is not None:
+ source_revision = await self.fetch_environment_revision(
+ project_id=project_id,
+ environment_variant_ref=Reference(id=source_variant.id),
+ environment_revision_ref=environment_revision_ref,
+ )
+ if not source_revision:
+ return None
+ source_revision_id = source_revision.id
+
_artifact_fork = ArtifactFork(
- variant_id=environment_variant_ref.id,
- revision_id=environment_revision_ref.id
- if environment_revision_ref
- else None,
+ variant_id=source_variant.id,
+ revision_id=source_revision_id,
variant=environment_variant_fork,
)| if evaluator_revision is not None: | ||
| if not evaluator_revision.data: | ||
| return None | ||
| ( | ||
| resolved_data, | ||
| resolution_info, | ||
| ) = await self.embeds_service.resolve_configuration( | ||
| project_id=project_id, | ||
| configuration=evaluator_revision.data.model_dump(mode="json"), | ||
| max_depth=max_depth, | ||
| max_embeds=max_embeds, | ||
| error_policy=ErrorPolicy(error_policy), | ||
| include_archived=include_archived, | ||
| ) | ||
| evaluator_revision.data = EvaluatorRevisionData(**resolved_data) | ||
| return (evaluator_revision, resolution_info) |
There was a problem hiding this comment.
Don't treat an invalid inline resolve payload as “not found”.
If evaluator_revision is present but data is missing, this branch returns None. The routers then serialize that as an empty 200 response, so malformed inline input becomes indistinguishable from a missing revision. Raise a domain validation exception here instead so the API boundary can return a 400. As per coding guidelines: "Define appropriate service exceptions" and "Include structured context in domain exceptions (not just a message string) so the router can build rich HTTP error responses."
| _artifact_fork = ArtifactFork( | ||
| **query_fork.model_dump(mode="json"), | ||
| variant_id=query_variant_ref.id, | ||
| revision_id=query_revision_ref.id if query_revision_ref else None, | ||
| variant=query_variant_fork, | ||
| ) |
There was a problem hiding this comment.
Resolve Reference inputs before extracting fork IDs.
Line 591 and Line 592 only copy .id from the incoming refs. That breaks fork requests that identify the source variant by slug or the source revision by slug/version, even though this method still advertises Reference inputs. Resolve the refs first (or make this API explicitly UUID-only) before building ArtifactFork.
| _artifact_fork = ArtifactFork( | ||
| variant_id=testset_variant_ref.id, | ||
| revision_id=testset_revision_ref.id if testset_revision_ref else None, | ||
| variant=testset_variant_fork, |
There was a problem hiding this comment.
Don't collapse source refs to .id in the new fork path.
Line 501 and Line 502 discard every selector except id, so a valid Reference(slug=...) source variant or Reference(version=...) source revision can no longer be forked from this service. Resolve the refs to concrete IDs first, or tighten the contract so this method only accepts UUIDs.
| _artifact_fork = ArtifactFork( | ||
| **workflow_fork.model_dump(mode="json"), | ||
| variant_id=workflow_variant_ref.id, | ||
| revision_id=workflow_revision_ref.id if workflow_revision_ref else None, | ||
| variant=workflow_variant_fork, |
There was a problem hiding this comment.
Preserve full Reference semantics when forking workflows.
Line 970 and Line 971 throw away slug/version selectors and keep only .id. That regresses any caller that forks from a slug-only variant ref or a version-based revision ref, and it also propagates into application/evaluator fork flows that now delegate here.
| | `POST /<entities>/revisions/retrieve` | `<entity>_ref: Reference` + `<entity>_variant_ref: Reference` + `<entity>_revision_ref: Reference` + `environment_ref: Reference` + `environment_variant_ref: Reference` + `environment_revision_ref: Reference` + `key: str` + `resolve: bool` *(queries, testsets, environments drop the `environment_*` triple and `key`)* | | ||
| | `POST /<entities>/revisions/resolve` *(workflows, applications, evaluators, environments)* | `<entity>_ref: Reference` + `<entity>_variant_ref: Reference` + `<entity>_revision_ref: Reference` + `<entity>_revision: <Entity>Revision` + `max_depth: int` + `max_embeds: int` + `error_policy: ErrorPolicy` | |
There was a problem hiding this comment.
Clarify retrieve exceptions to include resolve omission for queries/testsets.
The generic retrieve rule is broader than the concrete class listings: queries/testsets are shown later without resolve. Please update this sentence so the generic contract matches the per-entity definitions.
Suggested doc fix
-| `POST /<entities>/revisions/retrieve` | `<entity>_ref: Reference` + `<entity>_variant_ref: Reference` + `<entity>_revision_ref: Reference` + `environment_ref: Reference` + `environment_variant_ref: Reference` + `environment_revision_ref: Reference` + `key: str` + `resolve: bool` *(queries, testsets, environments drop the `environment_*` triple and `key`)* |
+| `POST /<entities>/revisions/retrieve` | `<entity>_ref: Reference` + `<entity>_variant_ref: Reference` + `<entity>_revision_ref: Reference` + `environment_ref: Reference` + `environment_variant_ref: Reference` + `environment_revision_ref: Reference` + `key: str` + `resolve: bool` *(queries/testsets drop `environment_*`, `key`, and `resolve`; environments drop `key` only)* |📝 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.
| | `POST /<entities>/revisions/retrieve` | `<entity>_ref: Reference` + `<entity>_variant_ref: Reference` + `<entity>_revision_ref: Reference` + `environment_ref: Reference` + `environment_variant_ref: Reference` + `environment_revision_ref: Reference` + `key: str` + `resolve: bool` *(queries, testsets, environments drop the `environment_*` triple and `key`)* | | |
| | `POST /<entities>/revisions/resolve` *(workflows, applications, evaluators, environments)* | `<entity>_ref: Reference` + `<entity>_variant_ref: Reference` + `<entity>_revision_ref: Reference` + `<entity>_revision: <Entity>Revision` + `max_depth: int` + `max_embeds: int` + `error_policy: ErrorPolicy` | | |
| | `POST /<entities>/revisions/retrieve` | `<entity>_ref: Reference` + `<entity>_variant_ref: Reference` + `<entity>_revision_ref: Reference` + `environment_ref: Reference` + `environment_variant_ref: Reference` + `environment_revision_ref: Reference` + `key: str` + `resolve: bool` *(queries/testsets drop `environment_*`, `key`, and `resolve`; environments drop `key` only)* | | |
| | `POST /<entities>/revisions/resolve` *(workflows, applications, evaluators, environments)* | `<entity>_ref: Reference` + `<entity>_variant_ref: Reference` + `<entity>_revision_ref: Reference` + `<entity>_revision: <Entity>Revision` + `max_depth: int` + `max_embeds: int` + `error_policy: ErrorPolicy` | |
| - [ ] **Fork parity — artifact level**: add `POST /<entities>/{id}/fork` to `queries`, `testsets`, and `environments` so all six git-backed entities have artifact-level fork. Requires `QueryFork` / `TestsetFork` / `EnvironmentFork` DTOs + `QueryForkRequest` / `TestsetForkRequest` / `EnvironmentForkRequest` models + router handlers + service methods. | ||
|
|
||
| - [ ] **Fork parity — variant level**: add `POST /<entities>/variants/fork` to all six git-backed entities. Currently only evaluators and queries have it; workflows, applications, testsets, and environments are missing it. | ||
|
|
There was a problem hiding this comment.
Task status is internally inconsistent between “Open” and “Done”.
These sections currently claim the same fork-parity work is both pending and completed. Please reconcile them so one source of truth remains for rollout tracking.
Also applies to: 47-50
|
|
||
| ## Done | ||
|
|
||
| - [x] **Fork parity — artifact level**: `QueryForkRequest`, `TestsetForkRequest`, `EnvironmentForkRequest` + DTOs (`TestsetFork`, `EnvironmentFork`) + service methods + router endpoints added. All six entities now have `POST /variants/fork`. |
There was a problem hiding this comment.
Artifact-level endpoint path is incorrect in the completion note.
For artifact-level parity, the route should be POST /<entities>/{id}/fork, not POST /variants/fork (variant-level path).
Suggested doc fix
-- [x] **Fork parity — artifact level**: `QueryForkRequest`, `TestsetForkRequest`, `EnvironmentForkRequest` + DTOs (`TestsetFork`, `EnvironmentFork`) + service methods + router endpoints added. All six entities now have `POST /variants/fork`.
+- [x] **Fork parity — artifact level**: `QueryForkRequest`, `TestsetForkRequest`, `EnvironmentForkRequest` + DTOs (`TestsetFork`, `EnvironmentFork`) + service methods + router endpoints added. All six entities now have `POST /<entities>/{id}/fork`.📝 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.
| - [x] **Fork parity — artifact level**: `QueryForkRequest`, `TestsetForkRequest`, `EnvironmentForkRequest` + DTOs (`TestsetFork`, `EnvironmentFork`) + service methods + router endpoints added. All six entities now have `POST /variants/fork`. | |
| - [x] **Fork parity — artifact level**: `QueryForkRequest`, `TestsetForkRequest`, `EnvironmentForkRequest` + DTOs (`TestsetFork`, `EnvironmentFork`) + service methods + router endpoints added. All six entities now have `POST /<entities>/{id}/fork`. |
No description provided.