Skip to content

ci(adcp-sdk): pin @adcp/sdk to 7.9.0 (last known-green), salt cache key (closes #779 Track B)#784

Merged
bokelley merged 3 commits into
mainfrom
claude/issue-779-pin-adcp-sdk
May 22, 2026
Merged

ci(adcp-sdk): pin @adcp/sdk to 7.9.0 (last known-green), salt cache key (closes #779 Track B)#784
bokelley merged 3 commits into
mainfrom
claude/issue-779-pin-adcp-sdk

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Re-opened of #780 (close+reopen failed via API). See #780 for full body. Pinning to 7.9.0; vendoring step from #775 preserved.

TL;DR

Refs

bokelley added 2 commits May 21, 2026 17:28
@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.
@bokelley bokelley enabled auto-merge (squash) May 21, 2026 21:29
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

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

LGTM after the duplicated fixture-vendoring block at the storyboard install step comes out. Pin + salted cache key is the right shape — the floating-@latest cache-poisoning footgun is closed cleanly.

Things I checked

  • Pin mechanism: env: ADCP_SDK_VERSION: "7.9.0" at workflow top (ci.yml:9-12), shell-expanded as @adcp/sdk@${ADCP_SDK_VERSION} in the 4 run: blocks. Workflow env: exports into step shells; value has no whitespace or shell metachars, so unquoted expansion is safe.
  • Cache key salting: all 4 actions/cache@v4 keys end in -${{ env.ADCP_SDK_VERSION }} (~lines 399, 555, 781, 889). restore-keys: ${{ runner.os }}-npm- fallback preserved so the first post-bump run still warms from prior tarballs.
  • The other 3 install sites (~lines 570, 786, 894) are clean — single set of vendoring lines, no duplication. Storyboard job is the only one with the doubled block.
  • code-reviewer Major and dx-expert Major both land on the same ci.yml:413-417 duplicate, with no other correctness flags.

Follow-ups (Major — fix in this PR before merge)

  • ci.yml:413-417: duplicated fixture-vendoring block in the storyboard install step. Five lines (SDK_ROOT=..., two mkdir -p/cp pairs) re-run verbatim after lines 408-412. Idempotent — same source bytes, same dest path — so CI doesn't fail, but it's debris from the #780#784 rebase and the other 3 install sites prove the right shape is a single block. Delete L413-417. This is what would flip the review to --approve.

Minor nits (non-blocking)

  1. Commit cddde254 message says "pin @adcp/sdk to 7.10.1" but the file pins 7.9.0. Stale text from before the rollback; the follow-up commit 6456aee2 is honest about being a 7.9.0 retrigger. A later bisector hunting "when did we pin to 7.10.1" will land on a commit that doesn't. Squash-merge papers over it; otherwise worth amending.
  2. Bump-instructions discoverability (ci.yml:9-12). Header comment is good. One added line — "to bump, edit this value and open a PR; cache invalidates automatically" — would close the loop for the next person who skims the workflow without reading the commit body.

Drop L413-417 and I'll approve.

Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

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

LGTM. Closes a real race — OS-only cache key plus floating @latest meant a warm runner served a stale tarball while a cold runner pulled current, and the same commit flipped red/green depending on which runner picked it up.

Things I checked

  • Workflow-level env: ADCP_SDK_VERSION: "7.9.0" (.github/workflows/ci.yml:12) is exposed to shell context in run: steps — ${ADCP_SDK_VERSION} resolves at lines 406, 568, 784, 892.
  • All four cache-key sites salted: lines 383, 550, 776, 884. restore-keys: ${{ runner.os }}-npm- is safe to keep — it only seeds warm tarballs into ~/.npm; the install step still pins the requested version, so npm fetches the delta if needed.
  • No @adcp/sdk@latest remains (grep is clean). Stale comments rationalizing @latest for drift detection were removed at the install-step sites.
  • Fixture-vendoring block from #775 is preserved exactly once per job — the third commit (bba4e325) dropped the duplicate that landed earlier.

Follow-ups (non-blocking)

  • Drift signal moved, not replaced. The old @latest rationale was "surface protocol drift the moment it ships." Pinning trades that for a labelled bump PR — but the next contributor reading lines 388-394 won't see what replaces the lost auto-drift signal. Worth one sentence pointing at #782 (the 7.10.x bump) or whatever periodic-bump mechanism owns this now.
  • Bump-flow findability. Header comment says "Bump deliberately" but doesn't tell future-you that the env var drives all four install sites and no other edit is needed. One line would save a grep.

Minor nits (non-blocking)

  1. Stale commit header. First commit subject (cddde254) says pin @adcp/sdk to 7.10.1, the file pins 7.9.0. Auto-merge is squash, so the PR title (correct: 7.9.0) wins on main — notable only if anyone reads git log on the unsquashed branch.
  2. Defensive quoting. npm install -g @adcp/sdk@${ADCP_SDK_VERSION} is unquoted at all four sites. Safe for semver values; "@adcp/sdk@${ADCP_SDK_VERSION}" would be slightly more defensive against a future tag with shell-special chars.

Safe to merge.

@bokelley bokelley merged commit e389db1 into main May 22, 2026
23 checks passed
@bokelley bokelley deleted the claude/issue-779-pin-adcp-sdk branch May 22, 2026 08:10
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