[docs] Plan per-parent grouping for revision queries - #6569
Conversation
Six routers expose POST /<entity>/revisions/query with the same request shape, and all reach GitDAO.query_revisions. None of them can express "the newest revision of each of these parents". windowing.limit applies to the whole result, so a limit of 5 over 5 parents can return 5 revisions of the first and none of the rest. Callers drop the limit and take everything. Measured on a real project, a batch over two workflows returns 177 revisions and costs 1916 ms of synchronous CPU: 385 ms building models, 146 ms model_dump, 92 ms serializing to 76.4 MB, and 1293 ms in gzip. None of it yields, so unrelated requests on that worker time out. The workspace records the incident, the measurements, the proposed field, the shapes rejected and why, the execution plan, and the open decisions. 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
WalkthroughThis change adds design documentation for grouping revision queries by artifact or variant. It defines the proposed API contract, records performance research, outlines implementation and validation work, and tracks current decisions and open questions. ChangesRevision Query Grouping Design
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to This PR does not change runtime behavior, but its design contract remains ambiguous about grouping scope, limits, and cursor behavior. Resolve those inconsistencies and add the core acceptance cases before using the workspace as the implementation specification. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
docs/design/revision-query-grouping/plan.md (1)
85-95: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd positive coverage for both grouping modes and parent semantics.
The test plan covers several variants and rejected combinations, but it does not explicitly require
by: "artifact"versusby: "variant"or verify thatwindowing.limitcounts parents. Add these assertions to the six-path acceptance tests because they are core API rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Team
Run ID: b404c206-84b5-4e39-8918-41ec154391bb
📒 Files selected for processing (6)
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.md
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
| grouping: Optional[RevisionGrouping] = Field( | ||
| default=None, | ||
| description=( | ||
| "Return the newest revisions within each parent instead of a flat list. " | ||
| "When set, `windowing.limit` and `windowing.next` count parents, not revisions." | ||
| ), | ||
| ) | ||
| ``` | ||
|
|
||
| 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 unsupported first-release semantics from the request contract.
RevisionGrouping contains only by, and lines 68-69 reject windowing.next. This section instead documents grouping.limit and says windowing.next counts parents. Document the first release consistently: windowing.limit caps parents, windowing.next is rejected, and grouping.limit is not supported.
| 2. `grouping` needs a non-empty list of parent references, and the API caps how many. | ||
| The result is then bounded by the number of parents the caller named. | ||
| 3. Selection order is fixed by the server, and `windowing.order` does not change it. | ||
| Ascending order would otherwise select the oldest revision inside each parent, which | ||
| contradicts the name of the feature. | ||
| 4. `grouping` with `windowing.next`, `windowing.newest`, or `windowing.oldest` returns a | ||
| client error. A revision cursor cannot page over parents. See the next section. | ||
| 5. `grouping` with the environments `references` filter returns a client error. That | ||
| filter compares neighboring rows in a history, so it needs the history intact. | ||
| 6. A parent with no eligible revision produces no row. It does not produce an empty one. | ||
| 7. `grouping.by: "variant"` groups by the variant that owns the revision. | ||
| `grouping.by: "artifact"` groups by the artifact above the variant. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define which references select the grouped parents.
The contract requires a non-empty list of “parent references,” but each request exposes separate <entity>_refs and <entity>_variant_refs fields while by accepts artifact or variant. State the required reference field for each value and define or reject requests that provide both or the wrong scope. Without this matrix, clients cannot know which parents are grouped.
| **The `limit` field, for the newest N per parent.** Every caller we found wants exactly | ||
| one. Supporting N forces a second SQL path with `ROW_NUMBER`, and it makes the result size | ||
| unbounded again, which is the problem we set out to fix. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the newest-N rationale.
With an API cap on parent references and windowing.limit capping returned parents, grouping.limit = N would bound rows to parents × N. It would require a ROW_NUMBER SQL path, but it would not make the result unbounded. Keep the scope decision if desired, but replace this reason with the actual product or implementation reason.
| 1. Let a caller ask for the newest N revisions of each parent, in one request. | ||
| 2. Push the fold into SQL, so the discarded rows never reach Python. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the goal with the first-release contract.
This goal says the feature returns the newest N revisions per parent. docs/design/revision-query-grouping/api-design.md and docs/design/revision-query-grouping/status.md limit the first release to one revision per parent and remove grouping.limit. Mark newest-N as a future goal, or change this text to newest one.
| A single revision costs 2.2 ms to build. The project holds 19 workflows and 269 | ||
| revisions, and two of those workflows hold 179 revisions and 30 MB between them. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ 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: 18462
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
fd -i 'context.md|research.md' . | head -50
printf '%s\n' '--- research section ---'
if [ -f docs/design/revision-query-grouping/research.md ]; then
cat -n docs/design/revision-query-grouping/research.md | sed -n '95,120p'
fi
printf '%s\n' '--- context references ---'
rg -n -C 4 '258|269|revision|workflow|30 MB|2\.2 ms' docs/design/revision-query-grouping --glob '*.md'Repository: Agenta-AI/agenta
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
fd -i 'context.md|research.md' . | head -50
printf '%s\n' '--- research section ---'
cat -n docs/design/revision-query-grouping/research.md | sed -n '95,120p'
printf '%s\n' '--- context references ---'
rg -n -C 4 '258|269|revision|workflow|30 MB|2\.2 ms' docs/design/revision-query-grouping --glob '*.md'Repository: Agenta-AI/agenta
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- document headers and measurement context ---'
cat -n docs/design/revision-query-grouping/context.md | sed -n '1,40p'
cat -n docs/design/revision-query-grouping/research.md | sed -n '1,18p'
printf '%s\n' '--- exact count occurrences in this design folder ---'
rg -n -F -e '258' -e '269' docs/design/revision-query-grouping --glob '*.md'
printf '%s\n' '--- provenance terms ---'
rg -n -i 'timestamp|date|snapshot|run|measured|measurement|project.*data|real data|at the time' docs/design/revision-query-grouping --glob '*.md'Repository: Agenta-AI/agenta
Length of output: 6706
🏁 Script executed:
#!/bin/bash
set -e
cat -n docs/design/revision-query-grouping/context.md | sed -n '1,40p'
cat -n docs/design/revision-query-grouping/research.md | sed -n '1,18p'
printf '%s\n' '--- counts ---'
rg -n -F -e '258' -e '269' docs/design/revision-query-grouping --glob '*.md'
printf '%s\n' '--- provenance ---'
rg -n -i 'timestamp|date|snapshot|run|measured|measurement|project.*data|real data|at the time' docs/design/revision-query-grouping --glob '*.md'Repository: Agenta-AI/agenta
Length of output: 6612
Reconcile the revision-count snapshot.
research.md attributes its 269-revision measurement to agenta-oss-team on image v0.114.5, but context.md gives 258 revisions without snapshot details. Add provenance to context.md and explain the difference, or align the count because these values support the payload and latency claims.
1 participant
What this adds
A design workspace for one API change. It is a plan, not an implementation. No code changes here.
The problem it plans to fix
Six routers expose
POST /<entity>/revisions/querywith the same request shape, and all reachGitDAO.query_revisions: workflows, testsets, evaluators, environments, applications, and queries. None of them can answer "give me the newest revision of each of these parents".windowing.limitapplies to the whole result set, not to each parent. A limit of 5 over 5 workflows can return 5 revisions of the first workflow and none of the others. Callers have no safe option, so they drop the limit and take everything. The comment inweb/packages/agenta-entities/src/workflow/api/api.tssays so:On the project measured, 19 workflows hold 269 revisions, and two of them hold 179 revisions and 30 MB between them. A batch over those two returns 177 revisions and costs 1916 ms of synchronous CPU:
jsonschema.check_schemamodel_dumpThat is how an agent turn failed with
session <id> record log is unreadable; cannot rebuild the conversation. The record log was fine. The runner gives the API five seconds, and a blocked worker did not answer in time. Issue #6563 has the full incident trace.The proposed interface
One optional field, a sibling of
windowing, on all six request models:{ "workflow_refs": [{"id": "..."}, {"id": "..."}], "grouping": { "by": "artifact" } }The first release returns at most one revision per requested parent. It carries no "newest N per parent" option and no paging over parents.
api-design.mdshows the worked case where a revision cursor makes one parent appear on two pages, which is why paging is rejected rather than redefined.What to read
README.mdcontext.mdresearch.mdapi-design.mdplan.mdstatus.mdNotes for reviewers
Codex reviewed this at medium effort and its findings are folded in. The one that changed the plan most: do not land the request field before the behavior works. The workflows and environments routers read
await request.json()and expand it into a parser with a fixed keyword signature, inside a bareexcept Exception: pass. Verified on a running API:The router swallows that and leaves the body
None, so the parent references are discarded and the query becomes project-wide. Shipping the field early would make this incident worse.Two decisions in
status.mdare open and block implementation:selectMostRecentWorkflowRevisionskips v0 and ranks bycreated_at, while SQL orders by UUID7 id, so a naive fold can pick a placeholder the frontend then discards.The related testset fix is in #6568. It is independent and can merge without this.