[feat] Resolve broken references in traces#4469
Conversation
Previously RetrievalInfo lived under api/oss/src/apis/fastapi/git/ and the routers assembled it directly, including making duplicate environment-revision DB calls to gather environment-side references. This commit moves the DTO and helpers to api/oss/src/core/git/ for symmetry with ResolutionInfo, threads retrieval_info through the services as the third element of a (revision, resolution_info, retrieval_info) tuple, and lets the routers act as pass-throughs. Highlights: - core/git/dtos.py: RetrievalInfo Pydantic model - core/git/utils.py: build_retrieval_info / revision_references helpers - Six services updated to return the 3-tuple; environment-backed paths merge environment + target references inside the service, no router-level DB call - Queries and testsets gain thin retrieve_*_revision wrappers for symmetry - /revisions/retrieve and /revisions/resolve responses carry retrieval_info - Unit + acceptance test coverage for all six entities, both retrieve and resolve, direct and environment-backed No request-field renames in this commit; only the internal refactor and the new retrieval_info on retrieve/resolve responses.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
size:XL
junaway
changed the title
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds retrieval provenance metadata to revision retrieval responses across multiple entity types (applications, environments, evaluators, workflows, queries, testsets). A new ChangesRetrieval Info Feature
Python SDK Resolver Middleware Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
@mmabrouk |
Railway Preview Environment
Updated at 2026-05-31T11:47:40.501Z |
![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 (2)
api/oss/src/core/applications/service.py (1)
595-627:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDerive the environment selector key before lookup.
This branch now returns
None, None, Nonewheneverkeyis omitted, even though the API contract inapi/oss/src/apis/fastapi/applications/models.py(Lines 403-409) says the server derives it fromapplication_ref. That breaks documented environment-backed retrievals like{application_slug}.revision.Suggested fix
environment_retrieval_info: Optional[RetrievalInfo] = None is_environment_backed = bool( environment_ref or environment_variant_ref or environment_revision_ref ) if is_environment_backed: + if key is None and application_ref and application_ref.slug: + key = f"{application_ref.slug}.revision" + environments_service = self.workflows_service.environments_service if not environments_service: return None, None, Noneapi/oss/src/core/workflows/service.py (1)
1153-1233: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftReplace the positional 3-tuple with a typed result DTO.
This expands a brittle unpack-by-position contract across layers, and callers are already discarding elements positionally in
_ensure_request_revision(). A dedicatedBaseModelresult would make the contract self-describing and safer to evolve.As per coding guidelines, Service methods must return typed DTOs (Pydantic
BaseModelsubclasses), not raw dicts, tuples, orAny.
🧹 Nitpick comments (2)
api/oss/tests/pytest/unit/applications/test_router.py (1)
195-206: ⚡ Quick winAssert the router actually delegates to
applications_service.retrieve_application_revision.This test validates the returned payload, but not the service invocation itself. Add an explicit await assertion to protect the router-to-service contract.
Proposed test hardening
assert response.retrieval_info is not None assert response.retrieval_info.key == "demo-app.revision" assert response.retrieval_info.references["environment"].id == environment_id assert response.retrieval_info.references["environment_revision"].version == "7" assert ( response.retrieval_info.references["application_revision"].id == application_revision_id ) + applications_service.retrieve_application_revision.assert_awaited_once() # The router no longer does its own env round-trip; the applications service # owns environment lookup and reference assembly. environments_service.retrieve_environment_revision.assert_not_awaited()api/oss/src/apis/fastapi/queries/models.py (1)
3-3: ⚡ Quick winDocument the new
retrieval_infofield in the public schema.Line 152 adds a new response field, but unlike the other
retrieval_infoadditions in this PR it has noField(...)metadata, so the OpenAPI schema won't explain what clients should expect here.Suggested patch
-from pydantic import BaseModel +from pydantic import BaseModel, Field @@ class QueryRevisionResponse(BaseModel): count: int = 0 query_revision: Optional[QueryRevision] = None - retrieval_info: Optional[RetrievalInfo] = None + retrieval_info: Optional[RetrievalInfo] = Field( + default=None, + description="References used to retrieve the top-level revision.", + )Also applies to: 149-152
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c0dd2d2d-ca02-4739-a11b-d11dbc5678e5
📒 Files selected for processing (45)
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/legacy_variants/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/service.pyapi/oss/src/core/evaluators/service.pyapi/oss/src/core/git/dtos.pyapi/oss/src/core/git/utils.pyapi/oss/src/core/queries/service.pyapi/oss/src/core/testsets/service.pyapi/oss/src/core/workflows/service.pyapi/oss/tests/pytest/acceptance/applications/test_application_retrieval_info.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/queries/test_query_retrieval_info.pyapi/oss/tests/pytest/acceptance/testsets/test_testset_retrieval_info.pyapi/oss/tests/pytest/acceptance/workflows/test_workflow_retrieval_info.pyapi/oss/tests/pytest/unit/applications/__init__.pyapi/oss/tests/pytest/unit/applications/test_retrieve_resolve.pyapi/oss/tests/pytest/unit/applications/test_router.pyapi/oss/tests/pytest/unit/environments/__init__.pyapi/oss/tests/pytest/unit/environments/test_retrieve_resolve.pyapi/oss/tests/pytest/unit/evaluators/test_retrieve_resolve.pyapi/oss/tests/pytest/unit/git/__init__.pyapi/oss/tests/pytest/unit/git/test_retrieval_info_utils.pyapi/oss/tests/pytest/unit/legacy_variants/test_configs_fetch.pyapi/oss/tests/pytest/unit/queries/__init__.pyapi/oss/tests/pytest/unit/queries/test_retrieve.pyapi/oss/tests/pytest/unit/testsets/__init__.pyapi/oss/tests/pytest/unit/testsets/test_retrieve.pyapi/oss/tests/pytest/unit/workflows/__init__.pyapi/oss/tests/pytest/unit/workflows/test_flag_ownership.pyapi/oss/tests/pytest/unit/workflows/test_retrieve_resolve.pysdks/python/agenta/sdk/middlewares/running/resolver.pysdks/python/oss/tests/pytest/utils/test_resolver_middleware.py
Resolved conflicts by keeping both import sets (RetrievalInfo/build_retrieval_info from HEAD and validate_* functions from incoming) across all service and router files. In environments/service.py, adopted the incoming simplification of retrieve_environment_revision to delegate to fetch_environment_revision (which includes the full validation set), restored build_retrieval_info call and fixed the incorrect 2-tuple return to 3-tuple. In the SDK test file, kept HEAD's test_stores_retrieval_references_on_tracing_context method and appended the incoming TestResolverMiddlewareTracingParameters class. Dropped unused VariantForkError import from all three routers (now handled by handle_git_exceptions). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Refactor of the six revisioned services (workflows, applications, evaluators, environments, queries, testsets) to thread a new RetrievalInfo (third element of the return tuple) alongside the existing (revision, resolution_info). Environment-backed lookups now merge environment + target references inside the service, removing a duplicate retrieve_environment_revision call in the routers. retrieval_info is exposed on /revisions/retrieve and /revisions/resolve responses for all six entities. The SDK resolver middleware learns a resolve_references_with_info variant that merges returned references into the tracing context.
Changes:
- Move
RetrievalInfoDTO intocore/git, addbuild_retrieval_info/revision_referencesutilities, and addretrieval_infofields to the relevant FastAPI response models. - Update all six services and their routers (plus legacy-variants pass-throughs) to produce/consume the 3-tuple, and add
retrieve_query_revision/retrieve_testset_revisionwrappers. - Extend SDK resolver middleware to merge server-provided retrieval references into
TracingContext, and add unit + acceptance tests across all six entities.
Reviewed changes
Copilot reviewed 39 out of 45 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| api/oss/src/core/git/dtos.py | Adds RetrievalInfo DTO with references + key. |
| api/oss/src/core/git/utils.py | New build_retrieval_info / revision_references helpers. |
| api/oss/src/core/workflows/service.py | Returns (revision, resolution_info, retrieval_info); merges env refs when env-backed. |
| api/oss/src/core/applications/service.py | Same triple-tuple refactor for applications. |
| api/oss/src/core/evaluators/service.py | Same triple-tuple refactor for evaluators. |
| api/oss/src/core/environments/service.py | Returns triple and removes stale commented log. |
| api/oss/src/core/queries/service.py | New retrieve_query_revision wrapper emitting retrieval_info. |
| api/oss/src/core/testsets/service.py | New retrieve_testset_revision wrapper emitting retrieval_info. |
| api/oss/src/apis/fastapi/{workflows,applications,evaluators,environments,queries,testsets}/router.py | Pass retrieval_info through; resolve endpoints synthesize it via build_retrieval_info. |
| api/oss/src/apis/fastapi/{workflows,applications,evaluators,environments,queries,testsets}/models.py | Add retrieval_info field to response models. |
| api/oss/src/apis/fastapi/legacy_variants/router.py | Updated all unpacks to the 3-tuple shape. |
| sdks/python/agenta/sdk/middlewares/running/resolver.py | New resolve_references_with_info + _merge_tracing_references; existing resolve_references becomes a thin wrapper. |
| api/oss/tests/pytest/unit/git/test_retrieval_info_utils.py | Unit tests for helper merging and key handling. |
| api/oss/tests/pytest/unit/{applications,evaluators,environments,workflows,queries,testsets}/test_retrieve_resolve.py (+ test_router.py, test_flag_ownership.py) | New/updated service-level unit tests covering direct + env-backed paths. |
| api/oss/tests/pytest/unit/legacy_variants/test_configs_fetch.py | Updated mock return values to 3-tuple. |
| api/oss/tests/pytest/acceptance/{applications,evaluators,environments,workflows,queries,testsets}/test_*_retrieval_info.py | End-to-end acceptance coverage for the new field. |
| sdks/python/oss/tests/pytest/utils/test_resolver_middleware.py | Asserts retrieval references are merged onto the tracing context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
api/oss/src/apis/fastapi/evaluators/router.py (1)
1403-1409:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit evaluator revision events under the evaluator domain.
These handlers now publish
domain="workflow", so evaluator retrieve/fetch/query/log events will be misclassified by any downstream audit, analytics, or filtering that keys off the domain.Suggested fix
- domain="workflow", + domain="evaluator",Also applies to: 1489-1495, 1658-1664, 1737-1743
api/oss/src/core/annotations/service.py (1)
93-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass environment-backed refs into evaluator retrieval.
Both calls only forward
evaluator*refs. That breaks the new environment-backed path:create()can miss the intended evaluator and fall back to creating a new simple evaluator, andedit()can still validate against the existing annotation's evaluator instead of the refs supplied inannotation_edit.references.Suggested fix
- ( - evaluator_revision, - _, - retrieval_info, - ) = await self.evaluators_service.retrieve_evaluator_revision( - project_id=project_id, - # - evaluator_ref=annotation.references.evaluator, - evaluator_variant_ref=annotation.references.evaluator_variant, - evaluator_revision_ref=annotation.references.evaluator_revision, - ) + effective_references = annotation_edit.references or annotation.references + ( + evaluator_revision, + _, + retrieval_info, + ) = await self.evaluators_service.retrieve_evaluator_revision( + project_id=project_id, + # + environment_ref=effective_references.environment, + environment_variant_ref=effective_references.environment_variant, + environment_revision_ref=effective_references.environment_revision, + key=( + effective_references.selector.get("key") + if effective_references.selector + else None + ), + # + evaluator_ref=effective_references.evaluator, + evaluator_variant_ref=effective_references.evaluator_variant, + evaluator_revision_ref=effective_references.evaluator_revision, + )Apply the same environment/selector forwarding in
create(...)usingannotation_create.references.Also applies to: 302-312
web/packages/agenta-entities/src/annotation/core/types.ts (1)
94-108:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove duplicate
evaluator_variantproperty.The
evaluator_variantproperty is defined twice in theUpdateAnnotationPayload.annotation.referencesinterface (lines 100 and 104). TypeScript will silently accept this but it's redundant and likely unintentional.🐛 Proposed fix to remove duplicate property
references?: { evaluator: {id?: string; slug?: string} evaluator_variant?: {id?: string; slug?: string} evaluator_revision?: {id?: string; slug?: string; version?: string | number} testset?: {id?: string} testcase?: {id?: string} - evaluator_variant?: {id?: string; slug?: string} }
🧹 Nitpick comments (2)
api/oss/tests/pytest/unit/annotations/test_annotations_service.py (1)
34-38: ⚡ Quick winAdd a retrieval-info case to this test.
This mock still returns
(revision, None, None), so the newretrieval_info.references/retrieval_info.selectormerge path inAnnotationsService.editis never exercised here.api/oss/src/core/tracing/utils/attributes.py (1)
266-269: 💤 Low valueRun
ruff formatto ensure consistent formatting.The reformatting of
model_dump()arguments should be validated by runningruff formatandruff check --fixfrom the API folder. As per coding guidelines, runruff formatthenruff checkafter making changes to API code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: acc5fcbd-3d4b-4c5f-9d98-835ccd6bc2a4
📒 Files selected for processing (57)
api/oss/src/apis/fastapi/applications/router.pyapi/oss/src/apis/fastapi/environments/router.pyapi/oss/src/apis/fastapi/evaluators/router.pyapi/oss/src/apis/fastapi/queries/router.pyapi/oss/src/apis/fastapi/testsets/router.pyapi/oss/src/apis/fastapi/workflows/router.pyapi/oss/src/core/annotations/service.pyapi/oss/src/core/applications/service.pyapi/oss/src/core/environments/service.pyapi/oss/src/core/evaluations/tasks/legacy.pyapi/oss/src/core/evaluations/tasks/live.pyapi/oss/src/core/evaluators/service.pyapi/oss/src/core/git/dtos.pyapi/oss/src/core/git/utils.pyapi/oss/src/core/invocations/service.pyapi/oss/src/core/queries/service.pyapi/oss/src/core/test_plan_reference_enrichment.mdapi/oss/src/core/testsets/service.pyapi/oss/src/core/tracing/dtos.pyapi/oss/src/core/tracing/utils/attributes.pyapi/oss/src/core/workflows/service.pyapi/oss/src/services/llm_apps_service.pyapi/oss/tests/pytest/acceptance/applications/test_application_retrieval_info.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/queries/test_query_retrieval_info.pyapi/oss/tests/pytest/acceptance/testsets/test_testset_retrieval_info.pyapi/oss/tests/pytest/acceptance/workflows/test_workflow_retrieval_info.pyapi/oss/tests/pytest/unit/annotations/test_annotations_service.pyapi/oss/tests/pytest/unit/applications/test_retrieve_resolve.pyapi/oss/tests/pytest/unit/applications/test_router.pyapi/oss/tests/pytest/unit/environments/test_retrieve_resolve.pyapi/oss/tests/pytest/unit/evaluators/test_retrieve_resolve.pyapi/oss/tests/pytest/unit/git/test_retrieval_info_utils.pyapi/oss/tests/pytest/unit/queries/test_retrieve.pyapi/oss/tests/pytest/unit/test_llm_apps_service.pyapi/oss/tests/pytest/unit/testsets/test_retrieve.pyapi/oss/tests/pytest/unit/tracing/utils/test_attributes.pyapi/oss/tests/pytest/unit/workflows/test_retrieve_resolve.pysdks/python/agenta/sdk/contexts/tracing.pysdks/python/agenta/sdk/decorators/tracing.pysdks/python/agenta/sdk/engines/tracing/processors.pysdks/python/agenta/sdk/middlewares/running/resolver.pysdks/python/agenta/sdk/models/tracing.pysdks/python/oss/tests/pytest/utils/test_resolver_middleware.pysdks/python/oss/tests/pytest/utils/test_selector_span_attribute.pyweb/oss/src/components/SharedDrawers/AnnotateDrawer/assets/transforms.tsweb/packages/agenta-annotation/src/state/controllers/annotationFormController.tsweb/packages/agenta-entities/src/annotation/core/types.tsweb/packages/agenta-entities/src/evaluationRun/core/schema.tsweb/packages/agenta-entities/src/runnable/index.tsweb/packages/agenta-entities/src/workflow/state/appUtils.tsweb/packages/agenta-entities/src/workflow/state/commit.tsweb/packages/agenta-entities/src/workflow/state/runnableSetup.tsweb/packages/agenta-entities/src/workflow/state/store.tsweb/packages/agenta-entity-ui/src/adapters/variantAdapters.tsweb/packages/agenta-playground/src/state/execution/executionRunner.ts
🚧 Files skipped from review as they are similar to previous changes (24)
- api/oss/tests/pytest/unit/testsets/test_retrieve.py
- api/oss/tests/pytest/acceptance/testsets/test_testset_retrieval_info.py
- api/oss/tests/pytest/acceptance/environments/test_environment_retrieval_info.py
- api/oss/tests/pytest/acceptance/queries/test_query_retrieval_info.py
- api/oss/tests/pytest/unit/queries/test_retrieve.py
- api/oss/src/apis/fastapi/environments/router.py
- api/oss/src/core/git/utils.py
- api/oss/src/apis/fastapi/queries/router.py
- api/oss/tests/pytest/acceptance/workflows/test_workflow_retrieval_info.py
- api/oss/tests/pytest/acceptance/applications/test_application_retrieval_info.py
- api/oss/tests/pytest/unit/applications/test_router.py
- api/oss/tests/pytest/unit/environments/test_retrieve_resolve.py
- api/oss/src/core/queries/service.py
- api/oss/tests/pytest/unit/git/test_retrieval_info_utils.py
- api/oss/src/core/evaluators/service.py
- api/oss/src/core/applications/service.py
- api/oss/src/apis/fastapi/applications/router.py
- api/oss/src/apis/fastapi/workflows/router.py
- api/oss/src/core/testsets/service.py
- api/oss/tests/pytest/unit/workflows/test_retrieve_resolve.py
- api/oss/src/apis/fastapi/testsets/router.py
- api/oss/tests/pytest/unit/applications/test_retrieve_resolve.py
- api/oss/tests/pytest/unit/evaluators/test_retrieve_resolve.py
- api/oss/src/core/workflows/service.py
No description provided.