fix(evals): persist inline dataset runs without version ids#1108
Conversation
🦋 Changeset detectedLatest commit: 4a4394e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughThis pull request enables offline evaluation runs to persist when using inline datasets by removing the dataset.versionId requirement. It introduces datasetItemId normalization logic that treats UUID-format IDs separately from non-UUID IDs, preventing API failures with string-based inline dataset items. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Deploying voltagent with
|
| Latest commit: |
489e462
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8d437ed9.voltagent.pages.dev |
| Branch Preview URL: | https://fix-evals-inline-dataset-run.voltagent.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/evals/src/voltops/run.ts (2)
419-430:isUuidregex excludes RFC 9562 nil and max UUIDs — verify with API.The regex
[1-8]in the version nibble and[89ab]in the variant position correctly validates v1–v8 RFC 4122/9562 UUIDs. However, it will reject the nil UUID (00000000-0000-0000-0000-000000000000) and the max UUID (ffffffff-ffff-ffff-ffff-ffffffffffff) introduced in RFC 9562, sending them asnull. If the API can legally store either of those as adataset_item_idforeign key, callers using them as item IDs would silently lose the link.This is almost certainly acceptable in practice (nil/max UUIDs are almost never used as real FK values), but worth a one-line comment in
isUuidto document the intentional exclusion.📝 Suggested inline doc
function isUuid(value: string): boolean { - return /^[0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(value); + // Matches RFC 4122 / RFC 9562 v1–v8, variant-1 UUIDs. + // Nil and max UUIDs are intentionally excluded; they are not valid FK values. + return /^[0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/evals/src/voltops/run.ts` around lines 419 - 430, Update isUuid/normalizeDatasetItemId to document the intentional exclusion of the RFC 9562 nil and max UUIDs: add a concise one-line comment above isUuid (and/or above normalizeDatasetItemId) stating that the regex deliberately validates RFC 4122/9562 version/variant bits for v1–v8 and therefore rejects the all-zero (nil) and all-ff (max) UUIDs, so those values will be normalized to null; mention that this is intentional to avoid silently accepting sentinel UUIDs as real dataset_item_id foreign keys.
399-416:datasetItemHashalways stringifiesitem.itemId ?? item.index— consider hash collision for numeric indices.
datasetItemHash: String(item.itemId ?? item.index)means that whenitem.itemIdisundefined, the hash becomes the numeric index ("0","1", …). Across multiple inline dataset runs, items at the same ordinal position in different runs will share the samedatasetItemHash. Depending on how the API usesdatasetItemHashto deduplicate or link results, this could cause subtle cross-run collisions.This is pre-existing behavior (unchanged in this PR), but the new code path — where
datasetItemIdis forced tonull— makesdatasetItemHashthe only correlation key for inline items, increasing the impact of this ambiguity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/evals/src/voltops/run.ts` around lines 399 - 416, datasetItemHash currently uses String(item.itemId ?? item.index) which can collide across runs when item.itemId is undefined; update the datasetItemHash computation in the return block (the datasetItemHash variable) to derive a more robust unique key by combining stable fields (e.g., item.index plus item.datasetVersionId or datasetId and/or a run identifier) or by computing a short deterministic hash of JSON.stringify({ index: item.index, datasetId: item.datasetId, datasetVersionId: item.datasetVersionId }) so inline items across runs don’t share the same "0","1",... keys; adjust where datasetItemHash is referenced elsewhere if necessary and keep normalizeDatasetItemId usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/evals/src/voltops/run.ts`:
- Around line 419-430: Update isUuid/normalizeDatasetItemId to document the
intentional exclusion of the RFC 9562 nil and max UUIDs: add a concise one-line
comment above isUuid (and/or above normalizeDatasetItemId) stating that the
regex deliberately validates RFC 4122/9562 version/variant bits for v1–v8 and
therefore rejects the all-zero (nil) and all-ff (max) UUIDs, so those values
will be normalized to null; mention that this is intentional to avoid silently
accepting sentinel UUIDs as real dataset_item_id foreign keys.
- Around line 399-416: datasetItemHash currently uses String(item.itemId ??
item.index) which can collide across runs when item.itemId is undefined; update
the datasetItemHash computation in the return block (the datasetItemHash
variable) to derive a more robust unique key by combining stable fields (e.g.,
item.index plus item.datasetVersionId or datasetId and/or a run identifier) or
by computing a short deterministic hash of JSON.stringify({ index: item.index,
datasetId: item.datasetId, datasetVersionId: item.datasetVersionId }) so inline
items across runs don’t share the same "0","1",... keys; adjust where
datasetItemHash is referenced elsewhere if necessary and keep
normalizeDatasetItemId usage unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/soft-dots-bow.mdexamples/with-chat-sdk/package.jsonpackages/evals/src/experiment/run-experiment.spec.tspackages/evals/src/voltops/run.spec.tspackages/evals/src/voltops/run.ts
1 participant
PR Checklist
Please check if your PR fulfills the following requirements:
Bugs / Features
What is the current behavior?
Offline eval runs using inline datasets (
dataset: { items: [...] }) are not persisted correctly.Two issues were observed:
dataset.versionIdwas missing.datasetItemIdwas sent as a non-UUID string, while API expects UUID fordataset_item_id.What is the new behavior?
dataset.versionIdis missing.datasetItemIdis normalized before append:nulland tracked viadatasetItemHash.This allows inline dataset offline eval runs to be created, appended, and completed successfully, and appear in Console runs.
fixes (issue)
Notes for reviewers
packages/evals/src/voltops/run.spec.tsandpackages/evals/src/experiment/run-experiment.spec.ts.pnpm --filter @voltagent/evals testpnpm --filter @voltagent/evals buildpnpm --filter voltagent-example-with-offline-evals buildSummary by cubic
Fixes offline eval runs with inline datasets so they persist correctly. Run creation works without dataset.versionId, and non-UUID inline item IDs are sent as null and tracked via datasetItemHash.
Written for commit 4a4394e. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests