Skip to content

Commit f8f53a0

Browse files
committed
refactor(agentex): simplify entity converter, DRY test setup, tighten docstrings
- Replace manual field-by-field convert_task_to_entity with TaskEntity.model_validate(task) — future fields propagate automatically. - Add test_running_task fixture; 5 of 6 current_state integration tests now share it instead of creating agent+task inline (~110 lines removed). - Collapse multi-line docstrings to one line in repository and service. - Remove redundant inline comments in use case. - Remove stray print statement in stream integration test.
1 parent 683b437 commit f8f53a0

8 files changed

Lines changed: 31 additions & 151 deletions

File tree

agentex/src/domain/entities/tasks.py

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -87,16 +87,4 @@ class TaskEntity(BaseModel):
8787

8888
def convert_task_to_entity(task: Task) -> TaskEntity:
8989
"""Converts the pydantic model from the API layer to the domain layer"""
90-
91-
return TaskEntity(
92-
id=task.id,
93-
name=task.name,
94-
status=TaskStatus[task.status.value] if task.status is not None else None,
95-
status_reason=task.status_reason,
96-
created_at=task.created_at,
97-
updated_at=task.updated_at,
98-
cleaned_at=task.cleaned_at,
99-
params=task.params,
100-
task_metadata=task.task_metadata,
101-
current_state=task.current_state,
102-
)
90+
return TaskEntity.model_validate(task)

agentex/src/domain/repositories/task_repository.py

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -243,11 +243,7 @@ async def merge_params(self, task_id: str, patch: dict) -> TaskEntity | None:
243243
async def update_mutable_fields(
244244
self, task_id: str, fields: dict[str, Any]
245245
) -> TaskEntity | None:
246-
"""Atomically set the given caller-mutable columns on one task row via
247-
``UPDATE ... RETURNING`` — column-scoped, so (unlike ``update``'s whole-row merge) it
248-
can't clobber a concurrently changed ``status``/``params``. Values apply verbatim
249-
(``current_state=None`` clears). Returns the updated entity, or ``None`` if absent.
250-
"""
246+
"""Column-scoped atomic update; can't clobber status/params. Returns updated entity or None."""
251247
unknown = fields.keys() - _MUTABLE_TASK_COLUMNS
252248
if unknown:
253249
raise ValueError(
@@ -259,9 +255,7 @@ async def update_mutable_fields(
259255
async def _update_returning(
260256
self, task_id: str, values: dict[str, Any]
261257
) -> TaskEntity | None:
262-
"""``UPDATE tasks SET <values> WHERE id`` → the updated entity, or ``None`` if no
263-
such task exists. Shared by merge_params and update_mutable_fields.
264-
"""
258+
"""UPDATE … SET … WHERE id → updated entity or None. Shared by merge_params and update_mutable_fields."""
265259
async with (
266260
self.start_async_db_session(True) as session,
267261
async_sql_exception_handler(),

agentex/src/domain/services/task_service.py

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -222,9 +222,7 @@ async def update_task(self, task: TaskEntity) -> TaskEntity:
222222
async def update_mutable_fields(
223223
self, task_id: str, fields: dict[str, Any]
224224
) -> TaskEntity | None:
225-
"""Column-scoped atomic update of the given columns, then publish task_updated.
226-
Returns the updated entity, or ``None`` if the task no longer exists.
227-
"""
225+
"""Column-scoped atomic update, then publish task_updated. Returns updated entity or None."""
228226
updated_task = await self.task_repository.update_mutable_fields(task_id, fields)
229227
if updated_task is None:
230228
return None

agentex/src/domain/use_cases/tasks_use_case.py

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -115,19 +115,16 @@ async def update_mutable_fields_on_task(
115115
else:
116116
raise ItemDoesNotExist(f"Task {name} not found")
117117

118-
# No-op if no mutable field was supplied.
119118
if task_metadata is None and merge_params is None and current_state is None:
120119
return task_entity
121120

122-
# Atomic JSONB shallow-merge; run first so its refreshed entity is the fallback return.
123121
if merge_params:
124122
merged = await self.task_service.merge_task_params(
125123
task_entity.id, merge_params
126124
)
127125
if merged is not None:
128126
task_entity = merged
129127

130-
# Single column-scoped write → one task_updated publish, no whole-row clobber.
131128
fields: dict[str, Any] = {}
132129
if task_metadata is not None:
133130
fields["task_metadata"] = task_metadata
@@ -138,7 +135,6 @@ async def update_mutable_fields_on_task(
138135
task_entity.id, fields
139136
)
140137
if updated is None:
141-
# Row vanished mid-flight (defensive; no live hard-delete path). Raise, don't return stale.
142138
raise ItemDoesNotExist(f"Task {id or name} not found")
143139
task_entity = updated
144140

agentex/tests/integration/api/tasks/test_tasks_api.py

Lines changed: 25 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,18 @@ async def test_pagination_tasks(self, isolated_repositories, test_agent):
5757
tasks.append(await task_repo.create(agent_id=test_agent.id, task=task))
5858
return tasks
5959

60+
@pytest_asyncio.fixture
61+
async def test_running_task(self, isolated_repositories, test_agent):
62+
"""Create a minimal running task for tests that don't care about its name"""
63+
task_repo = isolated_repositories["task_repository"]
64+
task = TaskEntity(
65+
id=orm_id(),
66+
name=f"running-task-{orm_id()[:8]}",
67+
status=TaskStatus.RUNNING,
68+
status_reason="Running task for testing",
69+
)
70+
return await task_repo.create(agent_id=test_agent.id, task=task)
71+
6072
@pytest_asyncio.fixture
6173
async def test_task_with_params(self, isolated_repositories, test_agent):
6274
"""Create a test task with params directly via repository"""
@@ -506,8 +518,7 @@ async def test_get_task_by_name_non_existent_returns_404(self, isolated_client):
506518
async def test_list_tasks_omits_params_in_response(
507519
self, isolated_client, test_task_with_params
508520
):
509-
"""The list summary must omit `params` (secrets/PII) but still carry the small
510-
opaque `current_state` label; fetch a single task for the full record."""
521+
"""List summary omits params but carries current_state."""
511522
# When - Request all tasks
512523
response = await isolated_client.get("/tasks")
513524

@@ -678,27 +689,10 @@ async def test_update_task_endpoint_success(
678689
assert response_data["task_metadata"]["metrics"]["complexity_score"] == 75
679690

680691
async def test_update_task_current_state(
681-
self, isolated_client, isolated_repositories
692+
self, isolated_client, test_running_task
682693
):
683694
"""PUT current_state: set it, omitted leaves it untouched, point-read reconciles."""
684-
agent_repo = isolated_repositories["agent_repository"]
685-
agent = AgentEntity(
686-
id=orm_id(),
687-
name="current-state-agent",
688-
description="Agent for current_state update testing",
689-
acp_url="http://test-acp:8000",
690-
acp_type=ACPType.SYNC,
691-
)
692-
await agent_repo.create(agent)
693-
694-
task_repo = isolated_repositories["task_repository"]
695-
task = TaskEntity(
696-
id=orm_id(),
697-
name="task-for-current-state",
698-
status=TaskStatus.RUNNING,
699-
status_reason="Test task for current_state",
700-
)
701-
created_task = await task_repo.create(agent_id=agent.id, task=task)
695+
created_task = test_running_task
702696

703697
# Fresh task: current_state present in response and null by default.
704698
response = await isolated_client.get(f"/tasks/{created_task.id}")
@@ -725,27 +719,10 @@ async def test_update_task_current_state(
725719
assert response.json()["current_state"] == "awaiting_input"
726720

727721
async def test_update_task_current_state_and_metadata_together(
728-
self, isolated_client, isolated_repositories
722+
self, isolated_client, test_running_task
729723
):
730724
"""current_state + task_metadata in one PUT both persist without clobbering status."""
731-
agent_repo = isolated_repositories["agent_repository"]
732-
agent = AgentEntity(
733-
id=orm_id(),
734-
name="current-state-combined-agent",
735-
description="Agent for combined update testing",
736-
acp_url="http://test-acp:8000",
737-
acp_type=ACPType.SYNC,
738-
)
739-
await agent_repo.create(agent)
740-
741-
task_repo = isolated_repositories["task_repository"]
742-
task = TaskEntity(
743-
id=orm_id(),
744-
name="task-for-combined-update",
745-
status=TaskStatus.RUNNING,
746-
status_reason="Test task for combined update",
747-
)
748-
created_task = await task_repo.create(agent_id=agent.id, task=task)
725+
created_task = test_running_task
749726

750727
response = await isolated_client.put(
751728
f"/tasks/{created_task.id}",
@@ -758,27 +735,17 @@ async def test_update_task_current_state_and_metadata_together(
758735
assert body["status"] == "RUNNING"
759736

760737
async def test_update_task_current_state_by_name(
761-
self, isolated_client, isolated_repositories
738+
self, isolated_client, isolated_repositories, test_agent
762739
):
763740
"""PUT /tasks/name/{name} forwards current_state too."""
764-
agent_repo = isolated_repositories["agent_repository"]
765-
agent = AgentEntity(
766-
id=orm_id(),
767-
name="current-state-by-name-agent",
768-
description="Agent for by-name current_state testing",
769-
acp_url="http://test-acp:8000",
770-
acp_type=ACPType.SYNC,
771-
)
772-
await agent_repo.create(agent)
773-
774741
task_repo = isolated_repositories["task_repository"]
775742
task = TaskEntity(
776743
id=orm_id(),
777744
name="task-for-current-state-by-name",
778745
status=TaskStatus.RUNNING,
779746
status_reason="Test task for by-name current_state",
780747
)
781-
await task_repo.create(agent_id=agent.id, task=task)
748+
await task_repo.create(agent_id=test_agent.id, task=task)
782749

783750
response = await isolated_client.put(
784751
"/tasks/name/task-for-current-state-by-name",
@@ -788,87 +755,30 @@ async def test_update_task_current_state_by_name(
788755
assert response.json()["current_state"] == "working"
789756

790757
async def test_update_task_current_state_empty_string(
791-
self, isolated_client, isolated_repositories
758+
self, isolated_client, test_running_task
792759
):
793760
"""Empty string is a valid label distinct from null (guards a falsy-check regression)."""
794-
agent_repo = isolated_repositories["agent_repository"]
795-
agent = AgentEntity(
796-
id=orm_id(),
797-
name="current-state-empty-agent",
798-
description="Agent for empty-string current_state testing",
799-
acp_url="http://test-acp:8000",
800-
acp_type=ACPType.SYNC,
801-
)
802-
await agent_repo.create(agent)
803-
804-
task_repo = isolated_repositories["task_repository"]
805-
task = TaskEntity(
806-
id=orm_id(),
807-
name="task-for-current-state-empty",
808-
status=TaskStatus.RUNNING,
809-
status_reason="Test task for empty current_state",
810-
)
811-
created_task = await task_repo.create(agent_id=agent.id, task=task)
812-
813761
response = await isolated_client.put(
814-
f"/tasks/{created_task.id}", json={"current_state": ""}
762+
f"/tasks/{test_running_task.id}", json={"current_state": ""}
815763
)
816764
assert response.status_code == 200
817765
assert response.json()["current_state"] == ""
818766

819767
async def test_update_task_current_state_too_long_rejected(
820-
self, isolated_client, isolated_repositories
768+
self, isolated_client, test_running_task
821769
):
822770
"""current_state exceeding the max length is rejected with 422."""
823-
agent_repo = isolated_repositories["agent_repository"]
824-
agent = AgentEntity(
825-
id=orm_id(),
826-
name="current-state-toolong-agent",
827-
description="Agent for max-length current_state testing",
828-
acp_url="http://test-acp:8000",
829-
acp_type=ACPType.SYNC,
830-
)
831-
await agent_repo.create(agent)
832-
833-
task_repo = isolated_repositories["task_repository"]
834-
task = TaskEntity(
835-
id=orm_id(),
836-
name="task-for-current-state-toolong",
837-
status=TaskStatus.RUNNING,
838-
status_reason="Test task for over-long current_state",
839-
)
840-
created_task = await task_repo.create(agent_id=agent.id, task=task)
841-
842771
response = await isolated_client.put(
843-
f"/tasks/{created_task.id}", json={"current_state": "x" * 256}
772+
f"/tasks/{test_running_task.id}", json={"current_state": "x" * 256}
844773
)
845774
assert response.status_code == 422
846775

847776
async def test_update_task_request_ignores_unknown_fields(
848-
self, isolated_client, isolated_repositories
777+
self, isolated_client, test_running_task
849778
):
850779
"""Unknown fields are ignored (200, not 422) — guards the extra="ignore" SDK-compat assumption."""
851-
agent_repo = isolated_repositories["agent_repository"]
852-
agent = AgentEntity(
853-
id=orm_id(),
854-
name="unknown-fields-agent",
855-
description="Agent for unknown-field compat testing",
856-
acp_url="http://test-acp:8000",
857-
acp_type=ACPType.SYNC,
858-
)
859-
await agent_repo.create(agent)
860-
861-
task_repo = isolated_repositories["task_repository"]
862-
task = TaskEntity(
863-
id=orm_id(),
864-
name="task-for-unknown-fields",
865-
status=TaskStatus.RUNNING,
866-
status_reason="Test task for unknown-field compat",
867-
)
868-
created_task = await task_repo.create(agent_id=agent.id, task=task)
869-
870780
response = await isolated_client.put(
871-
f"/tasks/{created_task.id}",
781+
f"/tasks/{test_running_task.id}",
872782
json={"current_state": "working", "field_from_a_newer_sdk": "ignored"},
873783
)
874784
assert response.status_code == 200

agentex/tests/integration/test_task_stream.py

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -287,8 +287,6 @@ async def collect_stream_events():
287287

288288
assert updated_task.current_state == "awaiting_input"
289289

290-
print("✅ current_state update successfully triggered stream event")
291-
292290
async def test_get_task_returns_updated_metadata_after_stream_update(
293291
self, test_agent_and_task, tasks_use_case
294292
):

agentex/tests/unit/services/test_task_service.py

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -992,9 +992,7 @@ async def test_update_mutable_fields_persists_and_publishes(
992992
async def test_update_mutable_fields_leaves_status_untouched(
993993
self, task_service, agent_repository, sample_agent, redis_stream_repository
994994
):
995-
"""The primitive is column-scoped: writing current_state leaves status untouched
996-
(the use-case stale-read clobber regression is guarded in test_tasks_use_case.py).
997-
"""
995+
"""Writing current_state leaves status untouched (column-scoped update)."""
998996
await create_or_get_agent(agent_repository, sample_agent)
999997
created_task = await task_service.create_task(
1000998
agent=sample_agent, task_name="task-for-noclobber"

agentex/tests/unit/use_cases/test_tasks_use_case.py

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -662,9 +662,7 @@ async def test_update_current_state_does_not_clobber_concurrent_status(
662662
agent_repository,
663663
sample_agent,
664664
):
665-
"""Regression: a stale RUNNING read racing a COMPLETED transition must not revert
666-
status on the current_state write. Fails on the old whole-row merge, passes column-scoped.
667-
"""
665+
"""Stale-read race: current_state write must not revert a concurrent status transition."""
668666
await create_or_get_agent(agent_repository, sample_agent)
669667
task = await task_service.create_task(
670668
agent=sample_agent, task_name="current-state-clobber-test"

0 commit comments

Comments
 (0)