tiennm99/DocsGPT
1
0 Fork 0
mirror of https://github.com/tiennm99/DocsGPT.git synced 2026-10-04 22:13:08 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: track agent creation errors more

Browse Source
This commit is contained in:
Alex committed 2026-08-09 11:20:35 +01:00
1 parent 732f7d1a89
commit a8f1e0959e
10 files changed
+221 -142
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

application/api/user/agents/routes.py
+81 -131
View File
@@ -178,6 +178,30 @@ def _resolve_folder_id(conn, folder_id, user):
return str(folder["id"]), None
def _reject(message: str, user: str, field: str = "-"):
"""Log a request-validation rejection at WARN and return its 400 response.
Every validation branch in the agent write paths used to ``make_response``
a 400 without logging anything, so a rejected update left no server-side
trace — the only evidence was the request span's status code. The client
compounded it by discarding the response body, which made an entirely
deterministic failure undiagnosable from any telemetry we keep. Route all
400s through here so the field and user always reach the logs.
Args:
message: User-facing reason, returned verbatim in the response body.
user: Subject claim of the caller, for correlating with client reports.
field: Name of the offending field, or ``"-"`` when not field-specific.
Returns:
A Flask 400 response carrying ``{"success": False, "message": ...}``.
"""
current_app.logger.warning(
"Agent update rejected: %s (field=%s, user=%s)", message, field, user
)
return make_response(jsonify({"success": False, "message": message}), 400)
def _format_agent_output(
agent: dict,
*,
@@ -771,14 +795,10 @@ class UpdateAgent(Resource):
try:
data[field] = json.loads(data[field])
except json.JSONDecodeError:
return make_response(
jsonify(
{
"success": False,
"message": f"Invalid JSON format for field: {field}",
}
),
400,
return _reject(
f"Invalid JSON format for field: {field}",
user,
field,
)
if data.get("json_schema") == "":
data["json_schema"] = None
@@ -855,14 +875,10 @@ class UpdateAgent(Resource):
if field == "status":
new_status = data.get("status")
if new_status not in ["draft", "published"]:
return make_response(
jsonify(
{
"success": False,
"message": "Invalid status value. Must be 'draft' or 'published'",
}
),
400,
return _reject(
"Invalid status value. Must be 'draft' or 'published'",
user,
field,
)
update_fields["status"] = new_status
elif field == "source":
@@ -872,14 +888,8 @@ class UpdateAgent(Resource):
elif looks_like_uuid(source_id):
update_fields["source_id"] = source_id
else:
return make_response(
jsonify(
{
"success": False,
"message": f"Invalid source ID format: {source_id}",
}
),
400,
return _reject(
f"Invalid source ID format: {source_id}", user, field
)
elif field == "sources":
sources_list = data.get("sources", []) or []
@@ -893,14 +903,8 @@ class UpdateAgent(Resource):
if looks_like_uuid(src):
valid.append(src)
else:
return make_response(
jsonify(
{
"success": False,
"message": f"Invalid source ID in list: {src}",
}
),
400,
return _reject(
f"Invalid source ID in list: {src}", user, field
)
update_fields["extra_source_ids"] = valid
elif field == "chunks":
@@ -911,33 +915,20 @@ class UpdateAgent(Resource):
try:
chunks_int = int(chunks_value)
if chunks_int < 0:
return make_response(
jsonify(
{
"success": False,
"message": "Chunks value must be a non-negative integer",
}
),
400,
return _reject(
"Chunks value must be a non-negative integer",
user,
field,
)
update_fields["chunks"] = chunks_int
except (ValueError, TypeError):
return make_response(
jsonify(
{
"success": False,
"message": f"Invalid chunks value: {chunks_value}",
}
),
400,
return _reject(
f"Invalid chunks value: {chunks_value}", user, field
)
elif field == "tools":
tools_list = data.get("tools", [])
if not isinstance(tools_list, list):
return make_response(
jsonify({"success": False, "message": "Tools must be a list"}),
400,
)
return _reject("Tools must be a list", user, field)
update_fields["tools"] = tools_list
elif field == "json_schema":
json_schema = data.get("json_schema")
@@ -947,10 +938,7 @@ class UpdateAgent(Resource):
json_schema
)
except JsonSchemaValidationError:
return make_response(
jsonify({"success": False, "message": "Invalid JSON schema"}),
400,
)
return _reject("Invalid JSON schema", user, field)
else:
update_fields["json_schema"] = None
elif field == "limited_token_mode":
@@ -962,14 +950,10 @@ class UpdateAgent(Resource):
)
update_fields["limited_token_mode"] = bool_value
if bool_value and data.get("token_limit") is None:
return make_response(
jsonify(
{
"success": False,
"message": "Token limit must be provided when limited token mode is enabled",
}
),
400,
return _reject(
"Token limit must be provided when limited token mode is enabled",
user,
field,
)
elif field == "limited_request_mode":
raw_value = data.get("limited_request_mode", False)
@@ -980,40 +964,34 @@ class UpdateAgent(Resource):
)
update_fields["limited_request_mode"] = bool_value
if bool_value and data.get("request_limit") is None:
return make_response(
jsonify(
{
"success": False,
"message": "Request limit must be provided when limited request mode is enabled",
}
),
400,
return _reject(
"Request limit must be provided when limited request mode is enabled",
user,
field,
)
elif field == "token_limit":
token_limit = data.get("token_limit")
update_fields["token_limit"] = int(token_limit) if token_limit else 0
# NOTE: unreachable from a multipart/form submit. ``data``
# then comes from ``request.form.to_dict()``, so this is
# the *string* "False" and ``not "False"`` is False. Left
# as-is deliberately: tightening it here would start
# rejecting form payloads that currently succeed.
if update_fields["token_limit"] > 0 and not data.get("limited_token_mode"):
return make_response(
jsonify(
{
"success": False,
"message": "Token limit cannot be set when limited token mode is disabled",
}
),
400,
return _reject(
"Token limit cannot be set when limited token mode is disabled",
user,
field,
)
elif field == "request_limit":
request_limit = data.get("request_limit")
update_fields["request_limit"] = int(request_limit) if request_limit else 0
# Same string-truthiness caveat as ``token_limit`` above.
if update_fields["request_limit"] > 0 and not data.get("limited_request_mode"):
return make_response(
jsonify(
{
"success": False,
"message": "Request limit cannot be set when limited request mode is disabled",
}
),
400,
return _reject(
"Request limit cannot be set when limited request mode is disabled",
user,
field,
)
elif field == "folder_id":
folder_input = data.get("folder_id")
@@ -1036,10 +1014,7 @@ class UpdateAgent(Resource):
normalized = normalize_workflow_reference(workflow_input)
if not normalized:
if workflow_required:
return make_response(
jsonify({"success": False, "message": "Workflow is required"}),
400,
)
return _reject("Workflow is required", user, field)
update_fields["workflow_id"] = None
else:
pg_workflow_id, wf_err = _resolve_workflow_for_user(
@@ -1055,12 +1030,7 @@ class UpdateAgent(Resource):
elif looks_like_uuid(value):
update_fields["prompt_id"] = value
else:
return make_response(
jsonify(
{"success": False, "message": f"Invalid prompt_id: {value}"}
),
400,
)
return _reject(f"Invalid prompt_id: {value}", user, field)
elif field == "allow_system_prompt_override":
raw_value = data.get("allow_system_prompt_override", False)
update_fields["allow_system_prompt_override"] = (
@@ -1072,28 +1042,14 @@ class UpdateAgent(Resource):
value = data[field]
if field in ["name", "description", "agent_type"]:
if not value or not str(value).strip():
return make_response(
jsonify(
{
"success": False,
"message": f"Field '{field}' cannot be empty",
}
),
400,
return _reject(
f"Field '{field}' cannot be empty", user, field
)
update_fields[field] = value
if image_url:
update_fields["image"] = image_url
if not update_fields:
return make_response(
jsonify(
{
"success": False,
"message": "No valid update data provided",
}
),
400,
)
return _reject("No valid update data provided", user)
newly_generated_key = None
final_status = update_fields.get("status", existing_agent.get("status"))
@@ -1112,14 +1068,11 @@ class UpdateAgent(Resource):
if not workflow_final:
missing_published_fields.append("Workflow")
if missing_published_fields:
return make_response(
jsonify(
{
"success": False,
"message": f"Cannot publish workflow agent. Missing required fields: {', '.join(missing_published_fields)}",
}
),
400,
return _reject(
"Cannot publish workflow agent. Missing required "
f"fields: {', '.join(missing_published_fields)}",
user,
",".join(missing_published_fields),
)
else:
# ``prompt_id`` is intentionally omitted: the
@@ -1163,14 +1116,11 @@ class UpdateAgent(Resource):
):
missing_published_fields.append("Source or retriever")
if missing_published_fields:
return make_response(
jsonify(
{
"success": False,
"message": f"Cannot publish agent. Missing or invalid required fields: {', '.join(missing_published_fields)}",
}
),
400,
return _reject(
"Cannot publish agent. Missing or invalid required "
f"fields: {', '.join(missing_published_fields)}",
user,
",".join(missing_published_fields),
)
if not existing_agent.get("key"):
newly_generated_key = str(uuid.uuid4())
frontend/src/agents/NewAgent.tsx
+55 -4
View File
@@ -62,6 +62,29 @@ import WorkflowBuilder from './workflow/WorkflowBuilder';
import type { Model } from '../models/types';
/**
* Pull the backend's own explanation out of a failed response.
*
* The agent write endpoints answer a rejected save with
* `{"success": false, "message": "<why>"}` — e.g. "Invalid chunks value: …"
* or "Field 'description' cannot be empty". Callers used to test only
* `response.ok` and throw a fixed string, so the one piece of information
* that could tell the user what to change was dropped on the floor.
*/
const extractApiError = async (
response: Response,
fallback: string,
): Promise<string> => {
try {
const body = await response.json();
if (typeof body?.message === 'string' && body.message.trim())
return body.message;
} catch {
// Non-JSON body (proxy HTML error page, empty 502) — use the fallback.
}
return fallback;
};
export default function NewAgent({ mode }: { mode: 'new' | 'edit' | 'draft' }) {
const { t } = useTranslation();
const navigate = useNavigate();
@@ -126,6 +149,7 @@ export default function NewAgent({ mode }: { mode: 'new' | 'edit' | 'draft' }) {
const [hasChanges, setHasChanges] = useState(false);
const [draftLoading, setDraftLoading] = useState(false);
const [publishLoading, setPublishLoading] = useState(false);
const [submitError, setSubmitError] = useState<string | null>(null);
const [jsonSchemaText, setJsonSchemaText] = useState('');
const [jsonSchemaValid, setJsonSchemaValid] = useState(true);
const [isAdvancedSectionExpanded, setIsAdvancedSectionExpanded] =
@@ -312,11 +336,20 @@ export default function NewAgent({ mode }: { mode: 'new' | 'edit' | 'draft' }) {
try {
setDraftLoading(true);
setSubmitError(null);
const response =
effectiveMode === 'new'
? await userService.createAgent(formData, token)
: await userService.updateAgent(agent.id || '', formData, token);
if (!response.ok) throw new Error('Failed to create agent draft');
if (!response.ok) {
setSubmitError(
await extractApiError(
response,
t('agents.form.errors.saveDraftFailed'),
),
);
return;
}
const data = await response.json();
const updatedAgent = {
@@ -329,7 +362,7 @@ export default function NewAgent({ mode }: { mode: 'new' | 'edit' | 'draft' }) {
if (effectiveMode === 'new') setEffectiveMode('draft');
} catch (error) {
console.error('Error saving draft:', error);
throw new Error('Failed to save draft');
setSubmitError(t('agents.form.errors.saveDraftFailed'));
} finally {
setDraftLoading(false);
}
@@ -427,11 +460,20 @@ export default function NewAgent({ mode }: { mode: 'new' | 'edit' | 'draft' }) {
try {
setPublishLoading(true);
setSubmitError(null);
const response =
effectiveMode === 'new'
? await userService.createAgent(formData, token)
: await userService.updateAgent(agent.id || '', formData, token);
if (!response.ok) throw new Error('Failed to publish agent');
if (!response.ok) {
setSubmitError(
await extractApiError(
response,
t('agents.form.errors.publishFailed'),
),
);
return;
}
const data = await response.json();
const updatedAgent = {
@@ -451,7 +493,7 @@ export default function NewAgent({ mode }: { mode: 'new' | 'edit' | 'draft' }) {
setImageFile(null);
} catch (error) {
console.error('Error publishing agent:', error);
throw new Error('Failed to publish agent');
setSubmitError(t('agents.form.errors.publishFailed'));
} finally {
setPublishLoading(false);
}
@@ -812,6 +854,15 @@ export default function NewAgent({ mode }: { mode: 'new' | 'edit' | 'draft' }) {
<span aria-hidden />
)}
<div className="flex flex-wrap items-center gap-2">
{submitError && (
<div
role="alert"
className="flex items-center gap-2 text-sm text-red-600 dark:text-red-400"
>
<span className="h-4 w-4 shrink-0 bg-[url('/src/assets/circle-x.svg')] bg-contain bg-center bg-no-repeat" />
{submitError}
</div>
)}
{hasChanges && (
<Button
type="button"
frontend/src/locale/de.json
+5 -1
View File
@@ -1249,7 +1249,11 @@
"description": "Das Löschen dieses Agenten ist endgültig und entfernt alle seine Zeitpläne und den Ausführungsverlauf.",
"deleteButton": "Agent löschen"
},
"externalKb": "Externe KB"
"externalKb": "Externe KB",
"errors": {
"publishFailed": "Der Agent konnte nicht veröffentlicht werden. Bitte erneut versuchen.",
"saveDraftFailed": "Der Entwurf konnte nicht gespeichert werden. Bitte erneut versuchen."
}
},
"logs": {
"title": "Agenten-Protokolle",
frontend/src/locale/en.json
+5 -1
View File
@@ -1250,7 +1250,11 @@
"description": "Deleting this agent is permanent and will remove all its schedules and run history.",
"deleteButton": "Delete agent"
},
"externalKb": "External KB"
"externalKb": "External KB",
"errors": {
"publishFailed": "Could not publish the agent. Please try again.",
"saveDraftFailed": "Could not save the draft. Please try again."
}
},
"logs": {
"title": "Agent Logs",
frontend/src/locale/es.json
+5 -1
View File
@@ -1249,7 +1249,11 @@
"description": "Eliminar este agente es permanente y eliminará todas sus programaciones e historial de ejecuciones.",
"deleteButton": "Eliminar agente"
},
"externalKb": "KB Externa"
"externalKb": "KB Externa",
"errors": {
"publishFailed": "No se pudo publicar el agente. Inténtalo de nuevo.",
"saveDraftFailed": "No se pudo guardar el borrador. Inténtalo de nuevo."
}
},
"logs": {
"title": "Registros del Agente",
frontend/src/locale/jp.json
+5 -1
View File
@@ -1249,7 +1249,11 @@
"description": "このエージェントを削除すると元に戻せません。すべてのスケジュールと実行履歴も削除されます。",
"deleteButton": "エージェントを削除"
},
"externalKb": "外部KB"
"externalKb": "外部KB",
"errors": {
"publishFailed": "エージェントを公開できませんでした。もう一度お試しください。",
"saveDraftFailed": "下書きを保存できませんでした。もう一度お試しください。"
}
},
"logs": {
"title": "エージェントログ",
frontend/src/locale/ru.json
+5 -1
View File
@@ -1249,7 +1249,11 @@
"description": "Удаление этого агента необратимо и удалит все его расписания и историю запусков.",
"deleteButton": "Удалить агента"
},
"externalKb": "Внешняя БЗ"
"externalKb": "Внешняя БЗ",
"errors": {
"publishFailed": "Не удалось опубликовать агента. Попробуйте ещё раз.",
"saveDraftFailed": "Не удалось сохранить черновик. Попробуйте ещё раз."
}
},
"logs": {
"title": "Логи Агента",
frontend/src/locale/zh-TW.json
+5 -1
View File
@@ -1249,7 +1249,11 @@
"description": "刪除此代理將無法復原，並會移除其所有排程和執行歷史。",
"deleteButton": "刪除代理"
},
"externalKb": "外部知識庫"
"externalKb": "外部知識庫",
"errors": {
"publishFailed": "無法發布該助理，請重試。",
"saveDraftFailed": "無法儲存草稿，請重試。"
}
},
"logs": {
"title": "代理日誌",
frontend/src/locale/zh.json
+5 -1
View File
@@ -1249,7 +1249,11 @@
"description": "删除此代理将无法恢复，并会移除其所有计划和运行历史。",
"deleteButton": "删除代理"
},
"externalKb": "外部知识库"
"externalKb": "外部知识库",
"errors": {
"publishFailed": "无法发布该助手，请重试。",
"saveDraftFailed": "无法保存草稿，请重试。"
}
},
"logs": {
"title": "代理日志",
tests/api/user/agents/test_routes_happy.py
+50
View File
@@ -534,6 +534,56 @@ class TestUpdateAgent:
response = UpdateAgent().put(str(agent["id"]))
assert response.status_code == 400
@pytest.mark.parametrize(
"payload,field",
[
({"status": "bogus"}, "status"),
({"source": "not-a-uuid"}, "source"),
({"sources": ["not-uuid"]}, "sources"),
({"chunks": -1}, "chunks"),
({"chunks": "not-an-int"}, "chunks"),
({"tools": "not-a-list"}, "tools"),
({"prompt_id": "not-a-uuid"}, "prompt_id"),
({"description": " "}, "description"),
],
)
def test_validation_rejection_is_logged(
self, app, pg_conn, caplog, payload, field
):
"""Every 400 must leave a WARN naming the field and the user.
Regression test for the silent-publish-failure bug: the route used to
``make_response(..., 400)`` without logging, so a rejected publish left
no server-side trace at all — the only evidence it happened was the
OTel span's status code. Combined with the frontend discarding the
response body, that made a deterministic validation failure impossible
to diagnose from any telemetry we keep.
"""
import logging
from application.api.user.agents.routes import UpdateAgent
user = f"u-log-{field}"
agent = _seed_agent(pg_conn, user=user)
body = {"name": "n", "description": "d", "status": "draft", **payload}
with caplog.at_level(logging.WARNING), _patch_db(
pg_conn
), app.test_request_context(
f"/api/update_agent/{agent['id']}", method="PUT", json=body,
):
from flask import request
request.decoded_token = {"sub": user}
response = UpdateAgent().put(str(agent["id"]))
assert response.status_code == 400
warnings = [
r.getMessage() for r in caplog.records if r.levelno >= logging.WARNING
]
assert any(
field in msg and user in msg for msg in warnings
), f"no WARN naming field={field!r} and user={user!r}; got {warnings!r}"
def test_invalid_chunks_returns_400(self, app, pg_conn):
from application.api.user.agents.routes import UpdateAgent