relayburn-sdk-node: shape conformance with TS @relayburn/sdk@1.x (#247c)#354
Merged
Conversation
Bring the napi-rs umbrella in line with packages/sdk/index.d.ts so
@relayburn/sdk@2.0 is a drop-in replacement for the 1.x TS SDK
modulo the documented number|bigint widening for u64 fields.
- SessionCostResult.totalUSD: pin js_name="totalUSD" so napi-rs
doesn't camelCase it to totalUsd.
- IngestOptions: { sessionId, harness, ledgerHome } matches TS 1.x
byte-for-byte; the binding routes to sdk::IngestRoots::default().
Drops the IngestRoots napi struct (callers needing root overrides
can drive the SDK directly until a typed escape hatch lands).
- top / minSample: widen from BigInt to plain u32. Both are small
counts; TS source-of-truth types them as number.
- ingest() return type: spell `Result<IngestReport, NapiError>` so
napi-derive emits Promise<IngestReport> instead of treating the
NapiResult alias as the literal return type.
- IngestHarness: new string_enum so callers branch on the canonical
wire values without stringly-typed literals.
- Umbrella facade (src/index.{js,cjs,d.ts}):
- Ledger as a JS-side class wrapping binding.ledgerOpen() so the
1.x Ledger.open() -> Promise<Ledger> signature lands without
forcing Mutex<LedgerHandle> plumbing on the Rust side.
- All read verbs re-exported as `async`, returning Promise<T> while
preserving the typed BurnErrorCode on e.code (verified end-to-end
via a real .node load with InvalidArgument).
- Adds 2.x extensions: search, exportLedger, exportStamps,
BurnErrorCode, OverheadFileKind, HotspotsGroupBy.
- Carries onLog?: (msg: string) => void on every read verb's
options. Currently dropped at the napi boundary (extra props on a
#[napi(object)] are ignored) — a forward-compat hook for when the
SDK grows the fallback-logging callback the TS SDK currently uses.
cargo build -p relayburn-sdk-node --release, cargo test -p
relayburn-sdk-node, pnpm run build:napi:debug, esbuild smoke test,
cargo build --workspace, and pnpm -r run build all clean. End-to-end
load test against a freshly built .node confirms typed e.code and
totalUSD casing survive the async wrapper.
Out of scope (other PRs):
- tests/fixtures/ledger seeding + RELAYBURN_SDK_NAPI_BUILT=1 flip
(PR β / #247-b).
- napi-build CI workflow (PR γ).
- binding.cjs loader path (.node lands at packages/sdk-node/ root,
loader looks under src/) — PR β.
Refs #247.
📝 WalkthroughWalkthroughNode SDK 2.x surface is updated to align with TypeScript 1.x contracts by restructuring N-API bindings for ingest options, wrapping async operations in both ESM and CommonJS facades, extending type definitions with new search/export APIs, and adjusting field types for numeric option parameters. ChangesNode SDK 2.x API Surface Alignment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/sdk-node/CHANGELOG.md (1)
10-18: ⚡ Quick winTrim this Unreleased entry to the user-visible delta.
This bullet currently mixes release notes, internal behavior notes, and issue bookkeeping. Please keep it to the surfaced API changes and drop details like the napi-boundary note and
(#247part c).✂️ Suggested wording
-- Shape conformance with TS `@relayburn/sdk@1.x`: `Ledger.open()` returns - a `Promise<Ledger>` instance, `sessionCost()` emits `totalUSD` - (screaming USD), every read verb is `async` (`Promise<T>`), - `IngestOptions` is `{ sessionId, harness, ledgerHome }`, `top` and - `minSample` accept plain `number`, and `onLog` callbacks are accepted - on every read verb's options (silently dropped at the napi boundary - until the SDK wires fallback logging). Adds `search`, `exportLedger`, - `exportStamps`, `BurnErrorCode`, `OverheadFileKind`, and - `HotspotsGroupBy` as 2.x extensions over the 1.x surface. (`#247` part c) +- Node SDK APIs now match the TS 1.x surface more closely: `Ledger.open()` + returns `Promise<Ledger>`, `sessionCost()` exposes `totalUSD`, read verbs + are async, `ingest()` accepts `{ sessionId, harness, ledgerHome }`, and + `overheadTrim()` / `compare()` accept numeric `top` / `minSample`. +- Added Node SDK exports for `search()`, `exportLedger()`, `exportStamps()`, + `BurnErrorCode`, `OverheadFileKind`, and `HotspotsGroupBy`.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 10 - 18, Trim the Unreleased bullet to only the user-visible API changes: list the surfaced signatures and behavior changes (e.g., Ledger.open() now returns Promise<Ledger>, sessionCost() emits totalUSD, all read verbs return Promise<T>, IngestOptions shape is { sessionId, harness, ledgerHome }, top and minSample accept plain number, read-verb options accept onLog callbacks) and the new API additions (search, exportLedger, exportStamps, BurnErrorCode, OverheadFileKind, HotspotsGroupBy); remove internal/backstory text such as the napi-boundary note, “screaming USD”, and the "(`#247` part c)" bookkeeping reference so the entry is concise and user-facing.
🤖 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 `@packages/sdk-node/CHANGELOG.md`:
- Around line 10-18: Trim the Unreleased bullet to only the user-visible API
changes: list the surfaced signatures and behavior changes (e.g., Ledger.open()
now returns Promise<Ledger>, sessionCost() emits totalUSD, all read verbs return
Promise<T>, IngestOptions shape is { sessionId, harness, ledgerHome }, top and
minSample accept plain number, read-verb options accept onLog callbacks) and the
new API additions (search, exportLedger, exportStamps, BurnErrorCode,
OverheadFileKind, HotspotsGroupBy); remove internal/backstory text such as the
napi-boundary note, “screaming USD”, and the "(`#247` part c)" bookkeeping
reference so the entry is concise and user-facing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 76e38dca-284a-4405-b376-88adefe2c564
📒 Files selected for processing (5)
crates/relayburn-sdk-node/src/lib.rspackages/sdk-node/CHANGELOG.mdpackages/sdk-node/src/index.cjspackages/sdk-node/src/index.d.tspackages/sdk-node/src/index.js
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.
6 tasks
This was referenced May 7, 2026
Merged
4 tasks
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).
willwashburn
added a commit
that referenced
this pull request
May 7, 2026
* relayburn-sdk-node: route napi build outputs into src/ so loader resolves
`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.
* relayburn-sdk-node: seed conformance fixture from cli-golden builder
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.
* relayburn-sdk-node: flip RELAYBURN_SDK_NAPI_BUILT=1 to gate the matrix
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.
* relayburn-sdk-node: hoist @relayburn/* so cli-golden seeder resolves
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.
* napi-build: skip conformance step on aarch64-linux cross-compile leg
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 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
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
PR α of the burn 2.0 cutover (epic #240, blocking #249). Brings the napi-rs umbrella
@relayburn/sdk@2.x(binding crate atcrates/relayburn-sdk-node+ facade atpackages/sdk-node/src/) into shape-parity with the TS 1.x SDK atpackages/sdk/index.d.ts, modulo the already-documentednumber | bigintwidening on u64 token-count fields.Punch-list
Ledgerclass — JS-side wrapper class insrc/index.{js,cjs}that awaitsbinding.ledgerOpen()and stashes the resolvedhome. No#[napi]class on the Rust side (would have forcedMutex<LedgerHandle>plumbing for no benefit).index.d.tsdeclaresclass Ledger { readonly home: string; static open(opts?: LedgerOpenOptions): Promise<Ledger> }.totalUSDcasing —#[napi(js_name = "totalUSD")]override onSessionCostResult.total_usd. Verified end-to-end against a freshly built.node:typeof sc.totalUSD === 'number', nototalUsdkey.onLog?: (msg) => voidon every read verb's options — declared in TS atindex.d.ts(already present from prior commits). The napi boundary silently drops it —#[napi(object)]'sfrom_napi_valuecodegen reads only declared fields, so extra props (like a JS function) ride through without a TypeError. Forward-compat hook for whenrelayburn-sdkgrows fallback-log threading;relayburn-sdkdoes not currently expose anOnLogCallbacktype for read verbs.IngestOptionsshape —{ sessionId, harness, ledgerHome }matches TS 1.x byte-for-byte. The binding routes tosdk::IngestRoots::default()and ignoressessionId/harness(TS 1.x'singest()likewise routes toingestAll()without filtering — confirmed atpackages/sdk/index.js:127-129). Drops theIngestRootsnapi struct.Promise<T>— every read verb is re-exported as anasyncfunction in the JS facade. The Rust side stays sync (returningResult<T, BurnError>) so the typedBurnErrorCodeone.codesurvives the wrapper. Verified:await m.compare({ minFidelity: 'bogus' })rejects withe.code === BurnErrorCode.InvalidArgument.ingest's return type spelledResult<IngestReport, NapiError>so napi-derive's syntacticResultdetection emitsPromise<IngestReport>instead of treating theNapiResultalias as the literal return.topandminSamplewidening —Option<u32>instead ofOption<BigInt>. Both are small counts (topis a recommendation cap,minSampleis a turn-count threshold) and TS types them asnumber;u32 -> u64is lossless on the way into the SDK.search,exportLedger,exportStamps,BurnErrorCode,OverheadFileKind,HotspotsGroupByas 2.x extensions toindex.{js,cjs,d.ts}.Ledgerre-exported via the wrapper class;ledgerOpenis internal-only.Out of scope
tests/fixtures/ledger/seeding + flippingRELAYBURN_SDK_NAPI_BUILT=1— PR β.binding.cjsloader-path bug (the regenerated dispatcher looks forindex.<target>.nodenext to__dirname=src/, butnapi builddrops the file atpackages/sdk-node/root) — also PR β..github/workflows/napi-build.yml— PR γ.Test plan
cargo build -p relayburn-sdk-node --releaseclean.cargo test -p relayburn-sdk-node— 10 tests pass (no regressions in the BIGINT_FIELDS membership / runtime invariant tests).cd packages/sdk-node && pnpm run build:napi:debugclean. Regeneratedbinding.d.tsshowsPromise<IngestReport>,totalUSD: number,top?: number,minSample?: number, and the newIngestHarnessenum.node --test test/esbuild-smoke.test.jsclean (esbuild bundles the umbrella facade with all the new exports).cargo build --workspaceclean.pnpm -r run buildclean..node:Ledger.open()returns a Promise,summary()'stotalTokensisbigint,sessionCost()'stotalUSDis correctly cased, typede.codesurvives the async wrapper,onLogcallbacks are silently accepted.Notes for PR β / γ
binding.cjs(which I left out of the diff per instructions) destructures the napi exports at the top:const { BurnErrorCode, summary, sessionCost, ..., IngestHarness, ingest, ledgerOpen } = nativeBinding. The hand-written stub onmaindoesmodule.exports = nativeBinding;and works for both shapes — so reverting binding.cjs in this PR doesn't break the umbrella facade. The CI matrix's regen overwrites it.index.darwin-arm64.nodeat the package root (next topackage.json), but the regenerated dispatcher doesexistsSync(join(__dirname, 'index.darwin-arm64.node'))where__dirnameissrc/. Worked around for this PR by copying the artifact intosrc/for the smoke test; PR β should align the build output and the loader.Refs #247.