Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/sdk-node/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## [Unreleased]

### Breaking Changes

- Removed the `onLog` option from `summary`, `sessionCost`, `overhead`, `overheadTrim`, `hotspots`, and `compare` option types. The 2.x stack is SQLite-native and has no archive-fallback path to surface, so the callback was already a no-op at the napi boundary. (#374)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Make the Unreleased entry concise and impact-first.

This line includes architecture backstory and an issue link; keep it to API impact and migration effect only.

Suggested edit
-- Removed the `onLog` option from `summary`, `sessionCost`, `overhead`, `overheadTrim`, `hotspots`, and `compare` option types. The 2.x stack is SQLite-native and has no archive-fallback path to surface, so the callback was already a no-op at the napi boundary. (`#374`)
+- Removed `onLog` from 2.x option types for `summary`, `sessionCost`, `overhead`, `overheadTrim`, `hotspots`, and `compare`; TypeScript callers should remove this field (runtime continues to ignore extra properties).

As per coding guidelines, “Curate [Unreleased] sections … with concise, impact-first entries … drop issue/PR links, internal notes, backstory…”.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- Removed the `onLog` option from `summary`, `sessionCost`, `overhead`, `overheadTrim`, `hotspots`, and `compare` option types. The 2.x stack is SQLite-native and has no archive-fallback path to surface, so the callback was already a no-op at the napi boundary. (#374)
- Removed `onLog` from 2.x option types for `summary`, `sessionCost`, `overhead`, `overheadTrim`, `hotspots`, and `compare`; TypeScript callers should remove this field (runtime continues to ignore extra properties).
🧰 Tools
🪛 LanguageTool

[grammar] ~7-~7: Ensure spelling is correct
Context: ...the callback was already a no-op at the napi boundary. (#374) ## [2.1.0] - 2026-05-...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🤖 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` at line 7, Edit the Unreleased changelog
entry to a concise, impact-first sentence that states the API change and
migration effect only: remove the architecture backstory and PR/issue link and
keep just that the `onLog` option has been removed from the `summary`,
`sessionCost`, `overhead`, `overheadTrim`, `hotspots`, and `compare` option
types (and note any direct migration instruction if needed). Ensure the line
replaces the current verbose entry so it reads as a short, user-facing change
about the breaking/API update.


## [2.1.0] - 2026-05-07

### Added
Expand Down
13 changes: 4 additions & 9 deletions packages/sdk-node/src/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ export interface SummaryOptions {
/** ISO timestamp (e.g. `2026-04-01T00:00:00Z`) or relative range (`24h`, `7d`, `4w`, `2m`). */
since?: string;
ledgerHome?: string;
/** Optional logger invoked when the SQLite archive read fails and the SDK falls back to a full ledger walk. */
onLog?: (msg: string) => void;
}
export declare function summary(opts?: SummaryOptions): Promise<{
totalTokens: number | bigint;
Expand All @@ -64,7 +62,6 @@ export interface SessionCostOptions {
/** Session id to total. Omit for `{ note: 'no session id provided' }`. */
session?: string;
ledgerHome?: string;
onLog?: (msg: string) => void;
}
export interface SessionCostResult {
sessionId: string | null;
Expand All @@ -85,7 +82,6 @@ export interface OverheadOptions {
since?: string;
kind?: OverheadFileKind;
ledgerHome?: string;
onLog?: (msg: string) => void;
}

export interface OverheadSection {
Expand Down Expand Up @@ -183,7 +179,6 @@ export interface HotspotsOptions {
groupBy?: HotspotsGroupBy;
patterns?: string[];
ledgerHome?: string;
onLog?: (msg: string) => void;
}

export interface HotspotsFileRow {
Expand Down Expand Up @@ -333,7 +328,6 @@ export interface CompareOptions {
minSample?: number;
minFidelity?: FidelityClass;
ledgerHome?: string;
onLog?: (msg: string) => void;
}

export interface CompareResult {
Expand All @@ -356,9 +350,10 @@ export declare function compare(opts: CompareOptions): Promise<CompareResult>
// ---------------------------------------------------------------------------
// 2.x extensions — surfaces present in `relayburn-sdk` (the Rust crate)
// but not in the TS 1.x `packages/sdk/index.d.ts`. Pre-1.0 widening per
// the SDK shape rule; embedders that pinned to 1.x won't see these names
// (the missing `onLog` etc. plus the absence of these is the only TS-vs-
// napi gap the conformance gate measures).
// the SDK shape rule; embedders that pinned to 1.x won't see these names.
// The 1.x `onLog` callback is intentionally omitted: it surfaced the
// archive-fallback path that no longer exists in the SQLite-native 2.x
// stack (see issue #374), so there is nothing to log.
// ---------------------------------------------------------------------------

export interface SearchQueryOptions {
Expand Down
56 changes: 56 additions & 0 deletions packages/sdk-node/test/no-onlog.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Regression guard for issue #374. The 2.x Node facade is SQLite-native and
// has no archive-fallback path to surface, so `onLog` was always a no-op at
// the napi boundary. We removed it from the public option types; this test
// fails if it ever sneaks back in (verb-by-verb), so we don't accidentally
// re-document a callback that doesn't fire.
//
// Also asserts that calling each verb with a stray `onLog` property is
// tolerated at runtime — JS allows extra props on options bags, and we don't
// want to break embedders mid-migration from 1.x just because they haven't
// dropped the field yet.

import { test } from 'node:test';
import assert from 'node:assert/strict';
import { readFileSync } from 'node:fs';
import { dirname, join, resolve } from 'node:path';
import { fileURLToPath } from 'node:url';

const __dirname = dirname(fileURLToPath(import.meta.url));
const DTS_PATH = resolve(__dirname, '..', 'src', 'index.d.ts');

test('public option types no longer declare onLog (#374)', () => {
const dts = readFileSync(DTS_PATH, 'utf8');
const offending = dts
.split('\n')
.map((line, i) => ({ line, n: i + 1 }))
.filter(({ line }) => /^\s*onLog\??\s*:/.test(line));
assert.deepStrictEqual(
offending,
[],
`onLog reappeared in src/index.d.ts at:\n` +
offending.map(({ n, line }) => ` ${n}: ${line.trim()}`).join('\n'),
);
});

test('verbs tolerate a stray onLog property at runtime', async (t) => {
if (process.env.RELAYBURN_SDK_NAPI_BUILT !== '1') {
t.skip('napi-rs binding not built — set RELAYBURN_SDK_NAPI_BUILT=1');
return;
}
let sdk;
try {
sdk = await import(join(__dirname, '..', 'src', 'index.js'));
} catch (err) {
if (/native binding not found/i.test(String(err && err.message))) {
t.skip('napi-rs binding load failed — build artifact missing');
return;
}
throw err;
}
// Cast through any-shape to bypass the now-stricter option types: the
// contract under test is the runtime forgiveness, not the TS shape.
const stray = { onLog: () => {} };
await assert.doesNotReject(() => sdk.summary(stray));
await assert.doesNotReject(() => sdk.sessionCost(stray));
await assert.doesNotReject(() => sdk.hotspots(stray));
});
Comment on lines +35 to +56

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Runtime tolerance test does not cover all affected 2.x verbs.

The test claims “each verb” but currently checks only summary, sessionCost, and hotspots. Since onLog was removed from six option types, migration safety should also cover overhead, overheadTrim, and compare.

Suggested expansion
   await assert.doesNotReject(() => sdk.summary(stray));
   await assert.doesNotReject(() => sdk.sessionCost(stray));
+  await assert.doesNotReject(() => sdk.overhead(stray));
+  await assert.doesNotReject(() => sdk.overheadTrim(stray));
   await assert.doesNotReject(() => sdk.hotspots(stray));
+  await assert.doesNotReject(() =>
+    sdk.compare({ models: ['model-a', 'model-b'], onLog: () => {} }),
+  );
🤖 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/test/no-onlog.test.js` around lines 35 - 56, The test only
checks runtime tolerance for sdk.summary, sdk.sessionCost, and sdk.hotspots but
misses the other verbs that had onLog removed; update the test to also call and
assert no rejection for sdk.overhead, sdk.overheadTrim, and sdk.compare using
the same stray = { onLog: () => {} } input so the runtime forgiveness is
validated for all affected functions (look for the test function named 'verbs
tolerate a stray onLog property at runtime' and add the three additional await
assert.doesNotReject calls for overhead, overheadTrim, and compare).

Loading