fix(types)!: expose object unions without RootModel - #1084
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| # - Use when the seller resolves accounts internally from brand identity | ||
| # - Requires brand reference + operator domain | ||
|
|
||
| AccountReference = AccountReference1 | AccountReference2 |
| # These public names intentionally expose the schema unions directly instead | ||
| # of the generator's outer RootModel wrappers. They compose cleanly in adopter | ||
| # annotations without imposing another wrapper around their constituent arms. | ||
| PostalArea = _Annotated[ |
| ] | ||
| """Postal-area union; validate raw values with ``TypeAdapter(PostalArea)``.""" | ||
|
|
||
| SignalRef = _Annotated[SignalRef1 | SignalRef2 | SignalRef3, Discriminator("scope")] |
| model_config = ConfigDict(extra="forbid") | ||
|
|
||
|
|
||
| account: AccountReference = StrictAccountReference(account_id="acct-1") |
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
| # - Use when the seller resolves accounts internally from brand identity | ||
| # - Requires brand reference + operator domain | ||
|
|
||
| AccountReference = AccountReference1 | AccountReference2 |
There was a problem hiding this comment.
MUST FIX: Breaking public-surface change under a non-breaking prefix. AccountReference, PostalArea, and SignalRef were public RootModel classes; this commit reshapes them into a union alias (542) and two Annotated aliases (597, 603). That changes the type signature of three public adcp.types exports: AccountReference.model_validate(...) / AccountReference(root=...) stop working, and isinstance(x, PostalArea) / isinstance(x, SignalRef) now raise TypeError on the Annotated form. The migrated call sites in this PR (AccountReference(root=...) → AccountReferenceById(...), .model_validate → TypeAdapter(...)) are exactly the adopter code that breaks.
Commit is fix(types): expose object unions without RootModel with an empty body — no !, no BREAKING CHANGE: footer. release-please cuts a patch from fix:, so the break ships without a major/breaking beta signal. Per repo policy this is a high. Carry fix!: or add a BREAKING CHANGE: footer. The PR body already has the migration note; only the commit signal is missing.
There was a problem hiding this comment.
Ladon verdict: Request changes
Request changes — 1 blocking finding.
Blocking findings
src/adcp/types/aliases.py:542— Breaking public-surface reshape (AccountReference, PostalArea, SignalRef converted from generated RootModel wrappers to composable union aliases — a type-signature change on publicadcp.*exports) shipped under a non-breakingfix:prefix with nofix!:/feat!:marker and noBREAKING CHANGE:footer or migration note. Per the repo's mandatory semver-signal gate, release-please cuts a minor fromfix:, so the break ships without a major. Add the breaking-change signal (fix!:/BREAKING CHANGE:footer) plus a migration note (PR body,MIGRATION_*.md, orCHANGELOG.md).
Note: the underlying refactor preserves load-bearing validation (PostalArea country/system BeforeValidator, SignalRef Discriminator("scope")) and is confirmed by new tests — the issue is purely the missing semver signal on the commit, which is a high finding here.
Blocking findings
- src/adcp/types/aliases.py:542 — Breaking public-surface reshape (AccountReference/PostalArea/SignalRef union aliases) shipped under a non-breaking fix: prefix without fix!:/BREAKING CHANGE: footer or migration note
BREAKING CHANGE: AccountReference, PostalArea, and SignalRef are now composable union aliases. Use TypeAdapter(Alias).validate_python(...) instead of Alias.model_validate(...).
bokelley
changed the title
c56446b to
5029a46
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — prior blocking finding resolved, no current findings.
The prior run flagged a [high] on src/adcp/types/aliases.py:542 — a breaking public-surface reshape (AccountReference, PostalArea, SignalRef from RootModel wrappers to composable union aliases) shipped under a non-breaking commit prefix. That finding is now resolved: the head commit is fix(types)! with a BREAKING CHANGE: footer plus migration notes in the PR body and docs, satisfying the mandatory semver signal on the public surface.
Forward-compat checks: discriminator ('scope') and PostalArea country/system validation are preserved via Annotated validators; AccountReference's two arms are disjoint under extra='forbid'. No fallback arm removed, no discriminator key changed on the wire. All migrated call sites and tests are consistent; no source site still relies on .model_validate / root= / .root for the reshaped names.
Decision-table walk: no critical/high findings (row 1 no). gated_paths is false (row 2 no). No (deleted) high-risk reasons — only (modified) (row 3 no). No medium findings at all, so rows 4, 5, 8 do not fire. Prior decision was request-changes, not escalate (row 6 no). No no-auto-approve team match (row 7 no). Falls through to row 9 → approve.
Note on review_decision=CHANGES_REQUESTED: this only forces escalate under row 2 when gated_paths is true, which it is not here. It does not otherwise block an approve. high_risk is true but only on (modified) files with zero medium-or-higher findings, so it is presumed safe per the reasons guidance.
1 participant
Summary
AccountReference,PostalArea, andSignalRefas composable public union aliases instead of generated outerRootModelwrappersPostalAreacountry/system validation andSignalRefdiscriminator semantics throughAnnotatedvalidatorsTypeAdapter, and add an adopter type-check contractFixes #1077.
Migration
Code that called
AccountReference.model_validate(raw),PostalArea.model_validate(raw), orSignalRef.model_validate(raw)should useTypeAdapter(TheAlias).validate_python(raw). Construct known account arms withAccountReferenceByIdorAccountReferenceByNaturalKey. Wire shapes are unchanged.Validation
7268 passed, 77 skipped, 9 deselected, 1 xfailedmake typecheck-allmake lint