ci: check out percolator-sdk as a sibling so pnpm install can resolve it (#232) - #233
ci: check out percolator-sdk as a sibling so pnpm install can resolve it (#232)#233dcccrypto wants to merge 2 commits into
Conversation
… it (#232) package.json/pnpm-lock.yaml resolve @percolatorct/sdk as `file:../../percolator-sdk`. CI checks out only this repo, so the path does not exist and `pnpm install --frozen-lockfile` aborts with ENOENT: no such file or directory, scandir '/home/runner/work/percolator-sdk' exit 254 No api PR has had a green build-and-test since ~2026-06-26, so ~20 open PRs are unverified. Same root cause and same remedy as percolator-indexer#172, except the path is two levels up rather than one. Checks the SDK out into the workspace and moves it to ../../percolator-sdk, since actions/checkout cannot write outside GITHUB_WORKSPACE. Verified locally against origin/main (b2751f4) in a scratch tree shaped like the runner (work/percolator-api/percolator-api + work/percolator-sdk): before: pnpm install --frozen-lockfile -> ENOENT, exit 254 after: pnpm install --frozen-lockfile -> exit 0 pnpm build -> exit 0 pnpm test -> 293 passed, 2 failed The 2 remaining failures are pre-existing and unrelated to this change: the SDK ships a committed dist/ that is stale against its own src/ (src sets V17_PROGRAMS_DEPLOYED = true, dist/index.js still has false), so getMatcherProgramId("devnet") throws in tests/sdk-smoke.test.ts. Tracked separately for the SDK repo. This change is what makes those tests run at all. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe CI workflow checks out the sibling SDK required by the lockfile and verifies its build output. SDK smoke tests conditionally validate v17 program ID behavior based on deployment status. ChangesSDK CI compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CI confirms the unblockThe first run on this branch reproduced the local verification exactly:
This is the first time The 2 failures are the pre-existing stale-SDK- Both resolve through So this PR will stay red until sdk#355 lands. It is still worth merging: the alternative is zero CI signal on ~20 PRs, versus 293 passing tests plus 2 accurately-reported real defects. The |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
34-38: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd
persist-credentials: falseto the secondary checkout.Since this checkout is only used to retrieve the SDK for local dependency installation and does not require pushing changes, you should disable credential persistence to improve the security posture and prevent the GitHub token from remaining in the local
.git/configof the cloned repository.🛡️ Proposed fix to disable credential persistence
- uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 with: repository: dcccrypto/percolator-sdk ref: main path: _percolator-sdk + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 34 - 38, Update the secondary checkout step using actions/checkout in the workflow to set persist-credentials to false alongside its existing repository, ref, and path options; leave the primary checkout and other workflow behavior unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 34-38: Update the secondary checkout step using actions/checkout
in the workflow to set persist-credentials to false alongside its existing
repository, ref, and path options; leave the primary checkout and other workflow
behavior unchanged.
…gram IDs
The sibling-checkout fix unblocks `pnpm install`, but `pnpm test` still failed
2/295: tests/sdk-smoke.test.ts asserted that getProgramId("devnet") and
getMatcherProgramId("devnet") return a PublicKey. The v17 SDK deliberately
removed that behaviour — both fail closed while V17_PROGRAMS_DEPLOYED === false
so a v17 encoder can never be pointed at a legacy program that cannot decode
v17 instruction payloads (Phase 7 cutover gate).
Assert whichever half of the documented contract is live, keyed on the SDK's
exported V17_PROGRAMS_DEPLOYED flag, so the test stays honest after cutover
rather than needing another edit.
Verified in a CI-identical layout (sdk checked out as a sibling two levels up):
pnpm install --frozen-lockfile / pnpm build / pnpm test -> 25 files, 295 passed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Heads-up from infra: I opened #238, which fixes #232 at the dependency level instead of the workflow level. The two are complementary and touch disjoint files, so they will not conflict. Your diagnosis in #232 was right, but the
#238 pins What I'd suggest:
Merging this first, then #238, should get |
Note for reviewers: this PR is green for a reason worth knowing
percolator-sdk's committed So this approach passes because it builds the SDK from source, which is not what Not arguing against merging this — it unblocks ~20 stacked api PRs and the CI change is sound. |
…gram ID Pinning @percolatorct/sdk to a git tarball (this PR) fixed `pnpm install`, but surfaced two real test failures: the v17 SDK fails closed and *throws* from getProgramId()/getMatcherProgramId() while V17_PROGRAMS_DEPLOYED === false, rather than handing back a legacy address that cannot decode v17 instruction payloads. Assert whichever half of that contract is live so the smoke test stays honest across the Phase 7 cutover. Ported from #233, which fixed the same two tests behind a CI sibling-checkout workaround that the tarball pin makes unnecessary. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Heads-up: I believe this is superseded by #238, which is now fully green. #238 fixes the root cause in Your Leaving this open for you to confirm rather than closing it myself. If you agree, closing in favour of #238 is the cleaner path. |
|
Some evidence for whoever picks this up, because the api PR queue currently looks I checked every open PR in this repo (27) rather than just mine:
Whatever was wrong with the Vercel project was fixed between 21 July and today.
So that one is not stale — Net: merging this turns I have not pushed empty commits to force re-runs — that is churn on other |
1 participant
Fixes #232.
What
Adds a sibling checkout of
dcccrypto/percolator-sdkahead ofpnpm installin thebuild-and-testjob.Why
package.jsonandpnpm-lock.yamlonmainresolve@percolatorct/sdkasfile:../../percolator-sdk. CI checks out only this repo, so that path does not exist and the install aborts before a single test runs:No api PR has had a green
build-and-testsince ~2026-06-26, so ~20 open PRs are currently unverified.Same root cause and same remedy as dcccrypto/percolator-indexer#172, which is merged and has
maingreen. One difference: the api path is two levels up (../../percolator-sdk), not one — fromwork/percolator-api/percolator-apithat resolves towork/percolator-sdk, which matches the ENOENT exactly.actions/checkoutcannot write outsideGITHUB_WORKSPACE, so the SDK is checked out into the workspace and then moved up.This needs no SDK publish and no lockfile regeneration — SDK
origin/main(v3.0.0,673bc47) ships a committeddist/.How to test
Verified locally against
origin/main(b2751f4) in a scratch tree shaped like the runner (work/percolator-api/percolator-api+work/percolator-sdk):pnpm install --frozen-lockfilepnpm buildpnpm testKnown: this does not get CI fully green
Two
tests/sdk-smoke.test.tsfailures remain. They are pre-existing and unrelated to this change — this PR is what makes them run at all.The SDK ships a committed
dist/that is stale against its ownsrc/. Commit673bc47is titled "flip V17_PROGRAMS_DEPLOYED to true", and it does so in source, but the shipped bundle was never rebuilt:src/config/program-ids.ts:65→export const V17_PROGRAMS_DEPLOYED = true;dist/index.js:2186→var V17_PROGRAMS_DEPLOYED = false;Consumers resolve
dist/, sogetMatcherProgramId("devnet")throws "v17 matcher program is not deployed for devnet". That is an SDK-repo fix (rebuild and commitdist/), reported separately to the sdk owner.Merging this is still a strict improvement: it converts CI from permanently red at install with zero signal into 293 tests of real signal plus 2 correctly-reported defects.
Follow-up (not in this PR)
The
dockerjob has the same latent bug — the Dockerfile runspnpm install --frozen-lockfilewith onlypackage.json/pnpm-lock.yamlin the build context, so it will hit the same ENOENT. It is currently unreachable (needs: build-and-test,if: main), so this PR makes it reachable and it is expected to fail onmainuntil fixed. I did not include a fix here because the Docker daemon was unavailable locally and I will not ship an unverified build change. Filed as a follow-up.🤖 Generated with Claude Code
Summary by CodeRabbit