[refactor] Drop duplicate EvaluationRun type, align step-reference shape#5466
Draft
ardaerzin wants to merge 1 commit into
Draft
[refactor] Drop duplicate EvaluationRun type, align step-reference shape#5466ardaerzin wants to merge 1 commit into
ardaerzin wants to merge 1 commit into
Conversation
…e shape usePreviewEvaluations declared its own EvaluationRun while @agenta/entities/evaluationRun already had the canonical zod-derived one. The local copy over-declared requiredness (name/description/status/meta/flags as non-null) where the schema says the backend can send null, so #5464 had to hand-add updated_at to keep it current. Deleted it; the two importers and the hook now take the type from the package. Swapping surfaced drift inside @agenta/entities itself: the step-reference shape was redeclared three times with different types. The core zod schema has {id, slug?: string|null, version?: number|null}, while the ETL RunStep said slug?: string and version?: STRING, and ColumnGroup.refs said id?: string. Both now reference the exported EvaluationRunStepReference from the validated schema. CandidateRun.status widened to match. Scoped out, with reasons: - PreviewTestset is NOT the entities Testset (different endpoint shape: it carries slug plus embedded data.testcases; entities Testset is lean metadata). PreviewTestCase does match Fern TestcaseOutput, but entities has no canonical testcase entity type to route it through yet. - OrganizationProvider has no entities domain at all, and the response settings is a free-form dict while Fern's SsoProviderSettingsDto models create-input. Creating a domain for one interface is disproportionate.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe PR centralizes Evaluation run type consolidation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
usePreviewEvaluationsdeclared its ownEvaluationRuninterface while@agenta/entities/evaluationRunalready had the canonical zod-derived one for the same entity. The local copy declaredname,description,status,metaandflagsas always-present, but the schema (which validates the actual payload) says the backend can send null for all of them. That gap is why #5464 had to hand-addupdated_atto the local copy to keep it current, while the entities type already had it.Changes
The local interface is deleted. Its two importers and the hook take
EvaluationRunfrom@agenta/entities/evaluationRun.CandidateRun.statusinCompareRunsMenuwas widened tostring | nullto match.Swapping surfaced drift inside
@agenta/entitiesitself. The step-reference shape is declared in three places with three different types:The core schema is the boundary-validated truth, so it now exports
EvaluationRunStepReferenceand both ETL declarations reference it instead of redeclaring it.Scoped out, and why
The other two candidates in the original list did not survive verification. Neither is a swap I would make without changing behavior or inventing structure.
PreviewTestsetis not the entitiesTestset. They describe different endpoints.PreviewTestsetcarriesslugplus an embeddeddata.testcases; the entitiesTestsetis lean metadata (id,name,description?,project_id?). Pointing one at the other would silently drop fields the UI reads.PreviewTestCasedoes match Fern'sTestcaseOutput(Fern's is a superset, all-optional). Butweb/osscannot import the Fern client directly, and the entitiestestcasedomain has no canonical testcase entity type to route it through. Adding one is reasonable follow-up work, not a drive-by.OrganizationProviderhas no entities domain at all. The EE response'ssettingsis a free-form dict, while Fern'sSsoProviderSettingsDtomodels the create-input (requiredclient_id/issuer_url). Creating a whole entities domain for one settings interface is disproportionate to the drift risk.Tests / notes
pnpm --filter @agenta/oss exec tsc --noEmitandpnpm --filter @agenta/ee exec tsc --noEmit: 0 errors each.pnpm turbo run build --filter=@agenta/entitiesandlint --filter=@agenta/oss: clean.What to QA
Nothing user-visible. If you want a smoke check: the evaluation runs table and the compare-runs menu still list runs with their status and created date, and the eval run details table still renders columns (the step-reference type feeds column grouping).