relayburn-sdk-node: wire conformance suite in CI (#247d)#355
Merged
willwashburn merged 7 commits intomainfrom May 7, 2026
Merged
relayburn-sdk-node: wire conformance suite in CI (#247d)#355willwashburn merged 7 commits intomainfrom
willwashburn merged 7 commits intomainfrom
Conversation
…lves
`napi build --js src/binding.cjs --dts src/binding.d.ts` regenerated the
loader at `packages/sdk-node/src/binding.cjs` but emitted the actual
`.node` artifact at `packages/sdk-node/index.<target>.node` (package
root). The generated dispatcher checks `existsSync(join(__dirname,
'index.<target>.node'))` with `__dirname = packages/sdk-node/src/`, so
the local-file branch never matched and it fell through to
`require('@relayburn/sdk-darwin-arm64')` — not installed in dev,
producing "native binding not found".
Pass `src` as the positional `[destDir]` argument with `--js
binding.cjs --dts binding.d.ts` (relative to destDir) so all three
outputs land in `src/` next to `index.js`. Update the CI artifact
upload glob and `.gitignore` to match the new path.
The conformance suite required `tests/fixtures/ledger/` to exist with a canonical `ledger.jsonl` + `content/` sidecar, but no such fixture was ever committed (the file's header referenced a `prepare-fixture-ledger` CI step that didn't land). Rather than commit a binary-ish snapshot that drifts silently, drive the suite from `tests/fixtures/cli-golden/scripts/build-ledger.mjs` — the hand-curated, byte-deterministic builder already maintained by the cli-golden tests. Each conformance run spawns the builder once into a tmp dir, then `cpSync`s the produced ledger into per-impl tmp homes so TS and napi reads see identical state. Keeps conformance self-contained and in lock-step with the same fixture cli-golden owns. Ingest is intentionally read-only (HOME pinned at an empty tmp tree on both sides) so the verb returns the trivial empty report rather than scanning the runner's real `~/.claude/projects/`. Deep-corpus ingest conformance is tracked as an α follow-up.
With α (#354) shape-conformant and the loader + fixture seeding wired up, the conformance suite can finally run for real. Flip the gate from '0' to '1' so `node --test test/conformance.test.js` does a `deepStrictEqual` check across all 7 verbs against TS `@relayburn/sdk@1.x` instead of skipping itself. Also adds a `pnpm run build` step before the conformance run — the TS 1.x SDK imports `@relayburn/{ledger,analyze,ingest,reader}`, which ship as `dist/` and need a `tsc --build` pass. Closes the β slice of #247; α follow-ups still required for the BigInt-vs-Number divergences and the Rust SDK's JSONL→SQLite replay gap.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughGitHub Actions workflow, npm build scripts, and tests changed to emit N-API artifacts into Changesnapi-rs Binding Conformance CI Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/napi-build.yml (1)
13-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBroaden the path filter to include the inputs this job now executes.
This workflow now depends on the seeded fixture builder and the TS 1.x workspace build, but changes under
tests/fixtures/cli-golden/**,packages/sdk/**, and the TS packages it imports can bypass the gate entirely because they do not trigger this workflow today.Suggested path additions
pull_request: paths: - 'crates/relayburn-sdk/**' - 'crates/relayburn-sdk-node/**' + - 'packages/sdk/**' + - 'packages/ledger/**' + - 'packages/analyze/**' + - 'packages/ingest/**' + - 'packages/reader/**' - 'packages/sdk-node/**' + - 'tests/fixtures/cli-golden/**' + - 'package.json' + - 'pnpm-lock.yaml' + - 'pnpm-workspace.yaml' - 'Cargo.toml' - 'Cargo.lock' - 'rust-toolchain.toml' - '.github/workflows/napi-build.yml' push: branches: [main] paths: - 'crates/relayburn-sdk/**' - 'crates/relayburn-sdk-node/**' + - 'packages/sdk/**' + - 'packages/ledger/**' + - 'packages/analyze/**' + - 'packages/ingest/**' + - 'packages/reader/**' - 'packages/sdk-node/**' + - 'tests/fixtures/cli-golden/**' + - 'package.json' + - 'pnpm-lock.yaml' + - 'pnpm-workspace.yaml' - 'Cargo.toml' - 'Cargo.lock' - 'rust-toolchain.toml' - '.github/workflows/napi-build.yml'🤖 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/napi-build.yml around lines 13 - 30, The workflow's path filters under the 'paths' arrays for the on: pull_request and push triggers are too narrow and must include additional inputs this job runs (fixture builder and TS workspace builds); update both 'paths' arrays in .github/workflows/napi-build.yml to add patterns such as tests/fixtures/cli-golden/**, packages/sdk/** and the TypeScript workspace packages (e.g., packages/*/** or the specific TS package globs the job depends on) so changes to those files will trigger the workflow; ensure the same added globs appear in both the pull_request and push path lists.
🧹 Nitpick comments (1)
packages/sdk-node/CHANGELOG.md (1)
19-24: ⚡ Quick winTrim this Unreleased entry to the user-visible effect.
This reads like implementation notes (
src/,build-ledger.mjs,RELAYBURN_SDK_NAPI_BUILT=1,deepStrictEqual) instead of a changelog entry. Please reduce it to the command/API touched and the practical effect in one concise bullet.As per coding guidelines, Curate
[Unreleased]sections inpackages/*/CHANGELOG.mdas PRs land with concise, impact-first entries: name the command/API/schema touched and practical effect; drop issue/PR links, internal notes, backstory, and 'foundation for...' phrasing🤖 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 `@packages/sdk-node/CHANGELOG.md` around lines 19 - 24, Trim the [Unreleased] CHANGELOG.md entry down to a single user-facing bullet that names the touched command/API and its practical effect: state that the napi build artifacts are now produced in CI enabling the conformance suite to validate the native SDK build and TypeScript compatibility; remove implementation details (src/, build-ledger.mjs, RELAYBURN_SDK_NAPI_BUILT, deepStrictEqual, PR number) so only the command ("napi build") and the observable outcome (CI now verifies native SDK artifacts/TS compatibility) remain.
🤖 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 @.github/workflows/napi-build.yml:
- Around line 188-202: The conformance step currently forces
RELAYBURN_SDK_NAPI_BUILT=1 and runs node --test 'test/conformance.test.js',
making known alpha mismatches a hard CI gate; change the job to be opt-in or
non-blocking by either (a) removing the hard-set env RELAYBURN_SDK_NAPI_BUILT
(so callers must opt-in via workflow_dispatch/input or a matrix entry), or (b)
add a conditional (if: github.event.inputs.run_napi_conformance == 'true')
around the step or top-level job, or (c) mark the step non-fatal with
continue-on-error: true until BigInt/JSONL→SQLite parity is fixed; use the job
title "Conformance test (TS `@relayburn/sdk`@1.x vs napi-rs `@2.0.0-pre`)" and the
env var RELAYBURN_SDK_NAPI_BUILT to locate the block to update.
- Around line 188-202: The conformance test step ("Conformance test (TS
`@relayburn/sdk`@1.x vs napi-rs `@2.0.0-pre`)") is running on a cross-compiled
aarch64 build and trying to load an arm64 .node on an x64 runner; add a guard to
that workflow step so it only executes the runtime test when the build target
and runner architecture match (e.g., check matrix.target for "aarch64" and only
run if runner.arch is ARM64, otherwise skip the test); modify the step condition
(the step with run: node --test 'test/conformance.test.js') to short-circuit on
cross-compiled aarch64-on-x64 combinations.
---
Outside diff comments:
In @.github/workflows/napi-build.yml:
- Around line 13-30: The workflow's path filters under the 'paths' arrays for
the on: pull_request and push triggers are too narrow and must include
additional inputs this job runs (fixture builder and TS workspace builds);
update both 'paths' arrays in .github/workflows/napi-build.yml to add patterns
such as tests/fixtures/cli-golden/**, packages/sdk/** and the TypeScript
workspace packages (e.g., packages/*/** or the specific TS package globs the job
depends on) so changes to those files will trigger the workflow; ensure the same
added globs appear in both the pull_request and push path lists.
---
Nitpick comments:
In `@packages/sdk-node/CHANGELOG.md`:
- Around line 19-24: Trim the [Unreleased] CHANGELOG.md entry down to a single
user-facing bullet that names the touched command/API and its practical effect:
state that the napi build artifacts are now produced in CI enabling the
conformance suite to validate the native SDK build and TypeScript compatibility;
remove implementation details (src/, build-ledger.mjs, RELAYBURN_SDK_NAPI_BUILT,
deepStrictEqual, PR number) so only the command ("napi build") and the
observable outcome (CI now verifies native SDK artifacts/TS compatibility)
remain.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f8a7faee-8f21-4d05-84a9-b518b30486f2
📒 Files selected for processing (5)
.github/workflows/napi-build.yml.gitignorepackages/sdk-node/CHANGELOG.mdpackages/sdk-node/package.jsonpackages/sdk-node/test/conformance.test.js
The conformance fixture seeder (`tests/fixtures/cli-golden/scripts/build-ledger.mjs`) imports `@relayburn/ledger` directly. Node ESM resolution walks up `node_modules/` from the script's location, but pnpm's default linking puts `@relayburn/ledger` under each consumer's `packages/<pkg>/node_modules/`, not the workspace root. So the seeder fails with `ERR_MODULE_NOT_FOUND` in any clean checkout — including all four legs of `napi-build.yml`, where it broke every conformance run before the deepStrictEqual phase. Fix: add `.npmrc` with `public-hoist-pattern[]=@relayburn/*` so pnpm hoists workspace siblings into the root `node_modules/`. The seeder's import now resolves, conformance proceeds to actual shape diffs (the remaining BigInt + JSONL→SQLite divergences are tracked as α follow-ups). Also unbreaks `pnpm run golden:capture`, which had the same latent bug.
willwashburn
added a commit
that referenced
this pull request
May 7, 2026
napi-rs serializes Rust u64/i64 as JS BigInt, but the TS 1.x @relayburn/sdk shape (mirrored in src/index.d.ts) emits plain Number for the same fields. PR alpha (#354) made the napi binding shape-conformant; PR beta (#355) flipped the conformance gate in CI and surfaced 6/7 verbs failing because of this BigInt vs Number wire-shape gap (and a separate JSONL->SQLite read-side gap fixed by a sibling PR). Add a coerceBigInts(value) helper that recursively walks a verb's return value and downcasts BigInt to Number when the value fits in [Number.MIN_SAFE_INTEGER, Number.MAX_SAFE_INTEGER]; values outside that range stay BigInt to avoid silent precision loss. Wrap each verb's return in both the ESM facade (src/index.js) and the CJS mirror (src/index.cjs): summary, sessionCost, overhead, overheadTrim, hotspots, compare, ingest, search, exportLedger, exportStamps. The TS 1.x types already declare number | bigint where this matters, so this is a runtime-shape fix rather than a type-surface change. Local conformance probe (cherry-picking beta's test against this fix): 3/7 verbs now pass (overhead, overheadTrim, ingest) vs the 1/7 beta reported pre-fix (overhead). The remaining 4 failures are the JSONL->SQLite read-side gap, which the sibling alpha-followup addresses. Refs #247, #354, #355.
willwashburn
added a commit
that referenced
this pull request
May 7, 2026
#357) The Rust SDK reads exclusively from `burn.sqlite` but never auto-built that mirror from a `ledger.jsonl` sibling. Freshly-ingested or JSONL-only ledgers (the cli-golden fixture, side-by-side TS/Rust tooling, users upgrading from 1.x) returned empty rows on read until something else populated the sqlite. The TS @relayburn/sdk@1.x didn't have this problem because it treats sqlite as a derived view rebuilt on demand. Lift the bootstrap algorithm from `tests/golden.rs::bootstrap_sqlite_from_jsonl` into the production SDK so reads always see the latest data. Algorithm — Option A, eager on `Ledger::open`. We snapshot the JSONL-vs- sqlite mtime BEFORE `Connection::open` creates `burn.sqlite` as a side effect (otherwise every fresh sqlite would look "current" relative to the JSONL and we'd skip the rebuild). If the JSONL is newer (or the sqlite is missing), wipe the derivable tables and replay the JSONL via the existing `writer::append_*` paths. Stamps + archive_state are first-party and preserved. Concurrency: SQLite WAL plus the configured `busy_timeout` serialize peer writers without a user-space lockfile — same design choice that let us drop `lock.ts` from the Rust port (see #259). Two concurrent opens both observing a stale sqlite would each attempt a rebuild; the second sees an already-warm sqlite and skips. Side effect: the cli-golden test helper no longer needs its own JSONL parser. Replace `bootstrap_sqlite_from_jsonl` (~150 lines of duplicated parse/replay logic) with a 30-line `reset_sqlite_for_fresh_bootstrap` that just deletes any prior sqlite so the SDK does the rebuild on the binary's first `Ledger::open`. Followup to PRs #354 (napi shape conformance) and #355 (conformance gate flip) which surfaced this gap. Verified manually against the cli-golden fixture: `RELAYBURN_HOME=/tmp/probe-ledger RELAYBURN_ARCHIVE=0 burn summary` on a JSONL-only ledger now returns 7 turns instead of 0.
# Conflicts: # packages/sdk-node/CHANGELOG.md
This was referenced May 7, 2026
willwashburn
added a commit
that referenced
this pull request
May 7, 2026
…/sdk@1.x (#358) The Rust SDK's `compare()` was pre-filtering the turn list by `opts.models` *before* computing `analyzedTurns` and the fidelity summary. The TS contract in `packages/sdk/index.js::compare()` does the opposite: `analyzedTurns = filteredTurns.length` is taken AFTER the fidelity gate but BEFORE the model allow-list, which is honored inside `buildCompareTable` (which also pre-seeds requested-but-absent models as all-empty columns). Net effect on the conformance fixture (`tests/fixtures/cli-golden`, seven turns spanning sonnet-4-6 / haiku-4-5 / gpt-5-codex / sonnet-4-6): calling `compare({ models: ['claude-sonnet-4-5', 'claude-opus-4-7'], minFidelity: 'partial' })` — neither requested model is present in the fixture — yielded `analyzedTurns: 0` and an all-zero fidelity summary on the Rust side (every turn dropped at the early model filter), versus `analyzedTurns: 7` plus a populated `byClass` / `byGranularity` / `missingCoverage` block on TS. The conformance gate at `packages/sdk-node/test/conformance.test.js` reduced this to a `deepStrictEqual` failure on the only verb that hits this code path. Fix: drop the early `requested_models` `retain` from `LedgerHandle::compare`. Provider filtering and fidelity summarization now run on the full slice the ledger query returned, matching the TS path; cell construction still honors the model allow-list via `AnalyzeCompareOptions::models`. The unused `compare_model_id` helper is removed. Test deltas: - `compare_metadata_counts_requested_models_only` was asserting the buggy behavior. Renamed to `compare_metadata_counts_all_matched_turns_pre_models_filter` and updated to the TS-parity expectations: `analyzed_turns == 3` / `summary.total == 3` for a 3-turn fixture even when the requested models only match 2 of them. - New `compare_reports_full_fidelity_summary_when_no_requested_model_appears` regression covering the exact conformance scenario (request two models that are absent from the ledger; metadata still describes the slice). Refs #240 (rust-port epic). Follows #354/#356/#357 and unblocks #355 (α-followup conformance gate). Local conformance now 7/7 green (summary, sessionCost, overhead, overheadTrim, hotspots, compare, ingest).
The aarch64-unknown-linux-gnu matrix leg cross-compiles on an x64 host (`runs-on: ubuntu-latest`), so the only `.node` artifact present at the end of the build step is the arm64 binary. The conformance step's `node --test` then runs under the runner's x64 interpreter, which cannot load the arm64 `.node` — `binding.cjs` resolves the binding by `process.arch`, so it tries `index.linux-x64-gnu.node` (or the matching `@relayburn/sdk-linux-x64-gnu` optionalDependency, which isn't installed in dev) and crashes the import before the test body's `t.skip` can fire. Result: every conformance test on this leg surfaces as a `MODULE_NOT_FOUND` test failure instead of a skip. Gate the "Build TS workspace" + "Conformance test" steps off this leg via `if: matrix.target != 'aarch64-unknown-linux-gnu'`. The cross-compile leg's job remains valuable — it validates the cross-build + artifact upload — but the conformance contract is already exercised by the three native legs (x64-darwin, arm64-darwin, x64-linux) that run on hosts matching their target arch.
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
1 participant
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
Three-blocker fix that makes the napi-rs conformance suite actually run
in CI against the now-merged shape-conformant bindings (alpha / #354). Each
blocker lands as its own commit:
napi buildwas emittingindex.<target>.nodeat the package root while the regenerated loaderat
src/binding.cjschecked__dirname(src/). Passsrcas thepositional
[destDir]so all three artifacts (.node,binding.cjs,binding.d.ts) co-locate. Update artifact upload glob +.gitignore.tests/fixtures/ledger/was neverseeded. The suite now spawns
tests/fixtures/cli-golden/scripts/build-ledger.mjs(the existinghand-curated, byte-deterministic builder) into a tmp dir on each run
and
cpSyncs the output into per-impl tmp homes. No committedbinary-ish fixture, no drift.
RELAYBURN_SDK_NAPI_BUILT=1in theworkflow + a
pnpm run buildstep so the TS 1.x SDK'sdist/-shapeddeps resolve before the conformance step runs.
Closes the beta slice of #247.
Alpha follow-ups surfaced by the now-running gate
The conformance step is now real and currently flags two classes of
divergence between TS @relayburn/sdk@1.x and the napi-rs binding (#354).
Beta does not fix these - both are alpha-scope:
u64/i64asBigInt(e.g.turnCount: 0n), TS 1.x uses plainNumber. Affectssummary.{turnCount,totalTokens},sessionCost.{turnCount,totalTokens},overheadTrim.summary.{filesAnalyzed,filesWithRecommendations},compare.{analyzedTurns, fidelity.excluded.*, totals.*.turns, minSample, fidelity.summary.*},hotspots(multiple),ingest.*.burn.sqlite, the seeded fixture is JSONL only. So napi reads returnempty rows while TS reads return populated data. Fixable by either
(a) auto-replaying JSONL->SQLite in the Rust
Ledger::openpath, or(b) pre-bootstrapping
burn.sqlitein the conformance fixture (therelayburn-cli/tests/golden.rs::bootstrap_sqlite_from_jsonlhelperalready does this for the CLI golden tests). Currently 6/7 verbs
diverge; only
overhead()passes (returns shape-equal empty resultson both sides because the project path doesn't exist).
PR gamma note (publish wiring)
The per-platform
package.jsonfiles inpackages/sdk-node/npm/<short>/declare
main: "relayburn-sdk.<target>.node", butnapi buildgenerates the file as
index.<target>.node(becausenapi.binaryNamein
package.jsonis not the field napi-rs reads - it looks fornapi.name, defaulting to"index"when missing). PR gamma will need toeither rename the artifact or set
napi.name = "relayburn-sdk".Test plan
pnpm install --ignore-workspaceinpackages/sdk-nodepnpm run build:napi:debugproducessrc/index.<target>.node+src/binding.cjs+src/binding.d.tsnode -e "require('./src/binding.cjs')"loads without throwingRELAYBURN_SDK_NAPI_BUILT=1 node --test test/conformance.test.jsruns all 7 verbs (failures are expected alpha follow-ups; see above)
node test/esbuild-smoke.test.jsstill passescargo build --workspace && pnpm -r run buildcleanRefs #247.