YNU-869: Add TS SDK protocol drift guards#705
Merged
Conversation
ihsraham
requested review from
dimast-x,
nksazonov and
philanton
as code owners
Contributor
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR introduces a comprehensive protocol drift guard system to detect unintended changes to the SDK public API, RPC contract, and cryptographic signing behavior. It adds static and runtime validation tests, CI workflows, and infrastructure scripts (shell, Go, Node.js) to enforce protocol stability across schema, ABI, transforms, and signing vectors. ChangesProtocol Drift Guards System
Database Schema Robustness
Sequence DiagramsequenceDiagram
actor CI as CI / Script
participant Script as runtime-smoke.mjs
participant NK as Nitronode<br/>(spawned)
participant WS as WebSocket
participant Client as TS SDK<br/>Client
participant Compat as NitroliteClient<br/>(compat layer)
CI->>Script: Invoke with env vars
Script->>Script: Write config dir<br/>(temp or custom)
Script->>NK: Spawn nitronode process
NK->>NK: Initialize
NK->>WS: Listen on port 7824
Script->>WS: Attempt connect with timeout
WS-->>Script: Connection open
Script->>Script: Wait for readiness<br/>(with backoff)
Script->>Client: Create with test signers
Script->>Client: ping()
Client->>NK: RPC: ping
NK-->>Client: pong
Client-->>Script: ✓
Script->>Client: getConfig()
Client->>NK: RPC: getConfig
NK-->>Client: config (camelCase)
Client-->>Script: ✓ validated shape
Script->>Client: getAssets()
Client->>NK: RPC: getAssets
NK-->>Client: assets array
Client-->>Script: ✓ bigint conversions
Script->>Client: getLastChannelKeyStates()
Client->>NK: RPC: getLastChannelKeyStates
NK-->>Client: states array (raw)
Client->>Client: Transform each state
Client-->>Script: ✓ typed states
Script->>Compat: Instantiate with client
Script->>Compat: getAppSessionsList()
Compat->>Client: getAppSessions()
Client->>NK: RPC
NK-->>Client: response
Client-->>Compat: sessions
Compat->>Compat: Map to compat shape
Compat-->>Script: ✓ compat works
Script->>Script: Write logs & summary
Script->>Client: Close
Script->>NK: Terminate process
Script->>CI: Exit(0) on success<br/>or Exit(1) + logs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 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 |
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Contributor
|
✅ Created PR with unit tests: #706 |
philanton
approved these changes
Apr 27, 2026
![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: 2
🧹 Nitpick comments (10)
sdk/PROTOCOL_DRIFT_GUARDS.md (1)
14-18: Minor: Sequentialcdcommands won't work as written.The commands shown run in subshells, so the second
cd sdk/ts-compatwould fail if run sequentially in a single shell from repo root (firstcddoesn't persist).📝 Clarify as separate commands
Run package checks directly: ```bash -cd sdk/ts && npm run drift:check -cd sdk/ts-compat && npm run drift:check +(cd sdk/ts && npm run drift:check) +(cd sdk/ts-compat && npm run drift:check) ```Or separate into two distinct code blocks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/PROTOCOL_DRIFT_GUARDS.md` around lines 14 - 18, Update the snippet so the two directory changes run independently instead of relying on a persistent shell state: replace the two lines "cd sdk/ts && npm run drift:check" and "cd sdk/ts-compat && npm run drift:check" with either subshell invocations like "(cd sdk/ts && npm run drift:check)" and "(cd sdk/ts-compat && npm run drift:check)" or split them into two separate code blocks so each command runs from the repo root without depending on the previous cd.sdk/ts/src/session_key_state_transforms.ts (1)
4-9: Consider handling array edge case inasRecord.Arrays pass the
typeof raw !== 'object'check sincetypeof [] === 'object'. If a caller accidentally passes an array, it would be cast to a record and subsequent field lookups would return array indices.🛡️ Optional defensive check
function asRecord(raw: unknown, context: string): Record<string, unknown> { - if (!raw || typeof raw !== 'object') { + if (!raw || typeof raw !== 'object' || Array.isArray(raw)) { throw new Error(`Invalid ${context}: expected object`); } return raw as Record<string, unknown>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts/src/session_key_state_transforms.ts` around lines 4 - 9, The asRecord helper currently treats arrays as objects because typeof [] === 'object'; update asRecord to reject arrays by checking Array.isArray(raw) and throwing an Error (including context) when raw is null/not an object or is an array so callers cannot accidentally get array indices from a cast; ensure you adjust the error message and keep the function signature and return type (asRecord(raw, context): Record<string, unknown>).sdk/ts/test/unit/public-api-drift.test.ts (1)
65-68: Redundant cast on Line 66.The
declarationparameter is already typed asts.Declaration, so casting it again is unnecessary.🔧 Remove redundant cast
function isPrivateOrProtected(declaration: ts.Declaration): boolean { - const flags = ts.getCombinedModifierFlags(declaration as ts.Declaration); + const flags = ts.getCombinedModifierFlags(declaration); return Boolean(flags & (ts.ModifierFlags.Private | ts.ModifierFlags.Protected)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts/test/unit/public-api-drift.test.ts` around lines 65 - 68, The function isPrivateOrProtected unnecessarily casts its parameter when calling ts.getCombinedModifierFlags; update the call in isPrivateOrProtected to pass declaration directly (i.e., remove the redundant cast) so it becomes ts.getCombinedModifierFlags(declaration) while leaving the rest of the logic (flags check against ts.ModifierFlags.Private | ts.ModifierFlags.Protected) intact.sdk/ts/test/unit/abi-drift.test.ts (2)
103-111: Map comparison relies on insertion order.The comparison
[...sdkSigs].toEqual([...artifactSigs])depends on Map iteration order being consistent between SDK and artifact ABIs. Since both are populated by iterating arrays and entries are sorted by name at insertion, this works, but it's fragile.Consider comparing sorted arrays explicitly or using a dedicated matcher:
🔧 Explicit sorted comparison
it('keeps checked-in ChannelHub ABI aligned with Foundry artifact for every artifact function', () => { const artifactSigs = functionSignatures( loadArtifact('contracts/out/ChannelHub.sol/ChannelHub.json') ); const sdkSigs = functionSignatures(ChannelHubAbi as readonly AbiEntry[]); - expect([...sdkSigs]).toEqual([...artifactSigs]); + const sortedSdk = [...sdkSigs].sort(([a], [b]) => a.localeCompare(b)); + const sortedArtifact = [...artifactSigs].sort(([a], [b]) => a.localeCompare(b)); + expect(sortedSdk).toEqual(sortedArtifact); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts/test/unit/abi-drift.test.ts` around lines 103 - 111, The test compares Maps via [...sdkSigs] and [...artifactSigs], which relies on insertion order and is fragile; update the test that builds artifactSigs and sdkSigs (from functionSignatures, loadArtifact, ChannelHubAbi) to compare deterministic sorted arrays instead — extract the signatures into arrays, sort them (e.g., by signature string/name) and assert equality of the sorted arrays so order differences won't cause spurious failures.
37-41: Trailing space whenstateMutabilityis undefined.The signature format produces a trailing space when
stateMutabilityis falsy (e.g.,"foo() -> () "). This could cause spurious mismatches.🔧 Trim or conditionally append stateMutability
function signature(entry: AbiEntry): string { const inputs = (entry.inputs ?? []).map(canonicalType).join(','); const outputs = (entry.outputs ?? []).map(canonicalType).join(','); - return `${entry.name}(${inputs}) -> (${outputs}) ${entry.stateMutability ?? ''}`; + const mutability = entry.stateMutability ? ` ${entry.stateMutability}` : ''; + return `${entry.name}(${inputs}) -> (${outputs})${mutability}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts/test/unit/abi-drift.test.ts` around lines 37 - 41, The signature function produces a trailing space when stateMutability is falsy; update the function signature(entry: AbiEntry) to conditionally append stateMutability (or trim the final string) so you only include a leading space and the stateMutability when entry.stateMutability is truthy; locate the function named signature that calls canonicalType on entry.inputs/outputs and modify the return construction to avoid emitting " ... () " when stateMutability is undefined.scripts/drift/runtime-smoke.mjs (3)
297-325: Fragile NitroliteClient instantiation via Object.create.Manually setting internal properties (
assetsBySymbol,_lastAppSessionsListError, etc.) creates tight coupling to NitroliteClient internals. If the class adds required initialization or renames private fields, this will silently break.Consider using a factory or exposing a test helper in the compat package.
💡 Alternative approach
If NitroliteClient cannot be instantiated without a full WebSocket setup, consider adding a static factory method or test helper:
// In sdk/ts-compat, add a test helper: // static createForTesting(innerClient: Client, userAddress: Address): NitroliteClient // Then in smoke test: const compatClient = NitroliteClient.createForTesting(client, wallet);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/drift/runtime-smoke.mjs` around lines 297 - 325, The test constructs a NitroliteClient via Object.create and manually sets internals (assetsBySymbol, _lastAppSessionsListError, etc.), which is fragile; add a safe test factory on NitroliteClient (e.g., static createForTesting(innerClient, userAddress)) or a compat test helper and use it in the smoke test to instantiate compatClient instead of mutating internals; update the test to call NitroliteClient.createForTesting(client, wallet) (or the new helper) and keep assertions around getAppSessionsList and getLastAppSessionsListError unchanged.
305-318: Console suppression may hide diagnostic errors.Blanket suppression of
console.infoandconsole.warnduring the compat test could mask useful diagnostic information if something goes wrong. Consider capturing the output instead of discarding it.🔧 Capture instead of discard
const originalInfo = console.info; const originalWarn = console.warn; + const capturedLogs = []; let compatSessions; try { - console.info = () => {}; - console.warn = () => {}; + console.info = (...args) => capturedLogs.push(['info', ...args]); + console.warn = (...args) => capturedLogs.push(['warn', ...args]); compatSessions = await withTimeout( 'compat.getAppSessionsList', compatClient.getAppSessionsList() ); } finally { console.info = originalInfo; console.warn = originalWarn; } + // capturedLogs available for debugging if assertions fail🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/drift/runtime-smoke.mjs` around lines 305 - 318, The current try/finally entirely silences console.info and console.warn (originalInfo/originalWarn) while awaiting compatClient.getAppSessionsList() via withTimeout, which can hide diagnostics; change this to capture calls instead of discarding them by replacing console.info and console.warn with wrappers that push messages into local arrays (e.g., infoLog/warnLog) while the await runs, then restore originals in the finally block and optionally log or attach the captured info/warn if compatSessions or the timeout fails; keep the same withTimeout call and variable names (compatSessions, withTimeout, compatClient.getAppSessionsList) so the behavior and sequencing remain unchanged.
22-24: Hardcoded private key is acceptable for smoke testing.This is the well-known Anvil/Hardhat test account
#2private key, safe for local testing purposes. Consider adding a brief comment noting this.📝 Add clarifying comment
+// Well-known Anvil/Hardhat test account `#2` - safe for local testing only const privateKey = '0x59c6995e998f97a5a0044966f094538f0d0921e301baca6a9ae52cd7834c90b9';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/drift/runtime-smoke.mjs` around lines 22 - 24, Hardcoded privateKey constant currently contains a known Anvil/Hardhat test account key; add a concise comment next to the privateKey declaration explaining that this is the well-known Anvil/Hardhat test account `#2` private key used only for local/smoke testing and not for production, to clarify intent and safety when reviewing the runtime-smoke.mjs script and the privateKey symbol.sdk/ts-compat/test/unit/public-api-drift.test.ts (2)
65-68: Redundant cast on Line 66 (same as sdk/ts version).The
declarationparameter is already typed asts.Declaration.🔧 Remove redundant cast
function isPrivateOrProtected(declaration: ts.Declaration): boolean { - const flags = ts.getCombinedModifierFlags(declaration as ts.Declaration); + const flags = ts.getCombinedModifierFlags(declaration); return Boolean(flags & (ts.ModifierFlags.Private | ts.ModifierFlags.Protected)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts-compat/test/unit/public-api-drift.test.ts` around lines 65 - 68, The cast is redundant in isPrivateOrProtected — remove the unnecessary cast in the call to ts.getCombinedModifierFlags so it uses the declaration parameter directly (i.e., call ts.getCombinedModifierFlags(declaration)), keeping the rest of the function (flags & (ts.ModifierFlags.Private | ts.ModifierFlags.Protected)) unchanged to preserve behavior.
27-179: Consider extracting shared test utilities.The
serializePublicApi,normalizeText,declarationKind, and related helper functions are nearly identical betweensdk/tsandsdk/ts-compatpublic-api-drift tests. Consider extracting to a shared test utility module to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts-compat/test/unit/public-api-drift.test.ts` around lines 27 - 179, Extract the duplicated public-api-drift test helpers into a shared test utility module and update both test suites to import from it: move normalizeText, createPackageProgram, declarationKind, signaturesForType, isPrivateOrProtected, propertiesForType, enumMembers, and serializePublicApi (and any FORMAT_FLAGS or type aliases they depend on) into a new shared file (e.g., a test-utils module), ensure it exports those functions, remove the duplicate implementations from sdk/ts and sdk/ts-compat tests, and change the tests to import the helpers from the new shared module so both suites reuse the single implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/ts/src/client.ts`:
- Around line 1707-1709: Validate that resp.states is an array before calling
map: replace the direct mapping with an Array.isArray(resp.states) check and
throw a clear drift-guard error (or return an empty list) if it’s not an array;
apply this guard at both places where resp.states is mapped (the callsite that
invokes transformChannelSessionKeyState with the `channel session key
state[...]` label and the other mapping around lines 1759-1761 that uses the
same transform), so malformed responses yield a descriptive error instead of a
generic .map crash.
In `@sdk/ts/src/utils.ts`:
- Around line 358-361: The current guard reads raw.allocations and uses `|| []`,
which hides malformed or non-object top-level payloads; update the code in
utils.ts to first validate that `raw` is a non-null object (not a primitive),
then assert that `raw` has an `allocations` property and that
`Array.isArray(raw.allocations)` is true, and throw a clear Error if those
checks fail instead of defaulting to an empty array; locate the
`raw`/`allocations` extraction in the function surrounding the shown diff and
replace the `const allocations = raw.allocations || [];` pattern with explicit
validations and a thrown error on invalid payloads.
---
Nitpick comments:
In `@scripts/drift/runtime-smoke.mjs`:
- Around line 297-325: The test constructs a NitroliteClient via Object.create
and manually sets internals (assetsBySymbol, _lastAppSessionsListError, etc.),
which is fragile; add a safe test factory on NitroliteClient (e.g., static
createForTesting(innerClient, userAddress)) or a compat test helper and use it
in the smoke test to instantiate compatClient instead of mutating internals;
update the test to call NitroliteClient.createForTesting(client, wallet) (or the
new helper) and keep assertions around getAppSessionsList and
getLastAppSessionsListError unchanged.
- Around line 305-318: The current try/finally entirely silences console.info
and console.warn (originalInfo/originalWarn) while awaiting
compatClient.getAppSessionsList() via withTimeout, which can hide diagnostics;
change this to capture calls instead of discarding them by replacing
console.info and console.warn with wrappers that push messages into local arrays
(e.g., infoLog/warnLog) while the await runs, then restore originals in the
finally block and optionally log or attach the captured info/warn if
compatSessions or the timeout fails; keep the same withTimeout call and variable
names (compatSessions, withTimeout, compatClient.getAppSessionsList) so the
behavior and sequencing remain unchanged.
- Around line 22-24: Hardcoded privateKey constant currently contains a known
Anvil/Hardhat test account key; add a concise comment next to the privateKey
declaration explaining that this is the well-known Anvil/Hardhat test account `#2`
private key used only for local/smoke testing and not for production, to clarify
intent and safety when reviewing the runtime-smoke.mjs script and the privateKey
symbol.
In `@sdk/PROTOCOL_DRIFT_GUARDS.md`:
- Around line 14-18: Update the snippet so the two directory changes run
independently instead of relying on a persistent shell state: replace the two
lines "cd sdk/ts && npm run drift:check" and "cd sdk/ts-compat && npm run
drift:check" with either subshell invocations like "(cd sdk/ts && npm run
drift:check)" and "(cd sdk/ts-compat && npm run drift:check)" or split them into
two separate code blocks so each command runs from the repo root without
depending on the previous cd.
In `@sdk/ts-compat/test/unit/public-api-drift.test.ts`:
- Around line 65-68: The cast is redundant in isPrivateOrProtected — remove the
unnecessary cast in the call to ts.getCombinedModifierFlags so it uses the
declaration parameter directly (i.e., call
ts.getCombinedModifierFlags(declaration)), keeping the rest of the function
(flags & (ts.ModifierFlags.Private | ts.ModifierFlags.Protected)) unchanged to
preserve behavior.
- Around line 27-179: Extract the duplicated public-api-drift test helpers into
a shared test utility module and update both test suites to import from it: move
normalizeText, createPackageProgram, declarationKind, signaturesForType,
isPrivateOrProtected, propertiesForType, enumMembers, and serializePublicApi
(and any FORMAT_FLAGS or type aliases they depend on) into a new shared file
(e.g., a test-utils module), ensure it exports those functions, remove the
duplicate implementations from sdk/ts and sdk/ts-compat tests, and change the
tests to import the helpers from the new shared module so both suites reuse the
single implementation.
In `@sdk/ts/src/session_key_state_transforms.ts`:
- Around line 4-9: The asRecord helper currently treats arrays as objects
because typeof [] === 'object'; update asRecord to reject arrays by checking
Array.isArray(raw) and throwing an Error (including context) when raw is
null/not an object or is an array so callers cannot accidentally get array
indices from a cast; ensure you adjust the error message and keep the function
signature and return type (asRecord(raw, context): Record<string, unknown>).
In `@sdk/ts/test/unit/abi-drift.test.ts`:
- Around line 103-111: The test compares Maps via [...sdkSigs] and
[...artifactSigs], which relies on insertion order and is fragile; update the
test that builds artifactSigs and sdkSigs (from functionSignatures,
loadArtifact, ChannelHubAbi) to compare deterministic sorted arrays instead —
extract the signatures into arrays, sort them (e.g., by signature string/name)
and assert equality of the sorted arrays so order differences won't cause
spurious failures.
- Around line 37-41: The signature function produces a trailing space when
stateMutability is falsy; update the function signature(entry: AbiEntry) to
conditionally append stateMutability (or trim the final string) so you only
include a leading space and the stateMutability when entry.stateMutability is
truthy; locate the function named signature that calls canonicalType on
entry.inputs/outputs and modify the return construction to avoid emitting " ...
() " when stateMutability is undefined.
In `@sdk/ts/test/unit/public-api-drift.test.ts`:
- Around line 65-68: The function isPrivateOrProtected unnecessarily casts its
parameter when calling ts.getCombinedModifierFlags; update the call in
isPrivateOrProtected to pass declaration directly (i.e., remove the redundant
cast) so it becomes ts.getCombinedModifierFlags(declaration) while leaving the
rest of the logic (flags check against ts.ModifierFlags.Private |
ts.ModifierFlags.Protected) intact.
🪄 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: d1c12dd8-3da9-450b-bd07-0a787f87b109
⛔ Files ignored due to path filters (2)
sdk/ts-compat/test/unit/__snapshots__/public-api-drift.test.ts.snapis excluded by!**/*.snapsdk/ts/test/unit/__snapshots__/public-api-drift.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
.github/workflows/main-pr.yml.github/workflows/main-push.yml.github/workflows/protocol-drift-external-smoke.ymlclearnode/store/database/database.goscripts/check-protocol-drift.shscripts/drift/generate-app-signing-vectors.goscripts/drift/runtime-smoke.mjssdk/PROTOCOL_DRIFT_GUARDS.mdsdk/ts-compat/package.jsonsdk/ts-compat/test/unit/client.test.tssdk/ts-compat/test/unit/public-api-drift.test.tssdk/ts/package.jsonsdk/ts/src/blockchain/evm/channel_hub_abi.tssdk/ts/src/client.tssdk/ts/src/session_key_state_transforms.tssdk/ts/src/utils.tssdk/ts/test/unit/abi-drift.test.tssdk/ts/test/unit/app-signing-drift.test.tssdk/ts/test/unit/public-api-drift.test.tssdk/ts/test/unit/rpc-drift.test.tssdk/ts/test/unit/rpc-dto-drift.test.tssdk/ts/test/unit/transform-drift.test.ts
ihsraham
changed the title
nksazonov
reviewed
May 5, 2026
ihsraham
force-pushed
the
protocol-ts-sdk-drift-guards
branch
from
70cf1c9 to
ea94a78
Compare
ihsraham
force-pushed
the
protocol-ts-sdk-drift-guards
branch
from
ea94a78 to
b757a98
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
nitronode/store/database/database.go (2)
173-175: 💤 Low valuePrefer
errors.Is(err, sql.ErrNoRows)over direct comparison.Direct equality with
sql.ErrNoRowsworks today but breaks the moment a driver or middleware (e.g., a wrapped sqlx error, retry layer) returns a wrapped error. Usingerrors.Isis the idiomatic Go form and is robust to wrapping.♻️ Proposed change
var exists int - if err := db.QueryRow("SELECT 1 FROM information_schema.schemata WHERE schema_name=$1", cnf.Schema).Scan(&exists); err != nil && err != sql.ErrNoRows { + if err := db.QueryRow("SELECT 1 FROM information_schema.schemata WHERE schema_name=$1", cnf.Schema).Scan(&exists); err != nil && !errors.Is(err, sql.ErrNoRows) { return fmt.Errorf("error while checking schema existance: %s", err.Error()) } else if err == nil {Add
"errors"to the import block.🤖 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 `@nitronode/store/database/database.go` around lines 173 - 175, Replace the direct comparison to sql.ErrNoRows with errors.Is to handle wrapped errors: add "errors" to the import block and change the check in the QueryRow result handling to use errors.Is(err, sql.ErrNoRows) (e.g. if err != nil && !errors.Is(err, sql.ErrNoRows) { return fmt.Errorf(...) } else if err == nil { ... }) so wrapped driver/middleware errors are handled correctly; update the conditional around db.QueryRow(...).Scan(&exists) accordingly.
180-180: 💤 Low valueConsider using
pq.QuoteIdentifierinstead of the hand-rolled helper.
github.com/lib/pqis already available as a transitive dependency (viagorm.io/driver/postgres), and it shipspq.QuoteIdentifierwith identical semantics to the current implementation. This approach additionally guards against embedded NUL bytes and removes security-sensitive code that future contributors must keep correct.Replace the
quotePostgresIdentifiercalls at lines 180 and 202, and remove the function definition at lines 244-246:♻️ Optional refactor using pq.QuoteIdentifier
import ( "database/sql" "embed" "fmt" "log" - "strings" "time" "github.com/jmoiron/sqlx" + "github.com/lib/pq" "github.com/pressly/goose/v3" ... )- if _, err = db.Exec(fmt.Sprintf("CREATE SCHEMA IF NOT EXISTS %s", quotePostgresIdentifier(cnf.Schema))); err != nil { + if _, err = db.Exec(fmt.Sprintf("CREATE SCHEMA IF NOT EXISTS %s", pq.QuoteIdentifier(cnf.Schema))); err != nil {- if _, err := db.Exec(fmt.Sprintf("SET search_path TO %s", quotePostgresIdentifier(cnf.Schema))); err != nil { + if _, err := db.Exec(fmt.Sprintf("SET search_path TO %s", pq.QuoteIdentifier(cnf.Schema))); err != nil {-func quotePostgresIdentifier(identifier string) string { - return `"` + strings.ReplaceAll(identifier, `"`, `""`) + `"` -}🤖 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 `@nitronode/store/database/database.go` at line 180, Replace the hand-rolled quotePostgresIdentifier usage with pq.QuoteIdentifier from github.com/lib/pq: call pq.QuoteIdentifier(cnf.Schema) where the code currently builds the CREATE SCHEMA SQL and where it sets the search_path (i.e., the two places using quotePostgresIdentifier), import the pq package, and then remove the quotePostgresIdentifier function definition entirely; keep existing error handling and string formatting but pass the pq-quoted identifier so NUL-byte checks and correct quoting are delegated to pq.scripts/drift/runtime-smoke.mjs (1)
336-341: 💤 Low valueObject.create bypass on
NitroliteClientis brittle to internal refactors.Constructing the compat client via
Object.create(NitroliteClient.prototype)and hand-injectinguserAddress,innerClient,assetsBySymbol, and the two_lastAppSessionsListError*private fields skips whatever the real constructor does and silently breaks if it ever initializes additional state used bygetAppSessionsList. Consider either invoking the real constructor with stub args or extracting the mapping as a separate exported helper that the smoke can call without faking instance state.🤖 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 `@scripts/drift/runtime-smoke.mjs` around lines 336 - 341, The compat client is being faked via Object.create(NitroliteClient.prototype) and manually setting userAddress, innerClient, assetsBySymbol, _lastAppSessionsListError and _lastAppSessionsListErrorLogged which bypasses NitroliteClient constructor logic used by getAppSessionsList; fix by either creating a real instance of NitroliteClient (call new NitroliteClient(...) with minimal/stubbed args so the constructor runs and then set innerClient/userAddress) or refactor the mapping logic into an exported helper (e.g., export a buildCompatMapping or populateAssetsBySymbol helper) and call that from the smoke test instead of faking instance state; update the runtime-smoke.mjs code to use the real constructor or the new helper and remove the manual injection of private fields (_lastAppSessionsListError/_lastAppSessionsListErrorLogged)..github/workflows/main-pr.yml (1)
34-114: ⚡ Quick winConsider extracting the drift jobs into a reusable workflow.
The two jobs added here are duplicated almost verbatim in
.github/workflows/main-push.yml(lines 34-114 of that file), which risks the two paths drifting (e.g., a future Node version bump applied to one but not the other). The repo already follows the reusable-workflow pattern (test-go.yml,test-forge.yml,test-sdk.ymlat lines 13, 17, 21 of this file), so extracting these into./.github/workflows/test-protocol-drift.ymlwithstatic/runtimemodes would fit existing conventions.🤖 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/main-pr.yml around lines 34 - 114, Duplicate jobs test-protocol-drift-static and test-protocol-drift-runtime should be extracted into a reusable workflow file (e.g., .github/workflows/test-protocol-drift.yml) that accepts an input like mode (static|runtime) and reproduces the same steps (checkout, setup-node/setup-go where used, npm ci/build steps, running ./scripts/check-protocol-drift.sh with --static or --runtime, and the runtime artifact upload on failure). Replace the two jobs in this workflow and the duplicate ones in main-push.yml with calls to the reusable workflow using uses: ./.github/workflows/test-protocol-drift.yml and pass with: mode: static (or runtime), preserving env (NITRONODE_RUNTIME_SMOKE_LOG_DIR), timeout-minutes, permissions, caching inputs (node-version-file, cache-dependency-path), and the conditional upload-artifact behavior so runtime logs are still uploaded on failure.
🤖 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.
Inline comments:
In `@scripts/drift/runtime-smoke.mjs`:
- Around line 91-114: The openWebSocket function uses the global WebSocket which
is only present in Node ≥22; update the script to detect availability and
fallback to a polyfill: inside openWebSocket (or before its first use) check if
globalThis.WebSocket is undefined via process.versions.node (or simply if
(!globalThis.WebSocket)), and if missing require and assign a compatible
implementation (e.g., const WebSocket = require('ws')) before constructing new
WebSocket(url); alternatively, gate the function to throw a clear error
suggesting to run on Node ≥22 or update package engines if you prefer to mandate
Node 22+. Ensure the symbol WebSocket used in openWebSocket and the timeoutMs
logic remains unchanged.
---
Nitpick comments:
In @.github/workflows/main-pr.yml:
- Around line 34-114: Duplicate jobs test-protocol-drift-static and
test-protocol-drift-runtime should be extracted into a reusable workflow file
(e.g., .github/workflows/test-protocol-drift.yml) that accepts an input like
mode (static|runtime) and reproduces the same steps (checkout,
setup-node/setup-go where used, npm ci/build steps, running
./scripts/check-protocol-drift.sh with --static or --runtime, and the runtime
artifact upload on failure). Replace the two jobs in this workflow and the
duplicate ones in main-push.yml with calls to the reusable workflow using uses:
./.github/workflows/test-protocol-drift.yml and pass with: mode: static (or
runtime), preserving env (NITRONODE_RUNTIME_SMOKE_LOG_DIR), timeout-minutes,
permissions, caching inputs (node-version-file, cache-dependency-path), and the
conditional upload-artifact behavior so runtime logs are still uploaded on
failure.
In `@nitronode/store/database/database.go`:
- Around line 173-175: Replace the direct comparison to sql.ErrNoRows with
errors.Is to handle wrapped errors: add "errors" to the import block and change
the check in the QueryRow result handling to use errors.Is(err, sql.ErrNoRows)
(e.g. if err != nil && !errors.Is(err, sql.ErrNoRows) { return fmt.Errorf(...) }
else if err == nil { ... }) so wrapped driver/middleware errors are handled
correctly; update the conditional around db.QueryRow(...).Scan(&exists)
accordingly.
- Line 180: Replace the hand-rolled quotePostgresIdentifier usage with
pq.QuoteIdentifier from github.com/lib/pq: call pq.QuoteIdentifier(cnf.Schema)
where the code currently builds the CREATE SCHEMA SQL and where it sets the
search_path (i.e., the two places using quotePostgresIdentifier), import the pq
package, and then remove the quotePostgresIdentifier function definition
entirely; keep existing error handling and string formatting but pass the
pq-quoted identifier so NUL-byte checks and correct quoting are delegated to pq.
In `@scripts/drift/runtime-smoke.mjs`:
- Around line 336-341: The compat client is being faked via
Object.create(NitroliteClient.prototype) and manually setting userAddress,
innerClient, assetsBySymbol, _lastAppSessionsListError and
_lastAppSessionsListErrorLogged which bypasses NitroliteClient constructor logic
used by getAppSessionsList; fix by either creating a real instance of
NitroliteClient (call new NitroliteClient(...) with minimal/stubbed args so the
constructor runs and then set innerClient/userAddress) or refactor the mapping
logic into an exported helper (e.g., export a buildCompatMapping or
populateAssetsBySymbol helper) and call that from the smoke test instead of
faking instance state; update the runtime-smoke.mjs code to use the real
constructor or the new helper and remove the manual injection of private fields
(_lastAppSessionsListError/_lastAppSessionsListErrorLogged).
🪄 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: 336c0e31-7b9c-4de5-bf1f-0621e744c78e
📒 Files selected for processing (9)
.github/workflows/main-pr.yml.github/workflows/main-push.yml.github/workflows/protocol-drift-external-smoke.ymlnitronode/store/database/database.goscripts/check-protocol-drift.shscripts/drift/generate-app-signing-vectors.goscripts/drift/runtime-smoke.mjssdk/PROTOCOL_DRIFT_GUARDS.mdsdk/ts-compat/package.json
ihsraham
force-pushed
the
protocol-ts-sdk-drift-guards
branch
from
b757a98 to
32ad8fc
Compare
This was referenced May 8, 2026
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.
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
mainand preserves current behavior for:nitronodenaming and pathsappDefinitionmapping and SDKapplicationIDexports1.2.1Change Areas
Drift Guards
nitronode, then verifies live SDK and compat calls.SDK Validation
statescontainers before mapping.Nitronode Database
connectToSqlite.Runtime Smoke And CI
./nitronodeand usesNITRONODE_RUNTIME_SMOKE_*env names in new scripts/workflows/docs.NITRONODE_RUNTIME_SMOKE_PRIVATE_KEYfor external smoke mode.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 typechecknpm --prefix sdk/ts testnpm --prefix sdk/ts run lintnpm --prefix sdk/ts run build:cinpm --prefix sdk/ts-compat run typechecknpm --prefix sdk/ts-compat testnpm --prefix sdk/ts-compat run lintnpm --prefix sdk/ts-compat run build:ciforge buildfor local ABI artifacts./scripts/check-protocol-drift.sh --static./scripts/check-protocol-drift.sh --runtimego test ./nitronode/... ./pkg/rpc/...git diff --checkGitHub checks have started on the rebased branch.