Skip to content

feat(schema): hoist 13 duplicate inline enum sets into shared enums/ definitions#3174

Merged
bokelley merged 4 commits into
mainfrom
claude/issue-3166-hoist-inline-enum-duplicates
Apr 26, 2026
Merged

feat(schema): hoist 13 duplicate inline enum sets into shared enums/ definitions#3174
bokelley merged 4 commits into
mainfrom
claude/issue-3166-hoist-inline-enum-duplicates

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

@bokelley bokelley commented Apr 25, 2026

Closes #3166

Second tranche of inline-enum hoisting, same pattern as #3148. All 13 candidate enum sets were verified byte-identical across every call site before replacement.

What changed

13 new shared enum files added to static/schemas/source/enums/:

File Values Sites
match-type.json ["broad","phrase","exact"] 10 (5 files)
collection-kind.json ["series","publication","event_series","rotation"] 3
travel-time-unit.json ["min","hr"] 3
frame-rate-type.json ["constant","variable"] 2
scan-type.json ["progressive","interlaced"] 2
gop-type.json ["closed","open"] 2
moov-atom-position.json ["start","end"] 2
binary-verdict.json ["pass","fail"] 2
account-scope.json ["operator","brand","operator_brand","agent"] 2
governance-decision.json ["approved","denied","conditions"] 2
billing-party.json ["operator","agent","advertiser"] 2
feature-check-status.json ["passed","failed","warning","unevaluated"] 3
snapshot-unavailable-reason.json ["SNAPSHOT_UNSUPPORTED","SNAPSHOT_TEMPORARILY_UNAVAILABLE","SNAPSHOT_PERMISSION_DENIED"] 2

21 consumer schemas updated across media-buy/, core/, collection/, content-standards/, property/, account/, governance/, and creative/.

Non-breaking justification

Replacing {"type":"string","enum":[...]} with a $ref to an equivalent standalone schema produces an identical JSON Schema subgraph under draft-07. All validators and codegen tools behave identically. No enum values added, removed, or reordered. Wire format unchanged. minor bump is for new $id-bearing addressable schema resources (consistent with #3148 precedent).

Deliberately excluded candidates

  • geometry ["Polygon","MultiPolygon"] — GeoJSON RFC 7946 subset; hoisting would incorrectly imply AdCP owns this discriminator. Left inline in catchment.json and product-filters.json.
  • sync-action — Diverging value sets across consumers (sync-accounts-response has 4 values without "deleted", sync-audiences-response has all 5, sync-creatives-response has only 2). Cannot be safely unified into a single enum file.
  • account-status — Already hoisted in a prior batch; enums/account-status.json already exists and is $ref'd correctly.

Notable implementation details

  • governance-decision.json carries a $comment noting that summary.statuses.human_reviewed in get-plan-audit-logs-response.json is a supplementary counter, NOT a valid governance decision value — prevents future consumers from incorrectly iterating the enum file and dropping it.
  • binary-verdict.json named explicitly to distinguish from feature-check-status.json (4-value past-tense set) and any future 3-value sets with unevaluated.
  • snapshot-unavailable-reason.json preserves SCREAMING_SNAKE_CASE exactly as found in the inline originals.
  • default: "broad" preserved alongside $ref in product-filters.json — valid draft-07 behavior, functions as documentation hint.

Follow-up noted (not in scope)

get-media-buy-artifacts-response.json contains an unhoisted 3-value inline enum ["pass","fail","unevaluated"] for local_verdict. This is distinct from both binary-verdict and feature-check-status and should be filed as a follow-up.

Pre-PR review

  • ad-tech-protocol-expert: approved — non-breaking per spec; governance-decision naming correct; geometry/sync-action exclusions confirmed; minor bump confirmed
  • code-reviewer: approved — no blockers; changeset count corrected (20→21)

Milestone

Targets 3.1.0 (next open minor milestone). Please set via milestone selector if not auto-assigned.

Session: https://claude.ai/code/session_01ETBaMbHLSMBLW6CUfYHtHp

claude added 2 commits April 25, 2026 15:52
…definitions

Follows the approach of #3148. Adds 13 new $id-bearing enum files to
static/schemas/source/enums/ and updates 21 source schemas to $ref them
instead of repeating inline definitions. Wire format unchanged in all cases.

New enum files: match-type, collection-kind, frame-rate-type, scan-type,
gop-type, moov-atom-position, binary-verdict, account-scope,
governance-decision, billing-party, feature-check-status,
snapshot-unavailable-reason, travel-time-unit.

Intentionally NOT hoisted: geometry ["Polygon","MultiPolygon"] (GeoJSON
subset risk per protocol-expert review), sync-action (diverging value
sets across consumers), account-status (already hoisted in prior batch).

Closes #3166

https://claude.ai/code/session_01ETBaMbHLSMBLW6CUfYHtHp
@bokelley bokelley force-pushed the claude/issue-3166-hoist-inline-enum-duplicates branch from cc8a3e1 to 98561cd Compare April 25, 2026 15:57
@bokelley bokelley changed the title feat(schema): hoist 16 duplicate inline enum sets into shared enums/ files feat(schema): hoist 13 duplicate inline enum sets into shared enums/ definitions Apr 25, 2026
@bokelley
Copy link
Copy Markdown
Contributor Author

Independent post-PR review (code-reviewer + ad-tech-protocol-expert)

Both reviewers ran independently on the full diff. One blocker + a couple of follow-ups worth deciding before merge.

Required before merge

1. sync-action.json duplicates existing creative-action.json.

schemas/cache/3.0.0/enums/creative-action.json already exists with ["created","updated","unchanged","failed","deleted"] — same 5-value set as the proposed sync-action.json (["created","updated","unchanged","deleted","failed"], just different ordering). Adding a second enum file for the same wire vocabulary while trying to eliminate duplicate enums is exactly backwards — it hardens drift instead of fixing it. The next person adding a sync action picks one of the two randomly.

Two ways forward:

  • Reuse: drop the new sync-action.json; point sync-event-sources-response.json and sync-audiences-response.json at the existing creative-action.json. Pre-existing name is creative-flavored but the values aren't creative-specific, so a rename PR (sync-event-action.json?) and re-pointing all consumers in one shot would be cleaner long-term.
  • Justify: if these are genuinely different vocabularies that coincidentally share values today, add a $comment on each explaining the divergence so the next maintainer knows not to merge them.

The order discrepancy (failed,deleted vs deleted,failed) suggests these were authored separately by different people and never reconciled — exactly the kind of drift the hoisting initiative is supposed to surface and fix.

Should verify

2. x-status: experimental propagation through $ref for governance-decision.json.

JSON Schema $ref doesn't inherit annotations, and codegen tools that flatten $ref may drop the enum-level x-status. Both consumers (check-governance-response.json and get-plan-audit-logs-response.json) carry top-level x-status: experimental, so the consuming type stays marked experimental in practice — but worth confirming the codegen pipeline (scripts/generate-types.ts and src/lib/utils/capabilities.ts:149,486) actually walks $ref for x-status rather than relying on the parent. If it doesn't, x-status: experimental on the enum file is decorative and the field should move (or stay duplicated on the parents).

Confirmed clean

Code-review verified all 16 hoists against the GA cache (schemas/cache/3.0.0/):

  • Byte-identical at every site: feature-status (3 sites, identical order), sync-action (2 sites, identical order), account-scope (2 sites), match-type (4 sites, pedagogical order [broad,phrase,exact] not alphabetical).
  • Intentional exclusions correct: sync-accounts-response.json 4-value subset (no deleted) stays inline with explanatory $comment; check-governance-response.json allOf branch predicates (approved/conditions 2-value guard, const: denied) correctly left inline.
  • No subset/ordering near-misses: verdict.json 2-value [pass,fail] is distinct from the 3-value [pass,fail,unevaluated] at get-media-buy-artifacts-response.json — that 3-value site correctly stays inline.
  • Naming consistent with the existing 100+ files in static/schemas/source/enums/. governance-decision.json matches the governance-*.json prefix family. verdict.json is generic but matches existing generic names (pacing.json, exclusivity.json, channels.json); both verdict consumers are in content-standards/ (same domain) so the share is coherent.
  • No missed call-sites — every (file, count) pair in the PR description matched the GA-cache spot-checks.
  • Wire compatibility clean — no consumer pattern-matches inline enum arrays. Storyboards and validators consume bundled schemas via $ref resolution; codegen renames duplicates (the Foo1 artifact this PR fixes).
  • governance-decision vs creative-approval-status split is correct: denied (governance refusal) and rejected (creative review) are genuinely different acts in different planes; pre-existing naming inconsistency is intentional.

Optional follow-up (informational, not blocking)

Strict draft-07 ignores sibling description next to $ref, so the rich call-site description text on governance-decision, account-scope, billing-party, sync-action, and match-type is lost when those become {$ref, description}. First tranche (#3148) shipped this same pattern with payment-terms without observable breakage, so tooling tolerates it — but consider adding enumDescriptions to the new enum files where the call-site doc was rich. Protects against doc loss in stricter consumers.

Versioning

minor bump confirmed correct — additive $id-bearing schemas, no normative wire change. Matches first-tranche convention.


Triggered by independent SDK-side review (ad-tech-protocol-expert + code-reviewer).

@bokelley bokelley marked this pull request as ready for review April 26, 2026 21:22
@bokelley
Copy link
Copy Markdown
Contributor Author

Ad-tech-protocol-expert review summary:

Verdict: sound-with-caveats

Wire-compat: spot-checked billing-party, match-type, governance-decision — all byte-identical (values + order). 12+ enum sites verified by inspection. No silent behavior change.

Semver minor: correct. New $id-bearing files are additive surface; no enum values added/removed/reordered.

⚠️ Description loss at some call sites: per-call-site descriptions (e.g., billing's "The seller must either accept this billing model or reject the request") were retained at some sites but dropped at others (e.g., account_scope in core/account.json, kind in get-collection-list-response.json). The hoisted files compensate via enumDescriptions, but that's non-standard — downstream codegen (Pydantic, TypeScript) won't surface it the way inline description did. Worth a pass to ensure every call site retains call-specific context where it differs from the generic enum semantics.

⚠️ binary-verdict naming: 12 of 13 follow existing kebab-case domain-noun convention (cf. account-status, catalog-action, delivery-type). binary-verdict is the outlier — too generic, collides conceptually with any future pass/fail evaluator outside content standards. Recommend renaming to content-standards-verdict or record-verdict before merge.

Ready for review. The binary-verdict rename is the clearest action item; description-loss audit is more discretionary.

@bokelley bokelley merged commit feed616 into main Apr 26, 2026
10 checks passed
@bokelley bokelley deleted the claude/issue-3166-hoist-inline-enum-duplicates branch April 26, 2026 22:08
bokelley added a commit that referenced this pull request Apr 26, 2026
…3316)

The previous workflow filtered PR comments out via
`github.event.issue.pull_request == null` with a comment claiming
"auto-fix handles those" — but no auto-fix workflow exists, and the
slack-routing only routes `/triage` slash commands. PR review feedback
(like the reviewer summaries on #3170/#3174/#3225/#3226 earlier today)
sat unactioned because nothing else was wired to pick them up.

Drop the filter. Route PR comments to the same triage routine, with
two adaptations on the payload so the routine can branch:

- `is_pr: true|false` flag at the top of the prompt
- `pr` block with head_ref, base_ref, draft, state when is_pr=true
- MODE directive when is_pr=true: "apply requested fix as a follow-up
  commit on the PR's head branch, or post a reply if it's a question"

Self-loop guard widened to skip comments containing "Fixed by Claude
Code" in addition to the existing "Triaged by Claude Code" — so the
routine's own follow-up replies on PRs don't re-fire it.

Concurrency group already keys on `issue.number`, which GitHub assigns
sequentially across both issues and PRs, so PR runs won't collide with
issue runs even when numbers happen to match.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hoist remaining inline-enum duplicates (follow-up to #3144)

2 participants