Skip to content

fix(training-agent): unique package IDs and assignment tracker-slot limits - #8712

Merged
bokelley merged 2 commits into
mainfrom
bokelley/training-agent-assignment-gaps
Nov 3, 2026
Merged

bokelley merged 2 commits into
mainfrom
bokelley/training-agent-assignment-gaps

Conversation

@bokelley

@bokelley bokelley commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This fixes the two training-agent gaps left open in #8695.

1. Package IDs are unique across buys

create_media_buy and update_media_buy new_packages used to restart at pkg-0 in every buy.

Why that was wrong:

  • package_id is defined as the "Seller's unique identifier for the package" (core/package.json).
  • sync_creatives assignment_operations address a package by package_id alone.
  • So once an account had two open buys, every assignment operation failed as ambiguous.

The fix: packages keep the deterministic pkg-N form but are numbered across the account's buys.

  • The first buy in a fresh account still gets pkg-0, pkg-1, …, and later buys continue the sequence.
  • Existing tests, docs examples, and the published vendor_metric_accountability storyboard, which keys simulate_delivery metric maps on a literal pkg-0, are unaffected.
  • An earlier revision of this PR scoped IDs to the buy (<media_buy_id>-pkg-N). That broke the storyboard, as Ladon pointed out, because storyboard substitution can't rewrite object keys.

2. Assignment trackers respect format tracker-slot limits

Assignment trackers and the creative's own tracker assets now share each format's declared tracker slots:

  • Where slots come from: params.slots on each format option the package accepts for the creative's format kind, or the canonical defaults.
  • Matching: trackers go to slots by event role. impression_tracker takes impression, click_tracker takes click, and viewability_tracker takes the viewable_* and audible_video_complete events. Any other pixel_tracker slot takes any event, and event-specific slots are filled first.

Where it's enforced:

When the capability isn't declared, trackers or landing_page_url on create/update rows now get UNSUPPORTED_FEATURE. They used to be accepted silently.

Spec gap, not fixed here

The spec doesn't say what happens when a format declares no tracker slots. Read literally, "each assignment tracker consumes one occurrence in one compatible declared tracker slot" would reject every assignment tracker on such a format. The canonical image format's tracking model fires trackers through macros and declares no tracker slots, so the training agent treats an undeclared slot list as making no slot claim and accepts.

That's the reading that keeps assignment trackers usable on the common display formats. It should be confirmed in the spec, and I can file an issue for it.

Testing

  • tests/unit/assignment-operations.test.ts: 17 pass. New tests cover:
    • unique package IDs across two buys;
    • a canceled buy's package being closed;
    • pure slot-allocation cases;
    • overflow rejected and fit accepted, using a seeded product with impression_tracker max: 1;
    • a replace whose transferred tracker collides with the replacement's own tracker;
    • create_media_buy rows enforcing the same limit;
    • UNSUPPORTED_FEATURE where the capability is undeclared.
  • training-agent, comply-test-controller, tenant-smoke and live-creative-updates unit tests: 1022 pass, with none of the existing test files changed.
  • Storyboards on /sales in candidate mode, all clean:
    • assignment_replace_transfers_settings, creative_live_updates, creative_live_update_refusal;
    • sales_broadcast_tv, delivery_reporting, property_delivery_reporting, installment_property_delivery_reporting.
    • The vendor-metric storyboards. vendor_metric_accountability is skipped on /sales, which is why CI stayed green before; its single-buy pkg-0 assumption now holds anyway.
    • delivery_reporting's 3 skips are identical on main.
  • No changeset, because this PR changes only the training agent and its tests.

🤖 Generated with Claude Code

…imits

Two gaps left by #8695:

- Package IDs. create_media_buy and update_media_buy new_packages minted
  pkg-N in every buy, but package_id is the seller's unique identifier
  (core/package.json) and sync_creatives addresses a package by
  package_id alone, so two open buys made every assignment operation
  ambiguous. IDs stay deterministic and are scoped to the buy:
  <media_buy_id>-pkg-N.
- Tracker slots. Assignment trackers and the creative's own trackers now
  share each format's declared tracker slots (params.slots or canonical
  defaults), matched by event role (impression, click, viewability;
  other pixel_tracker slots take any event). sync_creatives assign and
  replace (transferred trackers against the replacement's own) and
  create/update_media_buy assignment rows reject an assignment that has
  no valid allocation. A format that declares no tracker slot makes no
  slot claim, so nothing is refused there. Assignment rows on
  create/update that carry trackers or landing_page_url where the
  capability is not declared get UNSUPPORTED_FEATURE.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ladon verdict: Request changes

Request changes — 1 blocking finding.

Blocking findings

  • server/src/training-agent/task-handlers.ts:16898 — Package-ID rename (<media_buy_id>-pkg-N) breaks the vendor_metric_accountability conformance scenario, which still keys vendor-metric injection by literal pkg-0. The two-package lookup misses and the delivery-report validations fail. The PR updated unit fixtures but did not update the scenario source or smoke that scenario.

The tracker-slot allocation (trackerAllocationProblem) and UNSUPPORTED_FEATURE gating were reviewed and are sound. No schema/dist/changeset surface was touched, so no changeset is required. Fix the scenario source to use the new buy-scoped package_id (or restore compatibility) and smoke the vendor_metric_accountability scenario before merge.

Blocking findings

  • server/src/training-agent/task-handlers.ts:16898 — Package-ID rename breaks vendor_metric_accountability conformance scenario

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ladon verdict: Request changes

Request changes — 1 blocking finding.

Blocking findings

  • static/compliance/source/protocols/media-buy/scenarios/vendor_metric_accountability.yaml:518 — Buy-scoping package IDs to ${mediaBuyId}-pkg-N ends the deterministic pkg-0/pkg-1 ids. The compliance scenario hard-codes pkg-0 as the metric-map key (lines 518, 526), and validatePackageMetricMap now rejects it as an unknown package_id, failing the simulate_delivery step. The unit test (training-agent.test.ts:9461) was updated but the scenario asset was not — this is spec drift on a published compliance asset.

The tracker-slot allocation logic (assignment-operations.ts + task-handlers.ts) is sound: greedy specific-slot-first allocation is correct (event-specific slots are disjoint), occupancy counts the creative's own trackers, the replace path validates against the replacement's assets, and the capability gate is enforced on both create and sync paths — all well covered by new tests.

Follow-up (non-blocking): docs examples comply-test-controller.mdx:931 and targeting.mdx:48 still show pkg-0/pkg-1; update for consistency.

Blocking findings

  • static/compliance/source/protocols/media-buy/scenarios/vendor_metric_accountability.yaml:518 — Buy-scoped package IDs break vendor_metric_accountability compliance scenario (simulate_delivery fails because pkg-0 metric-map key is rejected as unknown package_id)

…r buy

Addresses review on #8712. Buy-scoped IDs (<media_buy_id>-pkg-N) broke the
published vendor_metric_accountability storyboard, which keys
simulate_delivery metric maps on the literal pkg-0 (storyboard
substitution covers values, not keys). Keep the pkg-N form and number
packages across the account's buys instead: the first buy in a fresh
account still gets pkg-0, pkg-1, and later buys continue the sequence,
so package_id stays unique for sync_creatives assignment operations.
The earlier test-expectation changes are reverted.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@aao-secretariat
aao-secretariat Bot dismissed stale reviews from themself November 3, 2026 03:43

Superseded by a later Ladon review that rechecked and resolved its blocking findings.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ladon verdict: Approve

Approve — prior blocking finding resolved, no new findings.

The prior run flagged a [high] issue: buy-scoping package IDs to ${mediaBuyId}-pkg-N broke the vendor_metric_accountability compliance scenario, which hard-codes pkg-0 (yaml:518,526) and thus failed simulate_delivery.

This pass reverts to account-scoped numbering (nextPackageIndex → pkg-${firstPackageIndex + index}, task-handlers.ts:16895): a fresh session's first buy gets pkg-0/pkg-1, so the single-buy scenario's pkg-0 key again names package 0. Only vendor_metric_accountability.yaml hard-codes a package_id across compliance sources, and it now matches. Numbering logic is sound on both paths — create uses nextPackageIndex(session); update takes max(session, working-copy mb) to cover uncommitted packages, preserving the "a package_id names one package" invariant.

No schema/dist/docs surface touched (only server/** training-agent code + tests), so no changeset required. Delta test fixtures updated consistently.

No current blocking or medium findings. gated_paths is false; high_risk is false; no team gate. Prior decision was request-changes (not escalate), so sticky-escalation row 6 does not apply. Falls through to row 9.

@bokelley
bokelley merged commit 4677105 into main Nov 3, 2026
93 of 96 checks passed
@bokelley
bokelley deleted the bokelley/training-agent-assignment-gaps branch November 3, 2026 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant