[sujanchalla0510]

Fixes #365.

CreativeResult drifted from static/schemas/source/creative/sync-creatives-response.json: it was missing the 8 spec-defined fields (account, assigned_to, assignment_errors, changes, expires_at, platform_id, preview_url, warnings) and defined rejection_reason, which exists nowhere in the spec.

Changes (current head 8aa637d; description refreshed 2026-10-16 per @torbenbrodt's review):

• adcp/types.go: add the 8 missing fields, all omitempty (non-breaking, per the triage on CreativeResult struct missing 8 spec fields, defines 1 not in spec #365). The pinned 3.1.9 bundle has exactly the 12 spec properties from CreativeResult struct missing 8 spec fields, defines 1 not in spec #365, and this change covers all 12.
• adcp/v3/types.go: the same 8 fields plus revision_id, localization, and macro_resolution_results — all 15 spec properties of the bundle this branch pins (3.2.0-rc.3; main has since moved on to 3.2.2, this branch's pin is unchanged). localization / macro_resolution_results are present but kept shallow: Localization is any and MacroResolutionResults uses the generated MacroResolutionResult type, rather than hand-expanding their object graphs.
• Schema pairing: CreativeResult is paired with the sync-creatives-response item schema in HAND_WRITTEN_SCHEMA_SPECS in both modules and removed from EXEMPT; a scoped ALLOWED_EXTRA_FIELDS allowance documents the deprecated rejection_reason.
• Deprecate (not remove) RejectionReason — removing it would break Go callers. Its deprecation comment now notes failure detail belongs in errors[] as core/error.json objects (see Follow-up: CreativeResult.Errors []string vs []AdcpError drift (breaking re-type deferred from #545) #546).
• Errors stays []string — re-typing to structured errors would be breaking; tracked in Follow-up: CreativeResult.Errors []string vs []AdcpError drift (breaking re-type deferred from #545) #546 together with the assignment_errors decode gap.
• Fixtures use the valid CreativeStatus value approved.

How tested: go build ./..., go vet ./..., and go test ./... in both the adcp and adcp/v3 modules — all pass. JSON round-trip tests (adcp/creative_result_test.go, adcp/v3/creative_result_test.go) cover the new wire fields and backward compat of the deprecated field, including a whole-JSON localization round-trip test in v3. @torbenbrodt additionally verified 8aa637d merged onto main (188cdcf): generator tests, regeneration with no diff, lint.py --strict and the any-coverage gate in both modules, and go vet + go test -race for root, adcp, adcp/v3 and the reference agents.

Follow-ups: #546 (the Errors / assignment_errors re-type), #567 (CreativeInput revision_id + localization, v3 only — the 3.1.9 schemas carry neither field).

[aao-secretariat]

Medium: Errors []string is still off-shape for the schema this PR claims to align to. Across the generated types every response errors[] is []AdcpError (AdcpError = map[string]any) — including the sibling sync task SyncGovernanceAccountResult.Errors []AdcpError (adcp/v3/types_gen.go:11154), and per-item errors carry error-code.json code/message objects. A real failed-creative payload "errors":[{"code":"invalid_format","message":"..."}] will fail json.Unmarshal (cannot unmarshal object into string), taking down the entire sync-creatives response parse on the failure path.

The PR description notes re-typing is breaking and deferred per triage — fair, and the fix needs a fix!/major marker + human sign-off, so it doesn't belong in this additive PR. But the "align CreativeResult with the schema" claim leaves the failure path unaligned, and the round-trip test only exercises "errors": [], so nothing here catches it. Worth a follow-up issue with a populated-errors payload pinning the decision.

[aao-secretariat]

Medium: Same errors shape gap as the adcp module. In v3 the AdcpError-object convention is even more load-bearing: AdcpError = map[string]any (adcp/v3/types_gen.go:13) and every generated response types errors as []AdcpError, e.g. sibling SyncGovernanceAccountResult.Errors []AdcpError (:11154). []string here will fail to unmarshal a real failed-creative payload with structured {code,message} error objects, breaking the sync-creatives failure path.

Deferring the re-type is defensible (breaking; needs fix!/major + human sign-off), but flag it as a known remaining drift rather than folding it under "aligned." The round-trip test uses "errors": [] and never exercises the object case.

[aao-secretariat]

Ladon verdict: Approve

Approve — clean, additive fix closing the CreativeResult drift in #365.

What I checked:

• Adds the 8 missing spec fields (plus revision_id in v3) to the hand-written CreativeResult struct, deprecates rather than removes RejectionReason (no exported-symbol removal → no breaking-change/conventional-commit trigger).
• Schema↔generated-types coherence rule structurally satisfied: CreativeResult is the sanctioned hand-written escape hatch (listed in KNOWN_TYPES/EXEMPT), so edits in adcp/types.go are within the supported escape hatch — no adcp/types_gen.go hand-edit, no adcp/schemas change without regen.
• No TMP signing, identity-agent TEE, or protocol-managed-skill paths touched.
• Round-trip tests shipped for both call sites; keyed literals used; testify already declared.

Decision path: no critical/high/medium findings (row 1/4/5 do not fire). gated_paths is false (row 2 n/a). high_risk is true but the only match, adcp/types.go, is (modified) with zero medium-or-higher findings — per the high_risk guidance a modification with no medium concern is presumed safe, and row 5 requires a medium finding to escalate. No no-auto-approve team match (row 7 n/a). Fewer than three medium findings (row 8 n/a). Falls through to row 9 → approve.

Non-blocking notes from the reviewer: field-shape fidelity could not be independently verified (no network access to upstream schemas); Errors remains []string per the #365 triage.

[sujanchalla0510]

Thanks for the careful review — the Errors []string vs []AdcpError drift is real, and it genuinely lives outside this PR's scope (breaking re-type needs a fix!/major marker + maintainer sign-off).

I've opened a follow-up issue pinning the decision: #546 — it documents the unmarshal failure path ({"errors":[{"code":"...","message":"..."}]} failing to parse on the sync-creatives failure path) and proposes the breaking re-type plus a populated-errors round-trip test. Leaving #545 additive as recommended.

[torbenbrodt]

Thanks for this, @sujanchalla0510. The eight fields from #365 and revision_id are typed correctly against the bundle main pins now (3.2.2), and creative_id/action stay required. I merged the branch onto current main locally: it merges cleanly, regeneration produces no diff, and lint.py --strict, the generator's Python tests, go vet and go test pass in both modules. Four additive things I'd suggest before merge, none of them breaking:

1. localization and macro_resolution_results can go in now, and v3 drops them on decode until they do. Since #555, main generates MacroResolutionResult, and CreativeCapabilities.Localization in adcp/v3/types.go already holds a localization object as any. So this is two lines:

   Localization           any                     `json:"localization,omitempty"`
   MacroResolutionResults []MacroResolutionResult `json:"macro_resolution_results,omitempty"`

   The schema makes localization "Required for every accepted creative whose request included a non-null localization object". This item validates against 3.2.2:

   {"creative_id":"summer_image_localized","action":"created","status":"pending_review","localization":{"default_locale_variant_id":"loc_en_us","unmatched_locale_action":"do_not_serve","locale_matching":"rfc4647_lookup","variants":[{"locale_variant_id":"loc_en_us","locale":"en-US","role":"source","assets":{"image":{"asset_type":"image","url":"https://cdn.example.com/summer-en.jpg","width":1080,"height":1080}}}]}}

   Unmarshalled into this PR's CreativeResult and marshalled back, it comes out as {"creative_id":"summer_image_localized","action":"created","status":"pending_review"}. With the two fields added, it round-trips unchanged.

2. Pair CreativeResult with its schema in lint so it can't drift silently again (the triage on #365 asked for it to leave EXEMPT). Add 'CreativeResult': 'creative/sync-creatives-response.json#/oneOf/0/properties/creatives/items' to HAND_WRITTEN_SCHEMA_SPECS and remove it from EXEMPT in lint.py. On main, that check reports exactly the fields #365 listed (plus revision_id, localization, macro_resolution_results). On this branch it reports only localization, macro_resolution_results and the extra rejection_reason. After (1), only the deprecated field is left, so lint needs a small allowance for it. The same pairing in adcp/schemas (v1) reports only rejection_reason.

3. Tests: use a schema-valid fixture and compare the whole JSON. "status": "active" isn't a CreativeStatus (processing, pending_review, approved, suspended, rejected, archived), so both fixtures fail validation against the schema. With approved they pass. The re-encode step checks only that keys are present, and its list leaves out errors, which the round trip drops ("errors": [] in, absent out). An equality check on decode-then-encode, plus an action: "failed" item with a structured error, would pin #546.

4. Reword the deprecation comment. "Per-item failure detail belongs in Errors" currently leads to invalid output: Errors: []string{"Video duration 31s exceeds the 30s maximum"} encodes as "errors":["Video duration 31s exceeds the 30s maximum"], which fails the schema because the items are core/error.json objects. rejection_reason on its own still validates, because the items allow additional properties. Until #546 lands, something like: "Deprecated: not part of the AdCP schema. Failure detail belongs in errors[] as core/error.json objects; see #546." On #546's point 3: warnings items are type: string, so Warnings []string is already right.

Smaller:

• The v3 test comment says the package pins 3.2.0-rc.3; main moved to 3.2.2 in #553. The 15-property count still holds.
• assignment_errors allows non-string values for keys outside ^[a-zA-Z0-9_-]+$ (additionalProperties: true). So {"pkg.std":{"code":"PACKAGE_NOT_FOUND"}} is schema-valid, but it fails to decode into map[string]string.
• Outside this PR: CreativeInput carries no revision_id or localization, so a seller using the typed SyncCreatives hook can't see what it has to echo back.

For context, #564 proposes a CI check that round-trips schema examples through the Go types. As it stands it wouldn't reach CreativeResult, because sync-creatives-response.json has no examples and the type is hand-written, so here the lint pairing in (2) is the guard.

Disclosure: we're building a creative agent and a seller on adcp/v3, so sync_creatives results are what our buyers will read.

[sujanchalla0510]

Thanks for the detailed review, @torbenbrodt — addressed in b9829f3:

1. Fields added (v3): Localization any and MacroResolutionResults []MacroResolutionResult on CreativeResult, exactly as you suggested. Your localized payload now round-trips unchanged — pinned by a new whole-JSON equality test (TestCreativeResultLocalizationRoundTrip). One caveat: this branch predates feat(adcp/v3): generate validate_input request, response and result types #555, so MacroResolutionResult only exists on main; I verified locally by compiling/testing this branch's changes against main's generated type (full v3 suite green, macro payload round-trips), but the branch itself needs a main update before the typed field compiles standalone.
2. Lint pairing: CreativeResult added to HAND_WRITTEN_SCHEMA_SPECS in both modules' generate.py with the sync-creatives-response.json#/oneOf/0/... pointer, removed from EXEMPT in both lint.py files, and a new ALLOWED_EXTRA_FIELDS table in lint.py documents rejection_reason as the one deliberate extra (deprecated, removal is breaking) instead of drift. (lint.py skips locally without the downloaded bundle; its unit tests show the same bundle-dependent baseline before and after this change.)
3. Tests: both fixtures now use approved (a real CreativeStatus) instead of active for the item status. The action: \"failed\" item with a structured error I have not added: with Errors []string it cannot be expressed without encoding the schema-invalid shape — that test belongs with Follow-up: CreativeResult.Errors []string vs []AdcpError drift (breaking re-type deferred from #545) #546's re-type to []AdcpError, which was previously deferred on this PR as a breaking change. The stale 'pins 3.2.0-rc.3' comment is dropped.
4. Deprecation comment: reworded in both modules to your suggested text ('Deprecated: not part of the AdCP schema. Failure detail belongs in errors[] as core/error.json objects; see Follow-up: CreativeResult.Errors []string vs []AdcpError drift (breaking re-type deferred from #545) #546.'), and noted there that Warnings []string is already correct.

Not touched, as you flagged them: the assignment_errors non-string-values decode gap and CreativeInput carrying no revision_id/localization — happy to open follow-ups for both if you want them tracked.

[aao-secretariat]

(non-blocking) Comment contradicts the code directly below it. It says localization and macro_resolution_results "are intentionally excluded here," but this same revision adds both as fields — Localization any and MacroResolutionResults []MacroResolutionResult. What's actually true is their object graphs aren't hand-expanded (localization is any, macro uses the generated type). Reword to say the fields are present but kept shallow, not excluded.

[aao-secretariat]

Ladon verdict: Approve

Approve — schema↔generated-types coherence and wire-shape fidelity verified; no blocking findings.

What I checked

• Pairs CreativeResult to creative/sync-creatives-response.json#/oneOf/0/properties/creatives/items in both the root and v3 modules, removes it from EXEMPT, and adds a scoped ALLOWED_EXTRA_FIELDS allowance for the deprecated rejection_reason.
• Struct changes are additive (all omitempty, no wire break) — no breaking-change / conventional-commit marker required.
• CI Build & Test (1.27) and Lint pass at HEAD, confirming the schema-pairing lint matches and python3 adcp/schemas/lint.py --strict would not surface drift.
• Round-trip tests added; localization round-trips whole-document in the test.
• high_risk: true but reasons are (modified) on adcp/schemas/** and adcp/types.go with no medium-or-higher finding — presumed safe per the high-risk reading rules.

Decision path: No critical/high findings (row 1 n/a); gated_paths: false (row 2 n/a); no (deleted) reasons (row 3 n/a); no medium findings (rows 4/5/8 n/a); prior decision was approve (row 6 n/a); no no-auto-approve team match (row 7 n/a). Falls through to row 9 → approve.

Medium findings: none.

Nits (non-blocking):

• adcp/v3/types.go:1269 — v3 CreativeResult doc comment claims localization/macro_resolution_results are "intentionally excluded" while the same revision adds both fields. Worth a one-line doc fix.

[torbenbrodt]

Thanks, @sujanchalla0510. I checked b9829f3 merged onto current main: both modules regenerate with no diff and pass lint.py --strict, the generator tests, go vet and go test. The CreativeResult pairing is live in both modules, and ALLOWED_EXTRA_FIELDS only exempts CreativeResult.rejection_reason: emptying it makes lint report rejection_reason, and deleting MacroResolutionResults makes it report macro_resolution_results. The new fixtures validate against their pinned schemas, and a localized item with an es-MX target and a macro result now round-trips unchanged.

Two small things left:

• +1 to Ladon: the comment above CreativeResult in adcp/v3/types.go still says localization and macro_resolution_results "are intentionally excluded".
• Follow-up: CreativeResult.Errors []string vs []AdcpError drift (breaking re-type deferred from #545) #546's point 3 ("Consider aligning Warnings []string at the same time if the schema shapes it the same way") can be closed there: the schema types warnings items as string, so the re-type shouldn't touch it.

Agreed, the failed-item structured-error test belongs with #546. On the follow-ups: yes please to an issue for CreativeInput missing revision_id and localization. That change is additive, and without it a seller on the typed SyncCreatives hook can't echo either field. For assignment_errors, I'd add it to #546 rather than open a new issue: any fix changes a public field's type, so it belongs in the same breaking-change decision.

[aao-secretariat]

Ladon verdict: Approve

Approve.

This pass is a delta of one file / 16 lines: a doc-comment correction in adcp/v3/types.go. The prior comment claimed localization and macro_resolution_results were "intentionally excluded," but the struct now carries both (Localization as any, MacroResolutionResults as the generated MacroResolutionResult type). The stale comment was updated to match the struct — no behavior or wire-shape change.

Checks:

• No critical/high findings (schema↔generated-type coherence, TMP signing, TEE boundary, protocol skills, breaking-wire-change markers all unaffected — this is comment-only).
• No medium findings.
• high_risk is true (modified files under adcp/schemas/** and adcp/types.go), but the reviewer surfaced no medium-or-higher concerns on those modifications, so the flag alone does not escalate (no deletions, no medium on modified files).
• gated_paths is false; no no-auto-approve team match.
• Prior decision was approve with no findings; this remains clean.

Decision table: rows 1–8 do not fire → row 9 approve.

[sujanchalla0510]

Thanks @torbenbrodt — the two small things are done:

• Stale comment fixed in 8aa637d: the CreativeResult doc comment in adcp/v3/types.go no longer says localization/macro_resolution_results are "intentionally excluded" — it now says both fields are present but kept shallow (localization as any, macro_resolution_results via the generated MacroResolutionResult type). (+1 to Ladon's nit — same line.)
• Follow-up: CreativeResult.Errors []string vs []AdcpError drift (breaking re-type deferred from #545) #546 updated: point 3 closed there — the schema types warnings items as string, so Warnings []string stays as-is and the re-type covers Errors only. The assignment_errors decode gap is folded into Follow-up: CreativeResult.Errors []string vs []AdcpError drift (breaking re-type deferred from #545) #546 as well, since any fix changes a public field's type and belongs in the same breaking-change decision.
• CreativeInput follow-up opened as CreativeInput carries no revision_id or localization (follow-up to #545) #567: add revision_id + localization to CreativeInput in both modules (additive), so the typed SyncCreatives hook can echo both fields.

[torbenbrodt]

Approving the code. Merged onto today's main (188cdcf), CI's core checks pass locally: generator tests, regeneration with no diff, lint.py --strict and the any-coverage gate in both modules, and go vet with go test -race for root, adcp, adcp/v3 and the reference agents. #546 tracks the Errors and assignment_errors re-type.

Not tracked yet:

• The description still says adcp/v3 pins 3.2.0-rc.3 (main pins 3.2.2), that v3 excludes localization and macro_resolution_results, and that it covers 13 of 15 fields. Please refresh it, since the squash body is built from it.
• The main round-trip tests check that each wire key is present after re-encoding, not that its value survives.
• ALLOWED_EXTRA_FIELDS has no check for stale entries, so rejection_reason stays allowed after the field is removed.
• #567 should cover adcp/v3 only: the adcp module's 3.1.9 schemas have neither revision_id nor localization.

[torbenbrodt]

@bokelley, one question before this merges. My approval covers the code, not the decisions #365's triage left to you:

• deprecating RejectionReason in place, which this PR does, rather than a semver bump;
• the doc-comment contract for account, assigned_to, platform_id and preview_url, which the triage's security reviewer asked a human to confirm before they were added.

Are you happy with both as they stand here?

[sujanchalla0510]

Thanks @torbenbrodt — approving review actioned, including the 'not tracked yet' list:

• Description refreshed (this PR): it no longer says v3 excludes localization / macro_resolution_results or covers 13 of 15 fields — v3 now covers all 15 spec properties of the bundle this branch pins, with the pin noted as 3.2.0-rc.3 (and main's move to 3.2.2 called out). The squash body should now match the head.
• CreativeInput carries no revision_id or localization (follow-up to #545) #567 corrected to v3 only — you're right, the 3.1.9 schemas carry neither revision_id nor localization, so the v1 CreativeInput stays as-is.
• The other two are now tracked on Follow-up: CreativeResult.Errors []string vs []AdcpError drift (breaking re-type deferred from #545) #546: round-trip tests checking value survival (not just key presence), and a stale-entry check for ALLOWED_EXTRA_FIELDS so the rejection_reason allowance can't linger after the field is removed.

Your two questions for @bokelley (deprecating RejectionReason in place vs a semver bump, and the doc-comment contract for account / assigned_to / platform_id / preview_url) are his calls — leaving them with him.

[bokelley]

@torbenbrodt @sujanchalla0510 — maintainer decisions, posted by Brian's coding agent at his direction:

• Deprecate RejectionReason in place. Keep the exported field for compatibility; its eventual removal belongs in a breaking release. Keep the Errors and assignment_errors type corrections in Follow-up: CreativeResult.Errors []string vs []AdcpError drift (breaking re-type deferred from #545) #546, and leave Warnings []string as it is.
• The additive account, assigned_to, platform_id, and preview_url fields are approved, subject to adding the requested field-level documentation before merge. The current head has the fields but lacks those comments. Describe account as the owning account, assigned_to as successfully assigned package IDs, and platform_id as the seller's creative identifier. These are account/resource metadata, not containers for end-user identifiers. Document preview_url as a preview link that may carry sensitive access tokens and should be handled accordingly; do not promise that URLs are token-free. Callers must be authorized for the account and resources returned.

The decisions above resolve the maintainer questions; adding the missing comments is the remaining action on this PR. This does not approve the breaking changes tracked by #546.

[bokelley]

@torbenbrodt — Brian has explicitly authorized implementing the SDK delegation. The guidance is now proposed in #582, in AGENTS.md with a contributor-facing README link.

It is prospective: a complete PR notice starts a fresh five-calendar-day window, states the proposed answer and UTC deadline, and records the head SHA. A hold pauses the clock until explicitly released; material changes restart it. The author records the question and outcome in a tracked issue before merging. All six constitutional escalation categories, required reviews/checks, and blocking findings remain in force. It takes effect when #582 is merged, and does not clear the explicit security hold on #554.

Posted by Brian's coding agent at his direction.

[sujanchalla0510]

Thanks @bokelley — both decisions actioned in 7a58d26 (comment-only):

• Field documentation added in both modules (adcp/types.go, adcp/v3/types.go): account as the owning account; assigned_to as successfully assigned package IDs; platform_id as the seller's creative identifier; the three documented as account/resource metadata, not containers for end-user identifiers; preview_url as a preview link that may carry sensitive access tokens and should be handled accordingly (no token-free promise); callers must be authorized for the account and resources returned.
• RejectionReason: stays deprecated in place, as decided — no change needed; the Errors/assignment_errors type corrections remain in Follow-up: CreativeResult.Errors []string vs []AdcpError drift (breaking re-type deferred from #545) #546 and Warnings []string is unchanged.

Local: adcp module tests pass; adcp/v3 suite passes against main's generated MacroResolutionResult overlay (the branch predates #555 standalone, same as before — CI builds the merge commit). CI re-running on the new head.

[aao-secretariat]

Ladon verdict: Approve

Approve — documentation-only delta since prior approve (8aa637d), no new findings.

Checked:

• Delta since prior approve is doc-comment-only on CreativeResult in adcp/types.go and adcp/v3/types.go. Comments verified accurate (account/resource metadata, PlatformID = seller creative id, PreviewURL token caveat, AssignedTo = package IDs).
• No semantic, wire-shape, type, or schema/lint-pairing changes in the delta — schema↔generated-type coherence preserved.
• No breaking exported-symbol changes, so no conventional-commit marker required.
• high_risk is true only via (modified) entries, but the reviewer surfaced no medium-or-higher concerns, so modifications are presumed safe per the high-risk reasoning rules.

Decision table: no critical/high findings (rows 1 skip), gated_paths false (row 2 skip), no deletions (row 3 skip), no medium findings (rows 4/5/8 skip), prior decision was approve (row 6 skip), no no-auto-approve team match (row 7 skip). Falls through to row 9 → approve.

[torbenbrodt]

Re-approving at 7a58d26: the commit adds only the field documentation Brian asked for (no field or tag changes), so his decisions on this PR are met.