YNU-869: Guard TS SDK drift and CI validation#701
Merged
Conversation
ihsraham
requested review from
dimast-x,
nksazonov and
philanton
as code owners
Contributor
📝 WalkthroughWalkthroughRemoved the deprecated channels.v1.get_states RPC surface across API, Go, and TypeScript; updated CI to run separate TS SDK and TS-compat validation jobs (including typecheck/lint/build); added ts/ts-compat build scripts; and introduced a Jest drift-guard test ensuring RPC method parity between Go and TS. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/main-pr.yml:
- Around line 26-31: The test-sdk-compat job runs before the TS SDK build
completes causing typecheck to fail; update the workflow so test-sdk-compat
depends on the TS SDK build by adding needs: test-sdk-ts to the test-sdk-compat
job (or alternatively merge the two jobs into one sequential job), ensuring the
job name test-sdk-compat has a needs reference to test-sdk-ts so sdk/ts/dist/
exists before the compat job runs.
In @.github/workflows/test-sdk.yml:
- Around line 39-49: The workflow runs tests twice for sdk/ts because
package.json's "build" executes "npm run test && tsc"; remove or conditionalize
the standalone "Run tests" step (the step named "Run tests" that runs "npm
test") so sdk/ts relies on the subsequent "Build" step to run tests, and retain
an explicit test step only for projects that need it (e.g., sdk/ts-compat).
Update the job to either delete the "Run tests" step or wrap it with a
conditional that checks inputs.project-path (or normalize an allowlist) so only
projects whose build does not run tests execute "npm test".
In `@sdk/ts/test/unit/rpc-drift.test.ts`:
- Around line 20-35: The extractRouterHandlers function currently drops
unresolved router constants silently; change it to fail-fast by detecting any
methodNames for which namedLiterals.get(name) is undefined and throwing (or
asserting) with a clear error listing the unresolved method constant names so
the test surfaces missing/renamed constants; locate the lookup in
extractRouterHandlers (the map namedLiterals and the final map/filter over
methodNames) and replace the silent .filter(Boolean) behavior with an explicit
check that collects missing names and throws if any exist, otherwise return the
resolved set as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86f72b97-6dbf-4888-94bc-2b9419a00f40
📒 Files selected for processing (13)
.github/workflows/main-pr.yml.github/workflows/main-push.yml.github/workflows/test-sdk.ymldocs/api.yamlpkg/rpc/README.mdpkg/rpc/api.gopkg/rpc/client.gopkg/rpc/client_test.gopkg/rpc/methods.gosdk/ts/src/rpc/api.tssdk/ts/src/rpc/client.tssdk/ts/src/rpc/methods.tssdk/ts/test/unit/rpc-drift.test.ts
💤 Files with no reviewable changes (9)
- pkg/rpc/README.md
- pkg/rpc/api.go
- pkg/rpc/client.go
- sdk/ts/src/rpc/methods.ts
- pkg/rpc/methods.go
- docs/api.yaml
- sdk/ts/src/rpc/client.ts
- sdk/ts/src/rpc/api.ts
- pkg/rpc/client_test.go
Comment on lines
+39
to
+49
| - name: Run tests | ||
| run: npm test | ||
| working-directory: ${{ inputs.project-path }} | ||
|
|
||
| - name: Lint | ||
| run: npm run lint | ||
| working-directory: ${{ inputs.project-path }} | ||
|
|
||
| - name: Build | ||
| run: npm run build | ||
| working-directory: ${{ inputs.project-path }} |
Contributor
There was a problem hiding this comment.
Double test execution for sdk/ts — build already runs test.
For sdk/ts, package.json defines "build": "npm run test && tsc", so adding both a npm test step (Line 40) and a npm run build step (Line 48) causes Jest to run twice for that project, unnecessarily lengthening CI. Based on learnings: "Do not run npm test && npm run build in sdk/ts (double-tests; build already runs tests)".
Consider either dropping the standalone test step, or splitting the reusable workflow logic so that sdk/ts relies on npm run build for its test pass while sdk/ts-compat (whose build is plain tsc) keeps the explicit npm test step.
Possible fix (single build step covers tests for sdk/ts; compat would need test kept separately)
- - name: Run tests
- run: npm test
- working-directory: ${{ inputs.project-path }}
-
- name: Lint
run: npm run lint
working-directory: ${{ inputs.project-path }}
- name: Build
run: npm run build
working-directory: ${{ inputs.project-path }}Note: this requires ensuring sdk/ts-compat's build also triggers tests (or keeping npm test for that project specifically).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test-sdk.yml around lines 39 - 49, The workflow runs tests
twice for sdk/ts because package.json's "build" executes "npm run test && tsc";
remove or conditionalize the standalone "Run tests" step (the step named "Run
tests" that runs "npm test") so sdk/ts relies on the subsequent "Build" step to
run tests, and retain an explicit test step only for projects that need it
(e.g., sdk/ts-compat). Update the job to either delete the "Run tests" step or
wrap it with a conditional that checks inputs.project-path (or normalize an
allowlist) so only projects whose build does not run tests execute "npm test".
nksazonov
approved these changes
Apr 22, 2026
Contributor
nksazonov
left a comment
nksazonov
left a comment
There was a problem hiding this comment.
Generally good, with some comments
philanton
approved these changes
Apr 22, 2026
Contributor
There was a problem hiding this comment.
Cool initiative! Later, we could upgrade this one or/and add something similar for SDK Client (and anything else worth checking integrity)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sdk/ts-compat/test/unit/client.test.ts (1)
1-122: LGTM — focused coverage for both session shapes.The two scenarios (v1
appDefinitionand legacy flat fields) directly exercise the new branch inmapSessions, includingbigint → numberandDecimal → scaled stringconversions per asset decimals. Console spies are installed/restored correctly viabeforeEach/afterEach.One optional nit: consider adding a third case that asserts the retry-without-status fallback behavior (
client.tslines 607–623) when a caller passes a concretestatuslike'open', since that path isn't exercised here. Not blocking — can be added in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts-compat/test/unit/client.test.ts` around lines 1 - 122, Add a third unit that verifies the "retry without status" fallback in getAppSessionsList: create a client via makeClient, replace client.innerClient.getAppSessions with a jest mock that mockRejectedValueOnce an Error-like object representing a 400 failure when called with {wallet, status:'open'} and then mockResolvedValueOnce a normal { sessions } response for the subsequent call without status; call client.getAppSessionsList(wallet, 'open') and assert the mock was called twice (first with status, then without) and that the final returned sessions match the expected mapped shape; reference getAppSessionsList and innerClient.getAppSessions in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sdk/ts-compat/test/unit/client.test.ts`:
- Around line 1-122: Add a third unit that verifies the "retry without status"
fallback in getAppSessionsList: create a client via makeClient, replace
client.innerClient.getAppSessions with a jest mock that mockRejectedValueOnce an
Error-like object representing a 400 failure when called with {wallet,
status:'open'} and then mockResolvedValueOnce a normal { sessions } response for
the subsequent call without status; call client.getAppSessionsList(wallet,
'open') and assert the mock was called twice (first with status, then without)
and that the final returned sessions match the expected mapped shape; reference
getAppSessionsList and innerClient.getAppSessions in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89e0c0f3-d99e-4c20-9ef4-94757b1ea4e8
📒 Files selected for processing (4)
.github/workflows/test-sdk.ymlsdk/ts-compat/src/client.tssdk/ts-compat/test/unit/client.test.tssdk/ts/test/unit/rpc-drift.test.ts
✅ Files skipped from review due to trivial changes (2)
- sdk/ts/test/unit/rpc-drift.test.ts
- .github/workflows/test-sdk.yml
nksazonov
approved these changes
Apr 27, 2026
nksazonov
changed the title
ihsraham
added a commit
that referenced
this pull request
May 6, 2026
This is part of the ongoing refinements to the TS SDKs, focused on the compat TypeScript SDK. It is now rebased on current `main` after #701. It makes the compat runtime surface more honest and more useful during migration: - direct-mapping legacy RPC helpers emit real v1-compatible payloads - workflow-only legacy helpers fail fast with clear migration guidance instead of returning fake wire payloads - selected legacy client conveniences are restored on top of the current SDK behavior - compat amount handling keeps the domains explicit: `transfer()` and `getBalances()` use canonical asset raw-unit strings, on-chain token helpers use raw token-unit `bigint`, and app-session allocations stay human-decimal strings - the rebase preserves #701 app-session `appDefinition` mapping and current `nitronode` naming - runtime-focused compat tests cover wire shape, client mappings, and amount semantics Validation run locally: - `npm --prefix sdk/ts-compat run typecheck` - `npm --prefix sdk/ts-compat test` - `npm --prefix sdk/ts-compat run lint` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added on-chain helpers (approve, allowance, token balance) and an open-channels query; RPC helpers now emit real v1-compatible requests and normalize envelope responses. * **Behavior Changes** * Several legacy helper flows now fail fast as migration shims instead of returning placeholders. * App-session submit/close helpers now require an explicit version and preserve human-readable allocation amounts. * **Documentation** * Clarified amount conventions and updated migration guides. * **Tests** * Added unit tests for amount semantics and RPC wire/request-response shapes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
ihsraham
added a commit
that referenced
this pull request
May 6, 2026
## Summary Adds protocol, TS SDK, and compat SDK drift guards on top of current `main`. #699 and #701 are now merged, so this branch has been rebased directly on `main` and preserves current behavior for: - `nitronode` naming and paths - #699 compat runtime semantics and migration surface - #701 `appDefinition` mapping and SDK `applicationID` exports - package versions at `1.2.1` ## Change Areas ### Drift Guards - Adds static guards for RPC method coverage, RPC DTO shape, public API snapshots, consumed ABI surface, app/session-key signing vectors, and response transforms. - Adds compat drift coverage for v1 app-session mapping, legacy flat fallback behavior, empty-session behavior, asset decimal conversion, config pass-through, and session-key public APIs. - Adds runtime smoke coverage that builds and starts an isolated local `nitronode`, then verifies live SDK and compat calls. ### SDK Validation - Validates key-state `states` containers before mapping. - Validates app-session payloads, allocations, app definitions, participants, and quorum at the wire boundary. - Rejects arrays in session key-state record transforms. ### Nitronode Database - Propagates SQLite migration errors from `connectToSqlite`. - Returns Postgres migration errors instead of panicking. - Keeps SQLite auto-migration aligned with current runtime tables, including state, transaction, app participants, and key-state tables. - Uses a parameterized schema existence check and quoted schema identifiers for Postgres schema setup. ### Runtime Smoke And CI - Builds `./nitronode` and uses `NITRONODE_RUNTIME_SMOKE_*` env names in new scripts/workflows/docs. - Requires `NITRONODE_RUNTIME_SMOKE_PRIVATE_KEY` for external smoke mode. - Keeps the hardcoded Anvil key only for isolated local smoke. - Restricts runtime-smoke log directories to repo-local paths. - Passes an allowlisted child-process env instead of forwarding all parent env. - Captures compat logs for failure diagnostics. - Builds Foundry artifacts before TS SDK/static drift ABI checks in CI. ## Review Notes Addressed current actionable review feedback around SDK response validation, database migration error handling, runtime-smoke env/log safety, public API tautology tests, ABI drift-test stability, and stale Clearnode wording in PR-added files. Deferred CodeRabbit docstring coverage and shared public API test utility extraction because they are broader cleanup items, not blockers for this PR. ## Verification Local checks run from a fresh worktree: - `npm --prefix sdk/ts run typecheck` - `npm --prefix sdk/ts test` - `npm --prefix sdk/ts run lint` - `npm --prefix sdk/ts run build:ci` - `npm --prefix sdk/ts-compat run typecheck` - `npm --prefix sdk/ts-compat test` - `npm --prefix sdk/ts-compat run lint` - `npm --prefix sdk/ts-compat run build:ci` - `forge build` for local ABI artifacts - `./scripts/check-protocol-drift.sh --static` - `./scripts/check-protocol-drift.sh --runtime` - `go test ./nitronode/... ./pkg/rpc/...` - `git diff --check` GitHub checks have started on the rebased branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
this is part of the ts sdk refinement work.
it does three things:
channels.v1.get_statessurface from shared rpc, ts raw rpc, and the protocol docssdk/tsto keep the ts raw rpc surface aligned withpkg/rpcand the live clearnode routermainprs and pushes for bothsdk/tsandsdk/ts-compat, using the real package paths and the fulltypecheck/test/lint/buildgateSummary by CodeRabbit
Bug Fixes
Tests
Chores
SDK
Additional compat fix discovered during stress Clearnode testing:
While wiring
cosign-demotowss://clearnode-stress.yellow.org/v1/ws,getAppSessionsList()exposed a compat drift: current v1 SDK sessions return participant/quorum/nonce data underappDefinition, but compat was still reading the old flatparticipantsshape. This PR now mapsappDefinition.participantsfirst and keeps a flat-shape fallback for older callers.