fix(states): update states by state ID only#278
Conversation
9146f3e to
0235fad
Compare
✱ Stainless preview buildsThis PR will update the openapi python typescript
|
|
@greptile |
| if task_state and task_state.task_id == task_id: | ||
| # Update the state field but preserve other fields | ||
| task_state.state = state | ||
| return await self.task_state_repository.update(task_state) |
There was a problem hiding this comment.
@smoreinis any reason not having the task_id would introduce a regression?
There was a problem hiding this comment.
we should be fine here, thanks for double checking
Summary
state_idtask_idandagent_idupdate payloads accepted but ignoredDAuthorizedId(TaskChildResourceType.state, update)to resolvestate_idto the stored parenttask_idand authorize against that task before the handler runs.statefield; storedtask_idandagent_idare preserved by the read-modify-write flowVerification
uv run --group test pytest tests/unit/api/test_states_schema.py tests/unit/use_cases/test_states_use_case.py -qNote: the focused local state API integration test was attempted earlier, but Docker/Testcontainers could not reach the local Docker socket before executing the test body. GitHub Actions integration shards passed.
Greptile Summary
This PR simplifies the state-update path by removing
task_id/agent_idfromUpdateStateRequestand relying solely on thestate_idpath parameter. Authorization is handled upstream byDAuthorizedId(TaskChildResourceType.state, update), which looks up the parent task and checks the caller's permissions before the handler runs.UpdateStateRequestdrops the two previously-required parent-identifier fields; extra fields sent by legacy clients are silently ignored (confirmed by the new unit test).StatesUseCase.updatenow raisesItemDoesNotExist(→ 404) instead of silently returningNonewhen the state is not found, providing defensive protection for race conditions between the auth lookup and the handler.Confidence Score: 5/5
Safe to merge — the change correctly narrows the update contract to state_id-only routing, with authorization handled entirely by the existing DAuthorizedId dependency.
All changed files are consistent with each other: schema, route, use case, OpenAPI spec, and tests all agree on the new contract. The defensive ItemDoesNotExist guard in the use case correctly handles race conditions. Legacy clients sending task_id/agent_id in the request body continue to work without errors. New unit tests verify both the happy path and the not-found path.
No files require special attention.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant DAuthorizedId participant _get_parent_task_id participant StateRepo participant AuthService participant StatesUseCase Client->>DAuthorizedId: "PUT /states/{state_id}" DAuthorizedId->>_get_parent_task_id: resolve state_id _get_parent_task_id->>StateRepo: "get(id=state_id)" StateRepo-->>_get_parent_task_id: StateEntity _get_parent_task_id-->>DAuthorizedId: task_id DAuthorizedId->>AuthService: check_task_or_collapse_to_404 AuthService-->>DAuthorizedId: authorized DAuthorizedId-->>Client: state_id resolved Client->>StatesUseCase: "update(id=state_id, state=new_state)" StatesUseCase->>StateRepo: "get(id=state_id)" StateRepo-->>StatesUseCase: StateEntity StatesUseCase->>StateRepo: update(task_state) StateRepo-->>StatesUseCase: updated StateEntity StatesUseCase-->>Client: 200 StateReviews (2): Last reviewed commit: "chore(states): update OpenAPI spec" | Re-trigger Greptile