Skip to content

fix(conformance): treat absent proposal support as unsupported#2248

Merged
bokelley merged 1 commit into
backport-2114-adcp-3.0from
backport-2245-7x
Jun 17, 2026
Merged

fix(conformance): treat absent proposal support as unsupported#2248
bokelley merged 1 commit into
backport-2114-adcp-3.0from
backport-2245-7x

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

  • Backport fix(conformance): treat absent proposal support as unsupported #2247 for the 7.11 maintenance branch.
  • Treat omitted media_buy.supports_proposals as unsupported for proposal-lifecycle requires_capability gates.
  • Add 7.11-compatible regression coverage for omitted support, unavailable raw capabilities, and true/false direct predicate behavior.

Fixes #2245 for 7.x / 7.11.x.

Validation

  • npm ci in the backport worktree
  • npm run build:lib
  • NODE_ENV=test node --test-timeout=60000 --test-force-exit --test test/lib/storyboard-capability-gate.test.js
  • npx prettier --check src/lib/testing/storyboard/runner.ts test/lib/storyboard-capability-gate.test.js
  • git diff --check
  • pre-push validation: npm run typecheck + build hook

@bokelley bokelley marked this pull request as ready for review June 17, 2026 14:37

@aao-ipr-bot aao-ipr-bot 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.

LGTM as a 7.x backport. Spec text in the 3.1-beta schema (tools.generated.ts:8575: "When false or absent, the seller serves products directly without proposal abstraction; conformance runners skip proposal-lifecycle storyboards.") makes this a witness move, not a translator move — absence has a declared meaning for this specific field and the runner is honoring it.

Things I checked

  • Changeset present, patch is the right type — no public API change, runner-internal behavior only.
  • isProposalLifecycleGate narrows cleanly and is used at both call sites (runner.ts:289 in the predicate, runner.ts:1622 in the executeStoryboardPass branch).
  • Tests cover the two paths the runtime branch handles: raw_capabilities: { media_buy: {} } with discovery tool present, and missing raw_capabilities with discovery tool absent. The direct evaluateCapabilityPredicate tests cover absent/true/false.
  • No fabrication. No custom transport. No published-API change. Doc links unaffected.

Follow-ups (non-blocking — file as issues)

  • Probe-failure gap. runner.ts:1621-1634 fires only when get_adcp_capabilities is absent from profile.tools. When the tool IS advertised but the discovery call throws — client.ts:377-381 sets profile.capabilities_probe_error and leaves raw_capabilities undefined — the new branch falls through and the proposal storyboard runs, producing exactly the misleading cascade this PR aims to prevent. Consider gating on rawCaps === undefined regardless, or OR-in profile.capabilities_probe_error !== undefined. Add a test for that path so it doesn't regress.
  • Wrong shape long-term. isProposalLifecycleGate hardcoding the path string media_buy.supports_proposals is fine as a one-off but the same "absence = unsupported" semantics already apply to buying_modes (defaults to ['brief'] when absent), reporting_delivery_methods (polling-only when absent), and others. The right primitive is a storyboard-side declaration on the gate itself (extending #1811's present: matcher, or requires_capability: { ..., declared_true: true }) so the storyboard author opts in, not the runner. Track for main; the backport stays as-is.
  • Changeset prose. "skip proposal storyboards when supports_proposals is absent" understates the behavior change for adopters who were relying on the old run-and-fail mode to flush under-declaration. Worth a one-liner noting graded outcomes will shift for under-declared sellers.

Approving on the strength of the spec citation plus the test coverage matching the two runtime branches the PR actually adds.

@aao-ipr-bot aao-ipr-bot 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.

Clean backport of #2247. The narrow special-case is the right shape: an optional opt-in feature should fail-closed — absent media_buy.supports_proposals means "didn't opt in," so skip the proposal-lifecycle storyboard rather than run it and cascade misleading failures.

This does not break the general absence-semantics rule. The default "run when undeclared" behavior at runner.ts:286 is preserved for every other capability; the exception fires only when isProposalLifecycleGate narrows path === 'media_buy.supports_proposals' && equals === true. Sits after the present:/contains: early-returns, so no cross-contamination with those matchers.

code-reviewer: sound. Not a witness/translator violation — this is a test-applicability skip decision, not a fabricated capability value written back into the profile or wire response. Nothing synthetic crosses a seam.

Things I checked

  • Dual-path logic. Path 1 (runner.ts:1597, rawCaps defined): resolveCapabilityPath returns undefinedevaluateCapabilityPredicate skips via the new branch. Path 2 (runner.ts:1609 else if, rawCaps undefined + no get_adcp_capabilities): skips. Both terminate in buildCapabilityUnsupportedResult. Test 1 exercises Path 1, Test 2 exercises Path 2.
  • null-on-wire and primitive-intermediate cases. Both still skip — via the existing actual !== undefined && actual !== predicate.equals branch — so the outcome is consistent regardless of which branch catches them.
  • profile === undefined short-circuits Path 2 safely (storyboard runs — reasonable default when there's no profile at all).
  • Changeset present, patch bump. Correct: internal-only refinement of conformance-runner skip behavior, no exported-symbol or response-shape change. Changeset-vs-wire-impact audit passes.
  • Test assertions match the actual skip-detail strings (media_buy.supports_proposals / did not declare / not satisfied); overall_passed, skipped_count, skip_reason, and the DETAILED_SKIP_TO_CANONICAL mapping are all exercised.

Follow-ups (non-blocking — file as issues)

  • Scenario C asymmetry on probe failure. The Path 2 guard !profile.tools.includes('get_adcp_capabilities') (runner.ts:1609) only catches "agent doesn't advertise the tool." If the agent advertises get_adcp_capabilities but the probe threw or returned nothing, rawCaps is undefined AND the tool is in profile.tools — neither branch fires, and the proposal storyboard runs. That's a defensible choice ("support unknown because probe failed" ≠ "support absent"), but it's undocumented. A one-line comment on the else if would stop a future reader filing it as a bug.

Minor nits (non-blocking)

  1. Stale doc. src/lib/testing/storyboard/types.ts:132-133 still says "When raw_capabilities is not available... the gate is a no-op and the storyboard runs." Now false for the proposal gate. Mirror the runner.ts:286 exception note here.
  2. Hardcoded === true in the detail string. The new message at evaluateCapabilityPredicate templates === true literally rather than interpolating predicate.equals like its sibling branch. Correct because isProposalLifecycleGate narrows equals to true — not fabricated, just stylistically inconsistent with the ${JSON.stringify(predicate.equals)} neighbor.

Approving. Follow-ups noted above.

@bokelley bokelley merged commit 2b2086c into backport-2114-adcp-3.0 Jun 17, 2026
8 checks passed
@bokelley bokelley deleted the backport-2245-7x branch June 17, 2026 14:42
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.

1 participant