[fix] Return the newest testset revision, not an arbitrary one - #6568
[fix] Return the newest testset revision, not an arbitrary one#6568mmabrouk wants to merge 1 commit into
Conversation
fetchLatestRevisionsBatch sent no windowing, and query_revisions adds an
ORDER BY only when windowing is present. With no ordering the rows came back
in unspecified order, and the loop kept whichever revision arrived last, so a
function named "latest" returned an arbitrary revision. It looked correct only
because a small unmodified table tends to return rows in insertion order.
It also sent {id, limit: 1} inside a Reference. Reference has no model_config,
so pydantic drops unknown keys and the limit never applied. The docstring
described a ReferenceWithLimit feature using SQL window functions; that name
appears nowhere else in the repository.
Ask for newest first, keep the first revision per testset, and prefer a
configured revision over an auto-created v0 placeholder. That last rule matches
the sibling fetcher in web/oss/src/state/entities/testset/revisionEntity.ts.
This does not stop the over-fetch. The API cannot express "the latest revision
of each of these parents", so the batch still reads every revision of every
testset.
Refs #6563
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📘 Docs preview
This comment updates in place on every push. |
📝 SummarySummary by CodeRabbit
WalkthroughThe change adds design documentation for grouped revision queries and records the rollout plan across six endpoints. It also updates testset batch fetching to select newest revisions first and use version-zero revisions only as fallbacks. ChangesRevision Query Grouping
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to Configured version-zero testsets can resolve to the wrong revision, and the planned grouping API has a contradictory limit contract. These should be corrected before merge. 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
web/packages/agenta-entities/src/testset/api/api.ts (1)
192-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the generated testsets client for this revision query.
getTestsetsClient().queryTestsetRevisionssupports the current request body andqueryParams, includingproject_id. Replace the directaxios.postcall and passproject_idthrough{queryParams: {project_id: projectId}}.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Team
Run ID: 6abc7a03-92ab-411a-93ec-1890535a4263
📒 Files selected for processing (7)
docs/design/revision-query-grouping/README.mddocs/design/revision-query-grouping/api-design.mddocs/design/revision-query-grouping/context.mddocs/design/revision-query-grouping/plan.mddocs/design/revision-query-grouping/research.mddocs/design/revision-query-grouping/status.mdweb/packages/agenta-entities/src/testset/api/api.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
| The two limits compose, and that is a sign the split is correct. `grouping.limit` says how | ||
| many revisions to keep inside each parent. `windowing.limit` still caps how many parents | ||
| come back. They answer different questions, so they sit side by side. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove the obsolete grouping.limit contract.
Lines 38-42 define the first release as by only and explicitly remove limit. Lines 56-58 still define grouping.limit. Choose one contract. For the planned shape, remove this paragraph and state that windowing.limit caps parent groups.
| - Check the OpenAPI schema actually describes the request body. The workflows handler reads | ||
| `Request.json()` rather than declaring a body model, and the generated Python client | ||
| exposes query parameters instead of the body. Regeneration alone will not fix that. | ||
| - Regenerate both clients with `clients/scripts/generate.sh`: Python under | ||
| `clients/python/agenta_client`, TypeScript under `web/packages/agenta-api-client`. | ||
| - Rebuild `@agentaai/api-client` so consumers see the new types. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make the body-schema fix explicit before regeneration.
The plan states that regeneration alone will not expose grouping, but Step 3 only says to check the schema and regenerate. Require the handler and OpenAPI body contract to be updated before generating clients.
| The fix asks for newest first, keeps the first revision seen per testset, and prefers a | ||
| configured revision over an auto-created version 0 placeholder. That last rule matches the | ||
| sibling fetcher in `web/oss/src/state/entities/testset/revisionEntity.ts`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🤖 get_repo_knowledge executed:
get_repo_knowledge Agenta-AI/agenta /tmp/coderabbit-repo-knowledge/agenta-ai-agenta-4b53879a/conventions /tmp/coderabbit-repo-knowledge/agenta-ai-agenta-4b53879a/learnings
Length of output: 14758
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- status document ---'
cat -n docs/design/revision-query-grouping/status.md
printf '%s\n' '--- relevant definitions and references ---'
rg -n -C 8 'fetchLatestRevisionsBatch|v0Fallback|revision\.version|version === 0|version: 0' web docs -g '*.ts' -g '*.tsx' -g '*.md' | head -n 320
printf '%s\n' '--- candidate revision entity files ---'
fd -i 'revisionEntity' .Repository: Agenta-AI/agenta
Length of output: 37412
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- status document ---'
cat -n docs/design/revision-query-grouping/status.md
printf '%s\n' '--- direct symbols ---'
rg -n -C 10 'fetchLatestRevisionsBatch|v0Fallback|revision\.version|version === 0|version: 0' web docs -g '*.ts' -g '*.tsx' -g '*.md' | head -n 320
printf '%s\n' '--- revision entity candidates ---'
fd -i 'revisionEntity' .Repository: Agenta-AI/agenta
Length of output: 42578
🏁 Script executed:
#!/bin/bash
set -eu
cat -n docs/design/revision-query-grouping/status.md
rg -n -C 8 'fetchLatestRevisionsBatch|v0Fallback|revision\.version|version === 0|version: 0' web docs -g '*.ts' -g '*.tsx' -g '*.md' | head -n 320
fd -i 'revisionEntity' .Repository: Agenta-AI/agenta
Length of output: 38496
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- package fetcher ---'
rg -n -C 14 'async function fetchLatestRevisionsBatch|const fetchLatestRevisionsBatch|fetchLatestRevisionsBatch' web/packages/agenta-entities/src/testset/api/api.ts
printf '%s\n' '--- sibling selection ---'
sed -n '348,418p' web/oss/src/state/entities/testset/revisionEntity.ts
printf '%s\n' '--- revision creation and version assignment ---'
rg -n -C 8 'version\s*=\s*0|version\s*=\s*["'"'"']0|version\s*:\s*0|is_draft|isDraft|draft.*revision|create.*revision|commit.*revision' api/oss/src web/packages/agenta-entities/src web/oss/src -g '*.py' -g '*.ts' -g '*.tsx' | head -n 360Repository: Agenta-AI/agenta
Length of output: 34080
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- package fetcher ---'
rg -n -C 14 'async function fetchLatestRevisionsBatch|const fetchLatestRevisionsBatch|fetchLatestRevisionsBatch' web/packages/agenta-entities/src/testset/api/api.ts
printf '%s\n' '--- sibling selection ---'
sed -n '348,418p' web/oss/src/state/entities/testset/revisionEntity.ts
printf '%s\n' '--- testset revision version contracts ---'
rg -n -C 8 'version\s*=\s*0|version\s*=\s*["'"'"']0|version\s*:\s*0|is_draft|isDraft|draft.*revision|create.*revision|commit.*revision' api/oss/src web/packages/agenta-entities/src web/oss/src -g '*.py' -g '*.ts' -g '*.tsx' | head -n 360Repository: Agenta-AI/agenta
Length of output: 33144
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- testset revision models and commit paths ---'
rg -l 'class .*Testset.*Revision|TestsetRevision|testset_revision|version' api/oss/src -g '*.py' | grep -E 'testset|revision|git' | head -n 120
printf '%s\n' '--- version-zero and testset revision references in API ---'
rg -n -C 6 'version.*0|0.*version|is_draft|testset_revision' api/oss/src -g '*.py' | head -n 420Repository: Agenta-AI/agenta
Length of output: 33636
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- revision DTO fields ---'
sed -n '1,115p' api/oss/src/core/testsets/dtos.py
sed -n '270,330p' api/oss/src/core/testsets/dtos.py
printf '%s\n' '--- commit service path ---'
rg -n -C 18 'async def commit_testset_revision|commit_testset_revision\(' api/oss/src/core/testsets/service.py api/oss/src/dbs/postgres/git/dao.py
printf '%s\n' '--- generic revision commit types and version assignment ---'
rg -n -C 12 'class Revision(Create|Commit|Log|Query)|version.*Field|version\s*=' api/oss/src/core/git api/oss/src/dbs/postgres/git -g '*.py'Repository: Agenta-AI/agenta
Length of output: 36981
Distinguish configured v0 revisions from placeholders. The DAO preserves version 0 when the first commit carries content, so a configured v0 revision can exist. fetchLatestRevisionsBatch routes every version-0 revision to v0Fallback and discards it whenever a newer revision exists. Use the DAO’s placeholder distinction or explicit provenance metadata.
🧰 Tools
🪛 LanguageTool
[grammar] ~26-~26: Ensure spelling is correct
Context: ...irst, keeps the first revision seen per testset, and prefers a configured revision over...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
Railway Preview Environment
|
393684e to
7b97e5d
Compare
1 participant
Context
fetchLatestRevisionsBatchreturns the latest revision of each testset. It did not. It returned an arbitrary one.The call sent no
windowing, andGitDAO.query_revisionsadds anORDER BYonly when windowing is present. With no ordering, Postgres returned the rows in whatever order it liked, and the loop overwrote the map on every row, so the last row to arrive won. It looked correct only because a small unmodified table tends to return rows in insertion order. After updates or a vacuum, it would not.Two smaller problems sat on top. The call sent
{id, limit: 1}inside aReference.Referenceisclass Reference(Identifier, Slug, Version)with nomodel_config, so pydantic drops unknown keys and the limit never applied. Checked on a running API:The docstring also described a
ReferenceWithLimitfeature that uses SQL window functions to return the top N per testset. That name appears in exactly one place in the repository: that comment. The feature does not exist.Changes
The request now asks for newest first. The loop keeps the first revision it sees for each testset and prefers a configured revision over an auto-created version 0 placeholder. That last rule matches the sibling fetcher in
web/oss/src/state/entities/testset/revisionEntity.ts, so the two agree on what "latest" means.Before:
After:
This does not stop the over-fetch. See the next section.
Why the over-fetch is not fixed here
The API cannot express "the latest revision of each of these parents", so this batch still reads every revision of every testset. Six endpoints share that gap. The design for the API change is in #6569, and the incident it caused is issue #6563. This PR is independent of both and can merge on its own.
Tests
tsc --noEmiton@agenta/entitiespasses.Referencedrop and the missingORDER BYagainst a running API and its Postgres, not from reading alone.What to QA