Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
lan17
changed the title
| /** Context */ | ||
| ctx?: Record<string, never>; | ||
| /** Input */ | ||
| input?: unknown; |
There was a problem hiding this comment.
The server OpenAPI spec still exposes optional ctx and input fields on ValidationError, but this regenerated type drops both. This makes the client contract narrower than the real 422 payloads. If any UI code accesses error.ctx or error.input, it would break at runtime without a type error. Consider regenerating from the current OpenAPI output or restoring these optional fields.
| control_exists = resp.status_code == 409 | ||
| if control_exists: | ||
| # Control exists, get its ID | ||
| resp = await client.get("/api/v1/controls", params={"name": control_name}) |
There was a problem hiding this comment.
This GET endpoint is a partial, case-insensitive filter, but the code blindly uses controls[0]. If multiple controls match the substring, this could update/attach the wrong control. Consider filtering the returned list to an exact name == control_name match.
| ) | ||
|
|
||
| # Then: a validation or not-found error is returned | ||
| # This will fail because the agent doesn't exist yet |
There was a problem hiding this comment.
This comment is inaccurate - the agent is created above (line 28-34), so it does exist. The missing piece is the evaluator, not the agent. Since the agent exists, the response should deterministically be 422 EVALUATOR_NOT_FOUND. Allowing 404 here would mask an AGENT_NOT_FOUND regression. Consider asserting specifically on 422.
| payload = VALID_CONTROL_PAYLOAD.copy() | ||
| payload["description"] = f"Name: {control_name}, Data: {data}" |
There was a problem hiding this comment.
The data parameter is only embedded in the description string - the actual control definition always uses VALID_CONTROL_PAYLOAD. Callers passing different data values aren't actually varying the control definition. Consider using data or VALID_CONTROL_PAYLOAD as the payload to strengthen coverage of the new atomic create flow.
abhinav-galileo
left a comment
abhinav-galileo
left a comment
There was a problem hiding this comment.
Approving - but let's address this:
#139 (comment)
2 participants
Summary
dataonPUT /api/v1/controlsinstead of allowing name-only shell control creation422and persist nothingBug Explanation
For a create-with-definition flow, the client did:
PUT /api/v1/controlswith justnamePUT /api/v1/controls/{id}/datawith the definition422, the empty shell from step 2 was still in the DBThat is the concrete bug this PR fixes. Invalid create requests were not atomic, so callers could end up with persisted empty
{}controls even though the overall create-with-definition flow had failed.One nuance: the agent association part was caller-flow dependent. The root server-side invariant failure was the persisted shell control after invalid data, and that is what this change removes.
Testing
cd server && uv run pytest tests/test_controls.py tests/test_controls_additional.py tests/test_controls_validation.py tests/test_agents_additional.py tests/test_auth.py tests/test_control_compatibility.py tests/test_error_handling.py tests/test_evaluation_e2e.py tests/test_evaluation_error_handling.py tests/test_init_agent.py tests/test_init_agent_conflict_mode.py tests/test_new_features.py tests/test_policies.py tests/test_policy_integration.py -qmake lintmake typecheckmake sdk-ts-buildmake ui-typecheck(still fails on pre-existing unrelated UI dependency/type issues)Closes #138