Harden behavior capabilities and provider selection fields#424
Conversation
Enforce the canonical capabilitiesAttributes shape ({capabilityType,
enabled}, no extra keys) and reject legacy string-list / {type: ...} shapes
at the SDK, MCP, and CLI boundaries via the typed BehaviorInput model.
capabilityType is not checked against a fixed set: any value passes through
and the API validates the enum on write, so new capabilities work without a
toolkit update.
Accept optional providerId / systemProviderId with at most one set per
behavior (reads resolve a single active provider), rejecting blank-when-set
values before they can reach the wire.
Extend the GET and update GraphQL selections so capabilitiesAttributes,
providerId, and systemProviderId round-trip.
Closes #410.
e04f815 to
6a7321f
Compare
There was a problem hiding this comment.
Summary
Reviewed with a local checkout of the PR head (checkout+tests): targeted SDK/MCP/CLI suites passed (229) and ruff was clean.
This lands the Phase 1 harden from #410 on top of the typed behavior models: canonical capabilitiesAttributes, provider exclusivity, and GET/update selections that round-trip capabilities and provider IDs. Ready to approve.
What worked well
- Validation sits on typed
BehaviorInputrather than dict-walking, with clear errors for{type: ...}, unknown capability keys, and dual provider IDs. Enum-softcapabilityTypeplus the skill/docs note that shape checks are not entitlement match the ticket. - GET and update GraphQL selections were extended together, with a service AST assert covering both documents.
Also noted
capabilitiesAttributes: ["advanced_ocr"]is correctly rejected, but the failure is still a generic Pydanticmodel_typecoercion error, so callers never see the canonical-shape guidance that_validate_capability_entriesalready writes for{type: ...}. A small boundary parse (for example aBeforeValidatoron the list) plusmatch=ontest_behavior_input_rejects_capability_string_listwould align that legacy path with the PR claim of actionable errors.
A legacy capabilitiesAttributes string list failed AiBehaviorCapabilityAttributes coercion with an opaque Pydantic model_type error before _validate_capability_entries ran, so callers never saw the canonical-shape guidance the other legacy shapes get.
Add a BeforeValidator that surfaces the actionable message for non-object entries; object entries (including {"type": ...}) still pass through to the after-validator unchanged. Tighten the string-list test with match=.
Good catch — fixed in 47173d8. You were right: the string-list path failed `AiBehaviorCapabilityAttributes` coercion with the opaque `model_type` error before `_validate_capability_entries` could run, so it never got the canonical-shape guidance the `{type: ...}` path does. Went with the `BeforeValidator` you suggested — `_reject_non_mapping_capability_entries` intercepts non-object entries and raises the same actionable message; object entries (including legacy `{type: ...}`) pass through untouched to the after-validator, so nothing else changes. Tightened `test_behavior_input_rejects_capability_string_list` with `match="must be an object, not str"` to lock it in. Now:
|
adriannoes
left a comment
There was a problem hiding this comment.
Summary
Re-checked after 47173d86. Local checkout+tests still green (229 on the targeted agent suites); CI lint/test/skills are green on this head.
The follow-up BeforeValidator on capabilitiesAttributes closes the string-list message gap from the earlier review: non-object entries now get the canonical-shape error, and test_behavior_input_rejects_capability_string_list locks it with match=. No remaining findings on my side.
What worked well
Tight, review-driven fix that preserves the {type: ...} path through the existing after-validator and only intercepts non-mapping entries. Approve.
Motivation
capabilitiesAttributeswas described three contradictory ways (tool docstrings said a list of strings, a unit test blessed{"type": ...}, the API only accepts{capabilityType, enabled}), and the toolkit passed every shape through — wrong payloads surfaced as opaque server errors or were silently ignored. Behavior LLM provider fields were not modeled, and the GET/update selections didn't request capabilities or provider IDs, so a GET → update round-trip silently discarded them.Outcome
capabilitiesAttributesentries must be exactly{"capabilityType": "<type>", "enabled": true|false}; legacy shapes (string lists,{"type": ...}) and unknown keys are rejected with actionable errors at the SDK, MCP, and CLI boundaries — declared on the typed models from 0. Prep: parse behavior actionParams into typed models #416, no dict-walking.capabilityTypeis deliberately not checked against a fixed set: any value passes through and the API validates the enum on write, so new capabilities work without a toolkit update. Docs name the common types with product aliases (IDP →advanced_ocr, Calculations & Analysis →math_operations) and state that shape validation is not plan entitlement — capabilities may require organization-level enablement.providerId/systemProviderIdwith at most one set (reads resolve a single active provider, so co-presence is unverifiable); blank-when-set provider IDs are rejected before they can dodge that check and reach the wire.capabilitiesAttributes { capabilityType enabled },providerId,systemProviderId.Testing
uv run pytest -m "not integration"+ ruff green. Exercised end-to-end against a dev org (pipe with the MCP server running this branch's code):providerIdalongsidesystemProviderIdfoo){"type": "advanced_ocr"}["advanced_ocr", ...])providerIdandsystemProviderIdadvanced_ocr: true,web_search: false) → GETenabled: falsefaithfulweb_search: true, null provider fields present) → GETcapabilityType(future_capability)capabilitiesAttributes+providerId/systemProviderIdround-tripStacked on #416 (typed behavior params). Closes #410.