Skip to content

feat(compliance): prove metric container subsumption end to end - #6887

Open
bokelley wants to merge 5 commits into
mainfrom
metric-container-subsumption
Open

feat(compliance): prove metric container subsumption end to end#6887
bokelley wants to merge 5 commits into
mainfrom
metric-container-subsumption

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

Implements #6785 (deferred at triage as blocked on #6618; unblocked since it merged). The container-subsumption rule in available-metric.json was prose-only — nothing pinned independent implementations to the same evaluation. Two halves close it:

Reference seller (server/src/training-agent/): one shared helper (metric-subsumption.ts, 14 unit tests) now backs both surfaces. Notably, required_metrics filtering on get_products did not exist at all for standard metrics (only required_vendor_metrics was implemented — the documented filter was silently ignored); it now evaluates container-subsumes-leaf semantics. get_media_buy_delivery implements requested_metrics narrowing (leaf resolves to its carrier, impressions/spend always included) across totals, package rows, and creative rows, with viewability mirrored to package grain.

Conformance storyboard (metric_container_subsumption, 10 graded validations): the four exercises from #6785, each anchored to normative schema text — container satisfies leaf requirement; mixed container/leaf declarations don't produce empty intersections; a leaf request returns the full carrier object (viewability.viewable_rate AND .measurable_impressions present, clicks absent, impressions/spend always present); and a leaf declaration does not satisfy a sibling leaf (filter-not-fail exclusion).

Verification

  • Storyboard in isolated execution: 10P / 0F / 0S.
  • Full local storyboard matrix: all seven tenants green; /sales at 126 clean / 559 steps (floor 553) — the new storyboard adds its steps on top of the floor.
  • Helper unit tests 14/14; typecheck clean; full schema suite green.
  • Patch changeset (additive conformance scenario).

Includes the floor re-baseline commit from #6885 (merge-order safe: whichever lands first, the other dedupes).

Closes #6785.

🤖 Generated with Claude Code

bokelley and others added 3 commits August 25, 2026 12:38
available-metric.json's container-subsumption rule was prose-only —
nothing pinned independent implementations to the same evaluation, the
divergence hazard tracked in #6785. Two halves close it:

The reference seller now implements the rule through one shared helper
(metric-subsumption.ts): required_metrics filtering on get_products
existed for vendor metrics only — the standard-metric filter was
silently ignored — and now evaluates container-subsumes-leaf semantics;
get_media_buy_delivery implements requested_metrics narrowing (leaf
resolves to its carrier, impressions and spend always included) across
totals, package rows, and creative rows, with viewability mirrored to
package grain.

The metric_container_subsumption storyboard grades all four exercises
from #6785 with validations anchored to the normative schema text:
container satisfies leaf requirement, no empty mixed intersection,
leaf request returns the full carrier object, and a leaf declaration
does not satisfy a sibling leaf (filter-not-fail).

Closes #6785.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The #6815 coverage rebaseline set current-source floors above what main
actually measures (creative 48/50 clean with 200 steps vs 49/209 floor;
creative-builder 48 vs 50, 172 vs 184; sales 553 steps vs 556) — every
gap is authored known-failing skips citing packaged-runner blockers,
with zero step failures. The workflow has been red on main since the
merge and the mirrored local pre-push gate blocks every compliance-
source push. Set floors to the values measured on main's own CI run so
the ratchet grades regressions from reality again. Closes #6876.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
// Sellers may report per-package viewability; the training agent's
// simulate_delivery viewability param is buy-scoped (mirrors totals),
// so it applies to every package uniformly.
...(simDelivery?.viewability ? { viewability: simDelivery.viewability } : {}),

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.

(non-blocking) Duplicate spread. packageDeliveryMetrics already spreads ...(simDelivery?.viewability ? { viewability: simDelivery.viewability } : {}) three lines up (L12958), before ...byCreative. This second spread of the identical key/value is a no-op — nothing between them mutates viewability. The comment describes real intent (buy-scoped viewability applied uniformly per package), but the first spread already achieves it. Drop the added line and, if the rationale is worth keeping, move the comment to L12958.

@aao-secretariat aao-secretariat Bot added the ladon/needs-human-review Ladon has escalated this PR for human review. label Aug 25, 2026

@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: Escalate to human review

Escalate — gated path requires human/CODEOWNERS review.

This PR modifies .github/workflows/training-agent-storyboards.yml, which matches the deterministic gated-path glob .github/workflows/**. gated_paths is true and review_decision is REVIEW_REQUIRED (not APPROVED), so decision-table row 2 fires: the hard approval gate is not satisfied and the outcome must be escalate.

There are no blocking (critical/high) findings — the wire-shape logic is sound, the metric-narrowing is schema-safe, and changesets are present and correctly scoped. The only reviewer finding is low (a no-op duplicate viewability spread in task-handlers.ts:12964), which does not block.

Escalation reasons

  • .github/workflows/training-agent-storyboards.yml (modified) is under a hard approval gate (.github/workflows/**); requires human/CODEOWNERS review (review_decision is currently REVIEW_REQUIRED, not APPROVED).

Once a human/CODEOWNERS approval lands (review_decision: APPROVED), this PR falls through to a clean approve on its findings.

Why human review

  • .github/workflows/training-agent-storyboards.yml (modified) is under the hard approval gate .github/workflows/**; review_decision is REVIEW_REQUIRED, not APPROVED — human/CODEOWNERS review required.
  • This PR touches a path under a hard, non-overridable approval gate (.github/workflows/training-agent-storyboards.yml (modified) matches .github/workflows/**) and the current GitHub review decision is 'REVIEW_REQUIRED', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.

bokelley and others added 2 commits August 26, 2026 06:05
run-storyboards-sharding.test.cjs pins the (tenant, min_clean,
min_passed) tuples across the workflow and the local matrix runner;
update its baseline table to the re-measured floors.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ladon/needs-human-review Ladon has escalated this PR for human review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Conformance storyboard: container-subsumption evaluation for metric set operations

1 participant