Skip to content

Fix : Fix revisions commit endpoints silently discard unknown fields … #4415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
justin212407 wants to merge 2 commits into from
Closed

Fix : Fix revisions commit endpoints silently discard unknown fields … #4415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6353c4
fix : fix revisions commit endpoints silently discard unknown fields …
justin212407 May 25, 2026
ec69a9f
Merge branch 'main' into revisions-commit-endpoint
justin212407 May 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion api/oss/src/core/applications/dtos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from typing import Optional, List
from uuid import UUID

from pydantic import Field
from pydantic import ConfigDict, Field

from oss.src.core.shared.dtos import sync_alias, AliasConfig
from oss.src.core.shared.dtos import (
Expand Down Expand Up @@ -212,6 +212,7 @@ class ApplicationVariantQuery(WorkflowVariantQuery):


class ApplicationRevisionData(WorkflowRevisionData):
model_config = ConfigDict(extra="forbid")
pass


Expand Down
3 changes: 2 additions & 1 deletion api/oss/src/core/evaluators/dtos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from typing import Optional, List
from uuid import UUID

from pydantic import Field
from pydantic import ConfigDict, Field

from oss.src.core.shared.dtos import sync_alias, AliasConfig
from oss.src.core.shared.dtos import (
Expand Down Expand Up @@ -208,6 +208,7 @@ class EvaluatorVariantQuery(WorkflowVariantQuery):


class EvaluatorRevisionData(WorkflowRevisionData):
model_config = ConfigDict(extra="forbid")
pass


Expand Down
3 changes: 2 additions & 1 deletion api/oss/src/core/queries/dtos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from typing import List, Optional
from uuid import UUID

from pydantic import BaseModel, Field
from pydantic import BaseModel, ConfigDict, Field


from oss.src.core.tracing.dtos import Filtering, Formatting
Expand Down Expand Up @@ -139,6 +139,7 @@ class QueryVariantQuery(VariantQuery):


class QueryRevisionData(BaseModel):
model_config = ConfigDict(extra="forbid")
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify whether query payload sanitization swallows validation failures and removes data.
rg -n -C4 '_sanitize_persisted_query_revision_data|model_validate\(|except Exception|payload\.pop\("data"' api/oss/src/core/queries/service.py

Repository: Agenta-AI/agenta

Length of output: 1062

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Inspect QueryRevisionData definition =="
ls -la api/oss/src/core/queries/dtos.py
wc -l api/oss/src/core/queries/dtos.py
rg -n "class QueryRevisionData|model_config" api/oss/src/core/queries/dtos.py
# show around the forbid-extra config
sed -n '120,180p' api/oss/src/core/queries/dtos.py

echo
echo "== Inspect _sanitize_persisted_query_revision_data usage & surrounding flow =="
wc -l api/oss/src/core/queries/service.py
rg -n "_sanitize_persisted_query_revision_data" api/oss/src/core/queries/service.py
# show the whole sanitizer + immediate callsite area
sed -n '1,220p' api/oss/src/core/queries/service.py

echo
echo "== Check router layer behavior when 'data' is missing =="
# locate router methods handling persisted query revisions
rg -n "persist|revision|payload\\[\"data\"\\]|payload\\.pop\\(\"data\"|sanitize" api/oss/src/apis/fastapi/ api/oss/src/core/queries -S || true
# try to show relevant router/service integration
rg -n "core\\.queries|queries\\.service|QueryRevision" api/oss/src/apis/fastapi -S || true

Repository: Agenta-AI/agenta

Length of output: 50373

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Locate queries router and commit endpoint =="
ls -la api/oss/src/apis/fastapi/queries || true
rg -n "commit_query_revision|/queries/.*/revisions/commit|QueryRevisionCommit" api/oss/src/apis/fastapi/queries -S || true
rg -n "commit.*revision|revisions/commit|QueryRevisionCommit" api/oss/src/apis/fastapi/queries -S || true

echo
echo "== Inspect queries router handler body around commit =="
# Try best-effort: grab the first matching handler and print a small window
match_file="$(rg -l "commit_query_revision" api/oss/src/apis/fastapi/queries -S || true)"
if [ -n "${match_file}" ]; then
  echo "Router file: ${match_file}"
  rg -n "commit_query_revision" "${match_file}" -S | head -n 20
  line="$(rg -n "commit_query_revision" "${match_file}" -S | head -n 1 | cut -d: -f1)"
  start=$((line-40)); end=$((line+80))
  sed -n "${start},${end}p" "${match_file}" | nl -ba | head -n 200
else
  echo "No commit_query_revision call found in queries router."
fi

echo
echo "== Inspect core QueryRevisionCommit DTO and commit service flow =="
rg -n "class QueryRevisionCommit|QueryRevisionCommit\\b" api/oss/src/core/queries/dtos.py api/oss/src/core/queries/service.py -S
# Show commit_query_revision method implementation region
rg -n "async def commit_query_revision" api/oss/src/core/queries/service.py -S
l="$(rg -n "async def commit_query_revision" api/oss/src/core/queries/service.py -S | head -n 1 | cut -d: -f1)"
start=$((l-10)); end=$((l+80))
sed -n "${start},${end}p" api/oss/src/core/queries/service.py | nl -ba | head -n 200

echo
echo "== Inspect FastAPI request models for queries revision commit (extras behavior) =="
# Look for the request model used in router for commit endpoint
rg -n "RevisionCommit|QueryRevisionCommit|revisions/commit" api/oss/src/apis/fastapi/queries -S || true
# If there is a models.py, inspect it briefly
if [ -f api/oss/src/apis/fastapi/queries/models.py ]; then
  echo "Found api/oss/src/apis/fastapi/queries/models.py"
  wc -l api/oss/src/apis/fastapi/queries/models.py
  rg -n "RevisionCommit|QueryRevisionCommit|extra" api/oss/src/apis/fastapi/queries/models.py -S || true
  rg -n "class .*Commit|def .*commit|revision_commit" api/oss/src/apis/fastapi/queries/models.py -S || true
else
  echo "No api/oss/src/apis/fastapi/queries/models.py"
  ls -la api/oss/src/apis/fastapi/queries || true
fi

Repository: Agenta-AI/agenta

Length of output: 3379

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Inspect QueryRevisionCommit DTO definition (type of .data) =="
rg -n "class QueryRevisionCommit\\b" api/oss/src/core/queries/dtos.py
l="$(rg -n "class QueryRevisionCommit\\b" api/oss/src/core/queries/dtos.py | head -n 1 | cut -d: -f1)"
start=$((l-30)); end=$((l+120))
sed -n "${start},${end}p" api/oss/src/core/queries/dtos.py

echo
echo "== Inspect FastAPI request model for commit =="
rg -n "class QueryRevisionCommitRequest" api/oss/src/apis/fastapi/queries/models.py
l="$(rg -n "class QueryRevisionCommitRequest" api/oss/src/apis/fastapi/queries/models.py | head -n 1 | cut -d: -f1)"
start=$((l-20)); end=$((l+60))
sed -n "${start},${end}p" api/oss/src/apis/fastapi/queries/models.py

echo
echo "== Inspect router handler for commit =="
l="$(rg -n "async def commit_query_revision\\b" api/oss/src/apis/fastapi/queries/router.py | head -n 1 | cut -d: -f1)"
start=$((l-30)); end=$((l+90))
sed -n "${start},${end}p" api/oss/src/apis/fastapi/queries/router.py

echo
echo "== Inspect QueriesService.commit_query_revision around sanitize =="
l="$(rg -n "async def commit_query_revision\\b" api/oss/src/core/queries/service.py | head -n 1 | cut -d: -f1)"
start=$((l-40)); end=$((l+80))
sed -n "${start},${end}p" api/oss/src/core/queries/service.py

Repository: Agenta-AI/agenta

Length of output: 13704

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Find call sites of commit_query_revision (core service) =="
rg -n "commit_query_revision\\(" api/oss/src/core/queries -S
rg -n "commit_query_revision\\(" api/oss/src -S | head -n 200

echo
echo "== Inspect how QueryRevisionCommit is constructed in those call sites =="
# Show small windows around each occurrence in core/queries/service.py
rg -n "commit_query_revision\\(" api/oss/src/core/queries/service.py -n -S
for l in $(rg -n "commit_query_revision\\(" api/oss/src/core/queries/service.py -S | cut -d: -f1 | head -n 20); do
  start=$((l-20)); end=$((l+40));
  echo "--- service.py around line ${l} ---";
  sed -n "${start},${end}p" api/oss/src/core/queries/service.py | nl -ba | head -n 120
done

echo
echo "== Check router model validation config for QueryRevisionCommitRequest =="
# QueryRevisionCommitRequest just wraps QueryRevisionCommit; check if any custom config is present
l="$(rg -n "class QueryRevisionCommitRequest\\b" api/oss/src/apis/fastapi/queries/models.py | head -n 1 | cut -d: -f1)"
start=$((l-30)); end=$((l+40))
sed -n "${start},${end}p" api/oss/src/apis/fastapi/queries/models.py

Repository: Agenta-AI/agenta

Length of output: 1240

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Find QueryRevisionCommit constructions =="
rg -n "QueryRevisionCommit\\(" api/oss/src/core/queries/service.py api/oss/src/core/queries -S
rg -n "QueryRevisionCommit\\(" api/oss/src -S | head -n 200

echo
echo "== Inspect service.py around internal commit callers (~1052, ~1278) =="
sed -n '980,1120p' api/oss/src/core/queries/service.py | cat -n
echo
sed -n '1200,1360p' api/oss/src/core/queries/service.py | cat -n

echo
echo "== Search for model_construct or raw dict usage for QueryRevisionData =="
rg -n "model_construct\\(|QueryRevisionData\\s*=|QueryRevisionData\\(" api/oss/src/core/queries -S
rg -n "QueryRevisionData\\.model_validate\\(|QueryRevisionData\\(" api/oss/src -S | head -n 200

Repository: Agenta-AI/agenta

Length of output: 13125

Avoid silently dropping QueryRevisionData when payload sanitization fails

  • _sanitize_persisted_query_revision_data catches Exception from QueryRevisionData.model_validate(...) and returns None; _sanitize_query_revision_payload then does payload.pop("data", None), so sanitizer/model-validation failures get converted into “no data”.
  • For the FastAPI commit route, extra fields should already be rejected because QueryRevisionData uses ConfigDict(extra="forbid") and the router accepts QueryRevisionCommitRequest (nested QueryRevisionCommit.data: Optional[QueryRevisionData]); still, invalid data reaching the sanitizer should be surfaced/logged rather than silently dropped.

formatting: Optional[Formatting] = None
filtering: Optional[Filtering] = None
windowing: Optional[Windowing] = None
Expand Down
3 changes: 2 additions & 1 deletion api/oss/src/core/testsets/dtos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from typing import Optional, List, Tuple
from uuid import UUID

from pydantic import BaseModel, Field
from pydantic import BaseModel, ConfigDict, Field

from oss.src.core.shared.dtos import (
sync_alias,
Expand Down Expand Up @@ -154,6 +154,7 @@ class TestsetVariantQuery(VariantQuery):


class TestsetRevisionData(BaseModel):
model_config = ConfigDict(extra="forbid")
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify whether testset payload sanitization swallows validation failures and removes data.
rg -n -C4 '_sanitize_persisted_testset_revision_data|model_validate\(|except Exception|payload\.pop\("data"' api/oss/src/core/testsets/service.py

Repository: Agenta-AI/agenta

Length of output: 2164

Avoid silently dropping invalid testset revision payloads

TestsetRevisionData forbids extras, but api/oss/src/core/testsets/service.py wraps TestsetRevisionData.model_validate(data) in try/except Exception and returns None; the caller then executes payload.pop("data", None) when sanitization fails. This converts validation failures into a silent removal of the revision payload’s data field instead of surfacing an error.

testcase_ids: Optional[List[UUID]] = None
testcases: Optional[List[Testcase]] = None

Expand Down
3 changes: 2 additions & 1 deletion sdks/python/agenta/sdk/models/testsets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from typing import List, Optional, Dict, Any
from uuid import UUID

from pydantic import BaseModel, Field
from pydantic import BaseModel, ConfigDict, Field

from agenta.sdk.models.shared import (
Identifier,
Expand Down Expand Up @@ -53,6 +53,7 @@ class TestsetFlags(BaseModel):


class TestsetRevisionData(BaseModel):
model_config = ConfigDict(extra="forbid")
testcase_ids: Optional[List[UUID]] = None
testcases: Optional[List[Testcase]] = None

Expand Down
3 changes: 3 additions & 0 deletions sdks/python/agenta/sdk/models/workflows.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ class WorkflowQueryFlags(BaseModel):


class WorkflowRevisionData(BaseModel):
model_config = ConfigDict(extra="forbid")
uri: Optional[str] = None
Comment on lines 121 to 123
Copy link
Copy Markdown
Contributor Author

@justin212407 justin212407 May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The manual curl verification confirms the 422 behavior works correctly for /applications/revisions/commit. Acceptance tests for all five domains would be a good follow-up. Happy to add those here in this pr or a new one as preffered.


url: Optional[str] = None
Expand Down Expand Up @@ -562,6 +563,7 @@ class EvaluatorVariantIdAlias(AliasConfig):


class EvaluatorRevisionData(WorkflowRevisionData):
model_config = ConfigDict(extra="forbid")
pass


Expand Down Expand Up @@ -712,6 +714,7 @@ class ApplicationVariantEdit(WorkflowVariantEdit):


class ApplicationRevisionData(WorkflowRevisionData):
model_config = ConfigDict(extra="forbid")
pass


Expand Down
Loading