Skip to content

fix(sdk): complete object union and structured error handling - #1093

Merged
bokelley merged 3 commits into
mainfrom
open-prs-1086-1088
Aug 26, 2026
Merged

fix(sdk): complete object union and structured error handling#1093
bokelley merged 3 commits into
mainfrom
open-prs-1086-1088

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

  • make legacy PostalArea values round-trip through public adapters and targeting models
  • expose concrete AccountReference union arms across all generated field annotations, including nested, compatibility, response, canonical-clone, and versioned paths
  • resolve decisioning AdcpError types lazily for both A2A and MCP without caching transient import failures
  • preserve standalone webhook imports by deferring decisioning-only server dependencies

Verification

  • 124 focused regression and code-generation tests passed
  • 41 lazy and standalone import tests passed
  • full source mypy passed across 1,163 files
  • Ruff, Black, Bandit, adopter type checks, and all pre-commit hooks passed
  • independent code, protocol, and stress reviews report no remaining blockers

Fixes #1086
Fixes #1087
Fixes #1088

Comment thread scripts/post_generate_fixes.py Outdated
aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Aug 26, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — bundled fix(sdk): with one non-blocking medium finding.

Checked:

  • Decisioning-import refactor (load-bearing): @lru_cache caches only success in _load_decisioning_adcp_error_types; the wrapper catches ImportError and returns (), so a transient mid-startup circular import no longer permanently disables structured errors. Regression tests in test_a2a_structured_error.py and test_mcp_structured_error.py prove the retry path. Correct shape.
  • Legacy PostalArea round-trip fix and concrete AccountReference union arms are codegen output paired with new post_generate_fixes.py helpers, not hand-edits — consistent with the "generated code is not source" governance.
  • No credential-in-ctx_metadata, import-layering, or CI-gate regressions observed.

Medium findings:

  • scripts/post_generate_fixes.py:1555 — AccountReference field expansion hardcodes exactly two union arms and would silently drop a future arm. Forward-compat gap worth tracking, but non-blocking.

Decision path: no critical/high findings (row 1 no). gated_paths is false (row 2 no). No (deleted) high-risk reasons (row 3 no). The single medium is category codegen/forward-compat, not data-loss/schema/infra (row 4 no). high_risk is true with (modified) files (row 5 needs a medium — but the medium finding is on scripts/post_generate_fixes.py, which is NOT one of the high-risk-flagged modified files; the flagged modified files under src/adcp/server/** and aliases.py carry no medium-or-higher finding), so row 5 does not fire. No prior escalate (row 6 no). No no-auto-approve team match (row 7 no). Only one medium finding (row 8 needs ≥3, no). Falls through to row 9 → approve.

Medium findings

  • scripts/post_generate_fixes.py:1555 — AccountReference field expansion hardcodes two arms, silently drops future arm

aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Aug 26, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean subsequent pass resolving the sole prior finding.

What I checked: This delta touches two files (scripts/post_generate_fixes.py and tests/test_code_generation.py). The prior run's single medium finding — the AccountReference field expansion hardcoding two union arms and silently dropping future ones — is resolved. The fix now parses core/account_ref.py via AST, extracts every RootModel union arm in order (union_arm_names, L40-48), fail-closes with RuntimeError on any unexpected union shape, tightens the idempotency guard from (?![12]) to (?!\d), and joins all discovered arms via a lambda. The idempotency test was extended to cover a three-arm union and run the fix twice.

Decision path: No critical/high findings (row 1 n/a). gated_paths is false (row 2 n/a). No (deleted) high-risk reasons — all server-path matches are (modified) (row 3 n/a). No medium findings at all in this run, so rows 4, 5, 8 don't fire. Prior decision was approve, not escalate (row 6 n/a). No no-auto-approve team match (row 7 n/a). Falls through to row 9 → approve.

The high-risk flag is true only because server/ files appear in the changed-file set, but this delta's actual edits are confined to the codegen post-processing script and its test. No new findings; net improvement over the prior pass.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean delta over a prior clean pass.

This PR removes the AccountReference RootModel wrapper, replacing it with the direct AccountReference1 | AccountReference2 union across generated_poc/account/*. The reviewer found no critical/high/medium findings. The only delta since the prior approving pass is two lines in tests/test_catalog_types.py updating assertions from req.account.root.account_id to req.account.account_id, which correctly tracks the RootModel removal (concrete union arms have no .root accessor). Test alignment is correct.

Decision-table walk:

  • Rows 1: no critical/high findings.
  • Row 2: gated_paths is false — not applicable despite review_decision: REVIEW_REQUIRED.
  • Row 3/4/5: high_risk is true (server/** and aliases.py modified), but there are no medium findings and no deletions in high-risk paths, so no escalation trigger fires.
  • Row 6: prior decision was approve, not escalate — sticky escalation N/A.
  • Row 7: no no-auto-approve team match.
  • Row 8: fewer than three medium findings (zero).
  • Row 9: approve.

No blocking or medium findings; the change is mechanically clean and semantically sound.

@bokelley
bokelley enabled auto-merge (squash) August 26, 2026 09:39
@bokelley
bokelley merged commit 337d0d8 into main Aug 26, 2026
27 checks passed
@bokelley
bokelley deleted the open-prs-1086-1088 branch August 26, 2026 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant