Remove onLog from @relayburn/sdk@2.x option types (#374)#384
Merged
Conversation
The 1.x SDK accepted `onLog` to surface archive-fallback messages from the JS-side fallback path. The 2.x Node facade is SQLite-native and silently dropped the callback at the napi boundary — there is no fallback to log. Drop the option from `summary`, `sessionCost`, `overhead`, `overheadTrim`, `hotspots`, and `compare` rather than wire a hook for events that don't fire. Adds a regression test asserting the d.ts no longer declares `onLog` on any option type, plus a runtime tolerance test that the verbs still accept a stray `onLog` property (so embedders mid-migration from 1.x don't blow up at runtime when their TS shape lags).
📝 WalkthroughWalkthroughThis PR removes the ChangesRemove onLog Callback from SDK Option Types
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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)
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
🤖 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 `@packages/sdk-node/CHANGELOG.md`:
- 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.
In `@packages/sdk-node/test/no-onlog.test.js`:
- Around line 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).
🪄 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: b7fea610-05e5-4057-8478-c40a96ac8e91
📒 Files selected for processing (3)
packages/sdk-node/CHANGELOG.mdpackages/sdk-node/src/index.d.tspackages/sdk-node/test/no-onlog.test.js
|
|
||
| ### 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) |
There was a problem hiding this comment.
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.
Comment on lines
+35
to
+56
| 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)); | ||
| }); |
There was a problem hiding this comment.
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).
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
2 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.
Closes #374.
Summary
The 1.x SDK accepted
onLogto surface archive-fallback messages from the JS-side fallback path. The 2.x Node facade is SQLite-native and silently dropped the callback at the napi boundary — there is no fallback to log. Per the issue's first acceptance option, the option is removed rather than wiring a structured diagnostic hook for events that don't fire.onLog?:fromSummaryOptions,SessionCostOptions,OverheadOptions,HotspotsOptions, andCompareOptionsinpackages/sdk-node/src/index.d.ts.onLogis intentionally absent (replaces the prior reference to it as a measured TS-vs-napi gap).[Unreleased] / Breaking Changesentry topackages/sdk-node/CHANGELOG.md.The 1.x SDK (
packages/sdk) and its consumers in@relayburn/cli/@relayburn/mcpare unchanged — they still depend on@relayburn/sdk(workspace) and use the working 1.xonLog.Test plan
pnpm run build(TS workspace builds clean)pnpm run test(875 tests pass)packages/sdk-node/test/no-onlog.test.js:src/index.d.tsno longer declaresonLog?:on any option type (regression guard)onLogproperty at runtime (skips when the napi binding isn't built, like the conformance suite — picks up automatically in CI underRELAYBURN_SDK_NAPI_BUILT=1)Generated by Claude Code