Skip to content

Commit cea9411

Browse files
committed
fix(agentex): address audit — atomic current_state write, clear-to-null, bounds
Resolve the review findings on the current_state observability change: - Blocker: route current_state (and task_metadata) through a new column-scoped atomic UPDATE (TaskRepository/AgentTaskService.update_mutable_fields) instead of update_task's whole-row session.merge of a stale entity. Touching only the supplied columns means a concurrent status transition or params merge can no longer be silently reverted (terminal task revert / deleted task resurrection / lost param edit). update_task keeps its full-row semantics for the status-writing callers (delete/fail/forward) that need them. - Support clearing current_state: an UNSET sentinel distinguishes an explicit null (clears the label) from an omitted field (left untouched), driven off the request's model_fields_set. - Bound the label: String(255) column + max_length on the request/response schema (new field, no back-compat risk) so it can't amplify unboundedly onto every task_updated SSE payload. Single source of truth: CURRENT_STATE_MAX_LENGTH. - Make the migration idempotent (ADD/DROP COLUMN IF [NOT] EXISTS), width 255. - Align the domain-entity field title with the API schema title. - Fix stale migration-linter path/flag in CLAUDE.md (scripts/ci_tools/migration_lint.py --base). - Tests: single-atomic-write (spy), status no-clobber regression, clear-to-null, by-name route, empty-string, over-length 422, combined update; robust bounded wait in the new stream test. Regenerated openapi.yaml.
1 parent 225e3a7 commit cea9411

14 files changed

Lines changed: 453 additions & 39 deletions

File tree

CLAUDE.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,7 @@ Use the escape hatch for "this needs a maintenance window with traffic shifted a
358358

359359
##### Anti-pattern → linter rule reference
360360

361-
The migration linter at `agentex/scripts/lint_migrations.py` enforces these rules at PR time via `.github/workflows/migration-lint.yml`. It only checks files changed vs the PR base, so existing migrations are not retro-flagged. The mapping below is what the linter catches:
361+
The migration linter at `agentex/scripts/ci_tools/migration_lint.py` enforces these rules at PR time via `.github/workflows/migration-lint.yml`. It only checks files changed vs the PR base, so existing migrations are not retro-flagged. The mapping below is what the linter catches:
362362

363363
| Anti-pattern | Linter rule |
364364
|---|---|
@@ -371,7 +371,7 @@ The migration linter at `agentex/scripts/lint_migrations.py` enforces these rule
371371
Run the linter locally before pushing:
372372

373373
```bash
374-
agentex/scripts/lint_migrations.py --base-ref origin/main
374+
agentex/scripts/ci_tools/migration_lint.py --base origin/main
375375
```
376376

377377
##### Other rules

agentex/database/migrations/alembic/versions/2026_07_22_1200_add_task_current_state_b2c3d4e5f6a7.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,12 @@
2121
def upgrade() -> None:
2222
# Opaque label mirroring an agent's StateMachine current state. Nullable and
2323
# additive; agents opt in by emitting it. Metadata-only add, non-blocking.
24-
op.add_column('tasks', sa.Column('current_state', sa.String(), nullable=True))
24+
# Idempotent (IF NOT EXISTS) so re-running on an environment that already has
25+
# the column is a no-op. Width matches TaskORM.current_state (String(255)).
26+
op.execute(
27+
"ALTER TABLE tasks ADD COLUMN IF NOT EXISTS current_state VARCHAR(255)"
28+
)
2529

2630

2731
def downgrade() -> None:
28-
op.drop_column('tasks', 'current_state')
32+
op.execute("ALTER TABLE tasks DROP COLUMN IF EXISTS current_state")

agentex/openapi.yaml

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6591,6 +6591,7 @@ components:
65916591
current_state:
65926592
anyOf:
65936593
- type: string
6594+
maxLength: 255
65946595
- type: 'null'
65956596
title: Opaque label mirroring the agent's StateMachine current state; null
65966597
when the agent does not emit one. Orthogonal to 'status'.
@@ -6820,6 +6821,7 @@ components:
68206821
current_state:
68216822
anyOf:
68226823
- type: string
6824+
maxLength: 255
68236825
- type: 'null'
68246826
title: Opaque label mirroring the agent's StateMachine current state; null
68256827
when the agent does not emit one. Orthogonal to 'status'.
@@ -7411,8 +7413,10 @@ components:
74117413
current_state:
74127414
anyOf:
74137415
- type: string
7416+
maxLength: 255
74147417
- type: 'null'
7415-
title: If provided, replaces the task's current_state label.
7418+
title: If provided, replaces the task's current_state label; pass null to
7419+
clear it, omit to leave it unchanged.
74167420
type: object
74177421
title: UpdateTaskRequest
74187422
ValidationError:

agentex/src/adapters/orm.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,10 @@ class TaskORM(BaseORM):
7777
task_metadata = Column(JSONB, nullable=True)
7878
# Opaque, framework-agnostic label mirroring an agent's StateMachine current
7979
# state. Written best-effort by the agent on each transition; reconciled on
80-
# read. Orthogonal to `status` (Temporal workflow lifecycle).
81-
current_state = Column(String, nullable=True)
80+
# read. Orthogonal to `status` (Temporal workflow lifecycle). Bounded: a
81+
# state label is short, and the value is echoed onto every task_updated SSE
82+
# payload, so we cap it to avoid unbounded stream amplification.
83+
current_state = Column(String(255), nullable=True)
8284
# Many-to-Many relationship with agents
8385
agents = relationship("AgentORM", secondary="task_agents", back_populates="tasks")
8486

agentex/src/api/routes/tasks.py

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
from src.domain.entities.tasks import TaskStatus as DomainTaskStatus
2323
from src.domain.services.authorization_service import DAuthorizationService
2424
from src.domain.use_cases.streams_use_case import DStreamsUseCase
25-
from src.domain.use_cases.tasks_use_case import DTaskUseCase
25+
from src.domain.use_cases.tasks_use_case import UNSET, DTaskUseCase
2626
from src.utils.authorization_shortcuts import (
2727
DAuthorizedId,
2828
DAuthorizedName,
@@ -196,7 +196,13 @@ async def update_task(
196196
id=task_id,
197197
task_metadata=request.task_metadata,
198198
merge_params=request.merge_params,
199-
current_state=request.current_state,
199+
# Pass through only when the client actually sent the key, so an explicit
200+
# null clears the label while an omitted field leaves it untouched.
201+
current_state=(
202+
request.current_state
203+
if "current_state" in request.model_fields_set
204+
else UNSET
205+
),
200206
)
201207
return Task.model_validate(updated_task_entity)
202208

@@ -218,7 +224,13 @@ async def update_task_by_name(
218224
name=task_name,
219225
task_metadata=request.task_metadata,
220226
merge_params=request.merge_params,
221-
current_state=request.current_state,
227+
# Pass through only when the client actually sent the key, so an explicit
228+
# null clears the label while an omitted field leaves it untouched.
229+
current_state=(
230+
request.current_state
231+
if "current_state" in request.model_fields_set
232+
else UNSET
233+
),
222234
)
223235
return Task.model_validate(updated_task_entity)
224236

agentex/src/api/schemas/tasks.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,11 @@
77
from src.api.schemas.agents import Agent
88
from src.utils.model_utils import BaseModel
99

10+
# Upper bound on the opaque `current_state` label. Kept in sync with the
11+
# `TaskORM.current_state` column width (String(255)); a state label is short and
12+
# the value rides every task_updated SSE payload, so it is capped.
13+
CURRENT_STATE_MAX_LENGTH = 255
14+
1015

1116
class TaskRelationships(str, Enum):
1217
"""Task relationships that can be loaded"""
@@ -65,6 +70,7 @@ class Task(BaseModel):
6570
)
6671
current_state: str | None = Field(
6772
None,
73+
max_length=CURRENT_STATE_MAX_LENGTH,
6874
title=(
6975
"Opaque label mirroring the agent's StateMachine current state; "
7076
"null when the agent does not emit one. Orthogonal to 'status'."
@@ -96,7 +102,11 @@ class UpdateTaskRequest(BaseModel):
96102
)
97103
current_state: str | None = Field(
98104
None,
99-
title="If provided, replaces the task's current_state label.",
105+
max_length=CURRENT_STATE_MAX_LENGTH,
106+
title=(
107+
"If provided, replaces the task's current_state label; "
108+
"pass null to clear it, omit to leave it unchanged."
109+
),
100110
)
101111

102112

agentex/src/domain/entities/tasks.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,10 @@ class TaskEntity(BaseModel):
6868
)
6969
current_state: str | None = Field(
7070
None,
71-
title="Opaque label mirroring the agent's StateMachine current state",
71+
title=(
72+
"Opaque label mirroring the agent's StateMachine current state; "
73+
"null when the agent does not emit one. Orthogonal to 'status'."
74+
),
7275
)
7376

7477
# allow extra fields for agents relationships

agentex/src/domain/repositories/task_repository.py

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
from collections.abc import Sequence
22
from datetime import UTC, datetime, timedelta
3-
from typing import Annotated, Literal
3+
from typing import Annotated, Any, Literal
44

55
from fastapi import Depends
66
from sqlalchemy import cast, distinct, func, select, update
@@ -254,6 +254,42 @@ async def merge_params(self, task_id: str, patch: dict) -> TaskEntity | None:
254254
return None
255255
return TaskEntity.model_validate(row)
256256

257+
async def update_mutable_fields(
258+
self, task_id: str, fields: dict[str, Any]
259+
) -> TaskEntity | None:
260+
"""Atomically set only the given scalar columns on a single task row.
261+
262+
Unlike ``update`` (a whole-row ``session.merge`` of a possibly-stale
263+
entity, which rewrites every column and can clobber a concurrently
264+
changed ``status``/``params``), this issues a single
265+
``UPDATE ... SET <only these columns> WHERE id = :id RETURNING *``.
266+
Touching only the supplied columns means a concurrent status transition
267+
or param merge is never reverted. Returns the updated entity, or
268+
``None`` if no task with ``task_id`` exists.
269+
270+
``fields`` values are applied verbatim, so passing ``current_state=None``
271+
clears the column (callers distinguish "clear" from "omit" upstream).
272+
"""
273+
if not fields:
274+
return await self.get(id=task_id)
275+
276+
async with (
277+
self.start_async_db_session(True) as session,
278+
async_sql_exception_handler(),
279+
):
280+
stmt = (
281+
update(TaskORM)
282+
.where(TaskORM.id == task_id)
283+
.values(**fields)
284+
.returning(TaskORM)
285+
)
286+
result = await session.execute(stmt)
287+
row = result.scalar_one_or_none()
288+
await session.commit()
289+
if row is None:
290+
return None
291+
return TaskEntity.model_validate(row)
292+
257293
async def transition_status(
258294
self,
259295
task_id: str,

agentex/src/domain/services/task_service.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,40 @@ async def update_task(self, task: TaskEntity) -> TaskEntity:
242242

243243
return updated_task
244244

245+
async def update_mutable_fields(
246+
self, task_id: str, fields: dict[str, Any]
247+
) -> TaskEntity | None:
248+
"""Atomically update only the given scalar columns on a task, then
249+
publish a task_updated event.
250+
251+
Column-scoped (see ``TaskRepository.update_mutable_fields``) so it cannot
252+
clobber a concurrently changed ``status``/``params`` — unlike
253+
``update_task``'s whole-row merge, which the status-writing callers
254+
(delete/fail/forward) still rely on. Returns the updated entity, or
255+
``None`` if the task no longer exists.
256+
"""
257+
updated_task = await self.task_repository.update_mutable_fields(
258+
task_id, fields
259+
)
260+
if updated_task is None:
261+
return None
262+
263+
try:
264+
topic = get_task_event_stream_topic(task_id=task_id)
265+
await self.stream_repository.send_data(
266+
topic,
267+
TaskStreamTaskUpdatedEventEntity(
268+
type="task_updated", task=updated_task
269+
).model_dump(mode="json"),
270+
)
271+
logger.info(f"task_updated event published to topic: {topic}")
272+
except Exception as e:
273+
logger.error(
274+
f"Error sending task_updated event to stream: {e}", exc_info=True
275+
)
276+
277+
return updated_task
278+
245279
async def merge_task_params(self, task_id: str, patch: dict) -> TaskEntity | None:
246280
"""Atomically shallow-merge ``patch`` into ``tasks.params``. Returns
247281
the updated entity, or ``None`` if no task with ``task_id`` exists.

agentex/src/domain/use_cases/tasks_use_case.py

Lines changed: 41 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,15 @@
1111
logger = make_logger(__name__)
1212

1313

14+
class _Unset:
15+
"""Sentinel distinguishing "field omitted" from "field explicitly set to null"
16+
in a PATCH-style update, so an explicit null can clear a column while an
17+
omitted field is left untouched."""
18+
19+
20+
UNSET: Any = _Unset()
21+
22+
1423
class TasksUseCase:
1524
"""
1625
Use case for managing tasks. Handles CRUD operations and delegates task operations to ACP servers.
@@ -95,13 +104,20 @@ async def update_mutable_fields_on_task(
95104
name: str | None = None,
96105
task_metadata: dict[str, Any] | None = None,
97106
merge_params: dict[str, Any] | None = None,
98-
current_state: str | None = None,
107+
current_state: str | None | _Unset = UNSET,
99108
) -> TaskEntity:
100-
"""Update mutable fields on a task entity. This is used by our API since not all fields should be mutable."""
109+
"""Update mutable fields on a task entity. This is used by our API since not all fields should be mutable.
110+
111+
``current_state`` uses the UNSET sentinel so an explicit null clears the
112+
column while an omitted field leaves it untouched. ``task_metadata`` keeps
113+
its legacy semantics (``None`` means "not supplied").
114+
"""
101115

102116
if not id and not name:
103117
raise ClientError("Either id or name must be provided")
104118

119+
current_state_supplied = not isinstance(current_state, _Unset)
120+
105121
# todo: make this a transaction?
106122
task_entity = await self.task_service.get_task(id=id, name=name)
107123
if task_entity.status == TaskStatus.DELETED:
@@ -111,32 +127,39 @@ async def update_mutable_fields_on_task(
111127
raise ItemDoesNotExist(f"Task {name} not found")
112128

113129
# No-op if no mutable field was supplied.
114-
if task_metadata is None and merge_params is None and current_state is None:
130+
if (
131+
task_metadata is None
132+
and merge_params is None
133+
and not current_state_supplied
134+
):
115135
return task_entity
116136

117137
# `merge_params` is a separate atomic JSONB shallow-merge so concurrent
118-
# callers don't overwrite each other's fields (vs reading→mutating→writing
119-
# the whole params dict on task_entity). Run it first so the refreshed
120-
# entity it returns becomes the base we apply `task_metadata` on top of;
121-
# otherwise the `task_entity = merged` reassignment would discard an
122-
# in-memory metadata change made before the merge.
138+
# callers don't overwrite each other's fields. Run it first so the
139+
# refreshed entity it returns becomes the value we return if no scalar
140+
# field updates follow.
123141
if merge_params:
124142
merged = await self.task_service.merge_task_params(
125143
task_entity.id, merge_params
126144
)
127145
if merged is not None:
128146
task_entity = merged
129147

130-
# Apply the whole-row field updates (task_metadata, current_state) in a
131-
# single update_task write so they emit one task_updated event rather than
132-
# two. current_state rides the same publish that already powers reactive
133-
# task_updated delivery to stream consumers.
134-
if task_metadata is not None or current_state is not None:
135-
if task_metadata is not None:
136-
task_entity.task_metadata = task_metadata
137-
if current_state is not None:
138-
task_entity.current_state = current_state
139-
task_entity = await self.task_service.update_task(task=task_entity)
148+
# Apply task_metadata/current_state via a single column-scoped atomic
149+
# update — one task_updated publish, and (unlike a whole-row merge) no
150+
# risk of clobbering a concurrently changed status/params. current_state
151+
# rides that publish, powering reactive delivery to stream consumers.
152+
fields: dict[str, Any] = {}
153+
if task_metadata is not None:
154+
fields["task_metadata"] = task_metadata
155+
if current_state_supplied:
156+
fields["current_state"] = current_state
157+
if fields:
158+
updated = await self.task_service.update_mutable_fields(
159+
task_entity.id, fields
160+
)
161+
if updated is not None:
162+
task_entity = updated
140163

141164
return task_entity
142165

0 commit comments

Comments
 (0)