ci(adcp-sdk): pin @adcp/sdk to 7.9.0 (last known-green), salt cache key (closes #779 Track B)#780
ci(adcp-sdk): pin @adcp/sdk to 7.9.0 (last known-green), salt cache key (closes #779 Track B)#780bokelley wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Diff is mechanically right — pin + salted cache + comment refresh land exactly where they need to. Holding on approve because the PR's own validation premise isn't satisfied yet.
The unchecked checkbox is the load-bearing one
Test plan item — currently unchecked, primary user-facing claim of the PR:
- CI green on this PR (proves the pin works against 7.10.1)
CI on this PR is not green. Three storyboard runs are red against @adcp/sdk@7.10.1:
AdCP storyboard runner — examples/seller_agent.py— FAILUREAdCP storyboard runner — v3 reference seller (translator)— FAILUREAdCP storyboard runner — sales-proposal-mode (proposal_finalize)— FAILURE
Failure shape is the same across all three (sample from examples/seller_agent.py):
v1-canonical-mapping.json not found. Looked in:
.../@adcp/sdk/schemas/cache/3.1.0-beta.2/registries/v1-canonical-mapping.json
.../@adcp/sdk/schemas/cache/3.1.0-beta.1/registries/v1-canonical-mapping.json
.../@adcp/sdk/schemas/cache/3.1.0-beta.0/registries/v1-canonical-mapping.json
.../@adcp/sdk/schemas/cache/latest/registries/v1-canonical-mapping.json
Run `npm run sync-schemas` for a 3.1+ AdCP version.
7.10.1 ships without a populated schemas/cache/ for the protocol version it points at — at least under npm install -g. PR body claims local repro on c23c9d52 against @adcp/sdk@7.10.1 returns 57/57 passing; CI here disagrees on a00a99ba. That delta is what I'd want resolved before merging — either the install is missing a post-install schema-sync step that the local repro had implicitly, or 7.10.1 is the wrong target.
Things I checked
- All 4
npm install -g @adcp/sdk@latestsites swapped to@adcp/sdk@${ADCP_SDK_VERSION}(ci.yml:395, 550, 759, 860). No stragglers —grep -rn "@latest" .github/is clean. - All 4 cache keys salted with
${{ env.ADCP_SDK_VERSION }}(ci.yml:383, 534, 753, 854).restore-keys: ${{ runner.os }}-npm-is a proper prefix of the new key, so partial restore from prior caches still works. - Workflow-level
env: ADCP_SDK_VERSIONpropagates correctly to both shellrun:(POSIX${ADCP_SDK_VERSION}) and GHA${{ env.X }}expressions inwith:blocks. - Version
"7.10.1"is shell-safe — unquoted@${ADCP_SDK_VERSION}is fine. - Conventional commit prefix
ci(adcp-sdk):is right; no public-API surface touched, no semver implications. code-reviewer: clean.dx-expert: clean — flagged dependabot/renovate config as a follow-up so the pin doesn't rot.
What would flip this to approve
One of:
- Storyboards go green on this PR after a re-run (caching artifact, schemas/cache somehow not populated on this particular runner) — re-run and see.
- Add a
npm run sync-schemasstep afternpm install -g @adcp/sdk@${ADCP_SDK_VERSION}in the four install sites, or whatever the equivalentadcp sync-schemasinvocation is for the globally-installed CLI. - Bump
ADCP_SDK_VERSIONto a release where the schemas cache ships populated in the npm tarball, and re-confirm CI green.
Whichever path, the PR's claim ("proves the pin works against 7.10.1") needs to actually be observable in this PR's CI before merging — otherwise we trade a known flake for a new known red.
Follow-ups (non-blocking — file as issues)
- Dependabot/Renovate for
@adcp/sdk. Pin-via-env-var trades silent drift for silent staleness. A.github/dependabot.ymlentry scoped to@adcp/sdk(or a RenovateregexManagersrule against theADCP_SDK_VERSION:line) would surface bumps as PRs instead of letting the pin rot — exactly the labelled-change-set DX the new comments promise. (dx-expert flagged.)
Minor nits (non-blocking)
${ADCP_SDK_VERSION}vs${{ env.ADCP_SDK_VERSION }}mix is correct but easy to misread. ci.yml:395 uses POSIX, ci.yml:383 uses GHA — both are right for their context (shell vs expression). A one-line comment near the env declaration would save future-maintainer head-scratching.
Holding on --comment; happy to approve once the storyboards actually go green on this PR.
@adcp/sdk@latest was floating, and the npm cache key
``${{ runner.os }}-npm-adcp-sdk`` was OS-only — so a runner that cached
an older release served stale SDK to every subsequent CI run on that
runner, while a fresh runner pulled the current @latest. Same commit
flipped red/green depending on which runner picked it up (see 2026-05-21
storyboard incident, adcp#4907).
- ADCP_SDK_VERSION env var pinned at workflow header (7.10.1).
- All 4 ``npm install -g @adcp/sdk@latest`` sites use the pin.
- All 4 cache keys salted with ${{ env.ADCP_SDK_VERSION }} so a bump
invalidates deterministically.
- Stale comments rationalizing @latest removed.
Bumping the SDK version now happens via PR — silent-upgrade footgun
closed. Tracks adcp-client-python#779 Track B and adcp#4907 Phase 1.
a00a99b to
8b5380f
Compare
Summary
Pins
@adcp/sdkfrom floating@latestto 7.9.0 in.github/workflows/ci.yml, and salts the npm cache key with the pinned version.Why 7.9.0 and not 7.10.x:
@adcp/sdk@7.10.0was published 2026-05-21T14:29Z. The first storyboard failure on main appeared at 14:42Z (on PR #774's merge). Last known-green CI on main was1c4e57d4at 13:55Z — that ran against 7.9.0. 7.10.x ships new scenarios (measurement_terms_rejected/create_media_buy_aggressive_terms,invalid_transitions/update_unknown_package, etc.) that this repo'sexamples/seller_agent.pydoesn't yet satisfy.This PR fixes the silent-upgrade footgun that let 7.10.0 land on every CI run without anyone noticing. The example-side gap to 7.10.x is tracked separately (filed as a follow-up issue, link below).
Background
The npm cache key was
${{ runner.os }}-npm-adcp-sdk— OS-only, no version salt. With@adcp/sdk@latestfloating, a new SDK release silently coexists with old-cached copies; same commit goes red/green depending on which runner picks it up. Bumping the SDK is now a labelled PR diff instead of an invisible registry-publish event.See:
Changes
env.ADCP_SDK_VERSION = "7.9.0".npm install -g @adcp/sdk@latestsites use@adcp/sdk@${ADCP_SDK_VERSION}.${{ env.ADCP_SDK_VERSION }}.@latest("intentional for drift detection", etc.) replaced with the pin's rationale.What this does NOT fix
@adcp/sdk@7.10.x— filed as a follow-up.Bumping the SDK version going forward
Edit
ADCP_SDK_VERSIONin.github/workflows/ci.yml. The salted cache key auto-invalidates. SDK breakage surfaces in a PR diff, not a flaky main.Closes
@adcp/sdkversion pin + cache hygiene)Refs
Test plan