fix(api-v1): return real task.status in CreateTaskResponse, not hardcoded "pending"#426
Conversation
… "pending" POST /v1/chat/tasks returned ``status="pending"`` in the response body even though ``begin_turn`` had already atomically claimed the row as RUNNING inside the same handler before the response was sent. An SDK client doing POST followed by an immediate GET on the same task would see two contradictory status values from back-to-back calls (``pending`` then ``running``). Read ``task.status.value`` after ``begin_turn`` returns instead -- ``begin_turn`` refreshes the in-memory ``task`` after committing the atomic claim, so the post-handler view reflects the row's real state. ``AppendMessageResponse`` already followed this contract; the create path now matches it. Schema docstrings for CreateTaskResponse and AppendMessageResponse updated to describe the post-claim ``running`` semantics; the ``test_create_task_happy_path`` assertion was checking for the old ``pending`` value and has been updated. No behavior change in the orchestrator or scheduler -- this is a response-payload correctness fix only. Twenty-eight v1 task tests pass; pre-commit (ruff / mypy / isort / codespell) green.
There was a problem hiding this comment.
Code Review
This pull request updates the task creation API to return the actual task status (typically 'running') instead of a hardcoded 'pending' value, ensuring consistency between the response and the database state. Documentation and tests have been updated to reflect this change. The reviewer suggested further improving consistency by replacing hardcoded status strings with the Enum value in other response models like AppendMessageResponse.
| task_id=int(task.id), | ||
| agent_id=int(agent.id), | ||
| status="pending", | ||
| status=task.status.value, |
There was a problem hiding this comment.
While using task.status.value correctly retrieves the string representation of the Enum, AppendMessageResponse (line 322) still uses a hardcoded string "running". For better consistency and robustness against future status changes, consider using task.status.value in both locations, or updating the response model to accept the Enum type directly.
|
The title v1 is distracting, maybe call it api v1 or sth. |
…nse status read Follow-up to the CreateTaskResponse status fix in the previous commit. Two consistency cleanups within the same handler module: 1. ``create_chat_task`` function docstring still described the return value as ``status='pending'`` -- inconsistent with the schema docstring and the implementation that the previous commit already moved to ``task.status.value`` (i.e. 'running'). Updated the Returns section to match. 2. ``append_message_to_task`` returned ``status="running"`` hard- coded while ``create_chat_task`` reads ``task.status.value``. Unified both endpoints on the same pattern -- reading from the refreshed in-memory row is defensive (any future ``begin_turn`` status-machine change is picked up automatically) and removes the asymmetry where one endpoint reflected the DB and the other asserted a fixed string. No behavior change for the current contract -- the previous commit already returned 'running' from CREATE; this commit makes APPEND share the same expression form and fixes the docstring drift. Twenty-eight v1 task tests pass; pre-commit green.
|
Renamed the scope from |
…lure
``execute_task_background`` previously logged and broadcast the
exception text on failure but never wrote it to ``task.error_message``.
The row's status stayed RUNNING, and ``finish_turn``'s
RUNNING-fallback branch then unconditionally set
``error_message`` to a generic placeholder ("Task execution failed
without status update; see /steps."), forcing SDK and web clients
to fetch ``/steps`` to discover what actually went wrong.
The fix writes the real exception text to ``task.error_message`` and
flips ``status`` to ``FAILED`` in the exception handler, using a
fresh session because the original may be in a failed-transaction
state. ``finish_turn``'s FAILED branch then only fills in a
placeholder when ``error_message`` is empty, so the real message is
preserved through to the final row state.
Same family of fix as the ``status="pending"`` correction above:
make the persisted task row reflect the real outcome rather than a
generic placeholder. Affects both SDK consumers (GET /v1/chat/tasks/
{id}) and web/WebSocket consumers reading the same row.
Twenty-eight v1 task tests pass; pre-commit (ruff / mypy / isort /
codespell) green.
Summary
Two related response-correctness fixes for the v1 SDK API surface — both make the task row reflect the real outcome instead of a hardcoded placeholder.
Fix 1 —
statusfield onPOST /v1/chat/tasksPOST /v1/chat/tasksreturnedstatus="pending"in the response body even thoughbegin_turnhad already atomically claimed the row asRUNNINGinside the same handler before the response was sent. An SDK client doingPOSTfollowed by an immediateGETon the same task would see two contradictory status values from back-to-back calls (pendingthenrunning).Reads
task.status.valueafterbegin_turnreturns.begin_turnrefreshes the in-memorytaskafter committing the atomic claim, so the post-handler view reflects the row's real state.AppendMessageResponsealready followed this contract; the create path now matches it, and the create-side response-string was unified to read from the refreshed row so both endpoints use the same expression form.Fix 2 —
task.error_messagepopulated with real exception textexecute_task_backgroundpreviously logged and broadcast the exception text on failure but never wrote it totask.error_message. The row's status stayedRUNNING, andfinish_turn's RUNNING-fallback branch then unconditionally seterror_messageto a generic placeholder ("Task execution failed without status update; see /steps."), forcing SDK and web clients to fetch/stepsto discover what actually went wrong.The fix writes the real exception text to
task.error_messageand flipsstatustoFAILEDin the exception handler, using a fresh session because the original may be in a failed-transaction state.finish_turn's FAILED branch then only fills in a placeholder whenerror_messageis empty, so the real message is preserved through to the final row state.Changes
src/xagent/web/api/v1/tasks.py:POST /v1/chat/tasksreadstask.status.valueinstead of hardcoded"pending"; brief comment mirrors the equivalent block at theAppendMessageResponsereturn site.POST /v1/chat/tasks/{id}/messagesswitched to the sametask.status.valuepattern so the two endpoints are symmetric (was hardcoded"running").create_chat_taskReturns:docstring updated to describe the post-claimstatus='running'contract.src/xagent/web/schemas/v1.py:CreateTaskResponseandAppendMessageResponsefield docstrings updated — both previously claimed"always 'pending'", both now describe the post-claimrunningsemantics.src/xagent/web/api/websocket.py:execute_task_backgroundexception handler now writesstr(e)[:4000]totask.error_messageand flipsstatustoFAILEDin a fresh session before broadcasting the error event.tests/web/api/v1/test_tasks.py:test_create_task_happy_pathwas asserting the old"pending"value and has been updated to assert"running", with a comment explaining the post-claim semantics.Why not a regression risk
statusanderror_messagecolumns; both are now accurate where they were previously a placeholder.AppendMessageResponsealready returned"running"for the same reason, so SDK clients tolerating that one will tolerate the create-side identically.Test plan
pytest tests/web/api/v1/ tests/web/services/ tests/web/api/test_agent_api_keys.py— 92 passedpre-commit run --files <touched files>— ruff / mypy / isort / codespell greenfinish_turn's FAILED branch preserves the populatederror_message(only writes a placeholder when empty).