feat: support typed variant attributes - #307
Conversation
Variant attributes now carry a declared type. An attribute gains an optional `kind` (string, number, boolean, enum, object) and, for enums, a `config` with the allowed values, matching the AttributeType the platform added in the variant-attribute-types project. Values are dual-written on push the way every platform write path does: the legacy stringified `values` map plus native `typed_values`, which wins on read. Pulls prefer `typedValues` from the projection and fall back to the whole `values` map when it is malformed, mirroring safeMergeVariantValues. A legacy value that only exists in the string map is parsed against the declared kind, so an attribute typed in Agent Studio but never re-saved still reads natively. Values are validated against their kind locally, so a mismatch is caught at push time naming the variant it came from rather than surfacing on a live call. On disk, `values` stays a map of strings -- the same strings the platform keeps in its legacy map -- with `kind` saying how to read them. Writing them natively would be prettier, but an ADK released before typed attributes calls .strip() on every value and would fail on an int, bool or nested map. Native YAML is still accepted on input, and both spellings hash identically so neither shows as a diff against the other. A project that never adopts a type sees no change to its file. New variants seed typed attributes with a native null rather than "", which the platform rejects for any non-string kind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Ruari-Phipps
left a comment
There was a problem hiding this comment.
Overall looks good!
There's quite a few verbose claude comments that could be cleaned up and would be good to add this in the test project included.
Would also be good to do manual QA of this, if you haven't already
| # Values are written as strings — the same strings the platform keeps in its | ||
| # legacy `values` map, with `kind` saying how to read them. Writing them | ||
| # natively would be prettier, but an ADK released before typed attributes | ||
| # calls .strip() on every value and would die on an int, bool or nested map | ||
| # the moment it read a file a newer ADK had written. |
There was a problem hiding this comment.
We don't have to maintain compatibility like this.
There was a problem hiding this comment.
Could we also update the test project in in tests please
| config: | ||
| values: [basic, premium] |
There was a problem hiding this comment.
Would there be anything else in this field potentially in the future? Just wondering if values should be top level?
There was a problem hiding this comment.
No plans to extend it for this feature. enum is the only kind with metadata today. It's nested so a future kind can carry its own settings without every other kind growing a field it ignores.
| @@ -385,6 +656,66 @@ def validate(self, resource_mappings: list[ResourceMapping], **kwargs): | |||
| f"Missing variants for variant attribute: {[known_variant_id_to_name[variant_id] for variant_id in missing_variants]}" | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
When testing I got this error:
ERROR:poly.handlers.sync_client:Failed to send commands: Error: {'name': 'ZodValidationError', 'message': '[command 1: variantCreateAttribute "New enum" (VARIANT_ATTRIBUTES-ba66e2d9)] Validation error: Invalid value, name should be Pythonic at "name"', 'details': [{'validation': 'regex', 'code': 'invalid_string', 'message': 'Invalid value, name should be Pythonic', 'path': ['name']}]}
This is outside the scope of this PR, but would be good to include it in the validation while we're here
There was a problem hiding this comment.
Bug I also found.
- Push enum variant attribute with a config
- Change attribute type and push
- Pull -> config returns and it fails validation
Either:
- Don't write config if not enum type
- Or we need to correctly clear the config when pushed empty. This might require a backend change
Drop a stale enum config on read. Changing an attribute away from enum leaves its old enum_config on the platform, so pulling it back gave a non-enum attribute a config its kind can't have, which then failed validation on the next push. Validate attribute names as Python identifiers. The platform rejects anything else with a ZodValidationError on push; catching it locally names the attribute and the rule instead. Reserved words are Python's own list, which is exactly what the platform checks. Write values in their declared type rather than as strings. The string encoding was there to keep files readable by an ADK released before typed attributes; that compatibility isn't needed. Add typed attributes to the test project, one per kind, and rename its attributes to valid identifiers now that names are checked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
Drop a stale enum config on read. Changing an attribute away from enum leaves its old enum_config on the platform, so pulling it back gave a non-enum attribute a config its kind can't have, which then failed validation on the next push. Validate attribute names as Python identifiers. The platform rejects anything else with a ZodValidationError on push; catching it locally names the attribute and the rule instead. Reserved words are Python's own list, which is exactly what the platform checks. Write values in their declared type rather than as strings. The string encoding was there to keep files readable by an ADK released before typed attributes; that compatibility isn't needed. Add typed attributes to the test project, one per kind, and rename its attributes to valid identifiers now that names are checked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage Report
Changed file coverage
|
Summary
Variant attributes can now declare a type. ADK reads an attribute's
kind(and anenum's
config) from the platform, validates every value against it locally, anddual-writes native typed values on push — matching the
AttributeTypesupport theplatform added in the Variant Attribute Types project.
Motivation
The platform now stores variant attribute values twice: the legacy stringified
valuesmap and a nativetypedValuesmap that wins on read. ADK knew aboutneither, so it flattened every typed attribute to a string on pull, and its pushes
were rejected outright by the backend's type validation once a project had a typed
attribute.
Linear project: https://linear.app/poly-ai/project/variant-attribute-types-a7bc31d07a9c/overview
Changes
kind(string,number,boolean,enum,object) and, for enums, aconfiglisting the allowed values.typedValues, falls back to the wholevaluesmap when it ismalformed, and parses a legacy string against the declared kind — so an attribute
typed in Agent Studio but never re-saved still reads natively.
mismatch came from instead of surfacing it on a live call.
valuesstays a map of strings on disk, withkindsaying how to read them. AnADK released before typed attributes calls
.strip()on every value and wouldfail on a native int, bool or nested map in a file a newer ADK had written. Native
YAML is still accepted on input, and both spellings hash identically so neither
shows as a diff against the other.
"", which theplatform rejects for any non-string kind.
blank typed attribute is omitted from the deployed agent (a blank string
substitutes empty), and Agent Studio's Date & time / Opening hours / Voice types
are stored as
stringorenumhere.A project that never adopts a type sees no change to its file at all.
Test strategy
poly <command>)1585 tests pass. New coverage pins the wire round trip per kind, the projection read
including the malformed-
typedValuesfallback, YAML round trips for every kind, andthe backward-compatible encoding — including a test that runs the pre-types reader's
exact comprehension over what this writes, plus a guard asserting that same reader
really does fail on a native value.
Not yet exercised against a live project. Worth a real
poly pushof a typedattribute before merging, since this changes the push payload.
Checklist
ruff check .andruff format --check .passpytestpassespolyCLI interface (or migration path documented)