Conversation
This reverts commit 7a1e985.
This reverts commit 008327d.
when copying from null pointer as pointed out here: TryGhost#1832
|
Caution Review failedPull request was closed or merged during review WalkthroughThis pull request performs a comprehensive fork takeover and modernization of a Node.js SQLite3 binding library. Changes include rebranding the package from Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| run: yarn install --frozen-lockfile --ignore-scripts | ||
|
|
||
| - name: Publish to npm | ||
| run: npm publish |
There was a problem hiding this comment.
CI publish job likely missing npm authentication configuration
Medium Severity
The publish-npm job uses actions/setup-node with registry-url, which creates an .npmrc referencing ${NODE_AUTH_TOKEN}, but no NODE_AUTH_TOKEN environment variable is provided on the npm publish step. While id-token: write suggests OIDC trusted publishing was intended, the actions/setup-node registry-url option is known to interfere with OIDC by injecting an unresolved token reference. The npm publish call likely needs either a --provenance flag or NODE_AUTH_TOKEN set explicitly.
| await promisifyRun(db, 'COMMIT'); | ||
| resolve(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Async callback breaks serialize mode in benchmarks
Low Severity
Passing an async callback to db.serialize() breaks serialization. The native Serialize implementation calls the callback synchronously and restores db->serialize to its prior value immediately after the callback returns. An async function returns a Promise at its first await, so serialize mode is restored to false before the remaining queued operations (the parallelize block, COMMIT) execute. This means those operations run in parallel mode, defeating the purpose of serialize and producing unreliable benchmark results.
Additional Locations (1)
2 participants
when copying from null pointer
as pointed out here:
#1832
Note
Medium Risk
Medium risk because it touches native BLOB handling and significantly changes packaging/release automation (scoped npm name, CI publishing, bundled SQLite version), which could affect consumers and distribution.
Overview
Fixes undefined behavior in native statement BLOB binding by skipping
memcpywhenlen == 0, and adds a regression test to ensure empty BLOBs round-trip as zero-lengthBuffers.Updates the project for the forked distribution: renames the npm package to
@homeofthings/sqlite3(version6.1.0), updates docs/links, adjusts the bundled SQLite version incommon-sqlite.gypi, and adds new developer docs plus atinybench-based benchmark suite.Modernizes repo hygiene and automation by adding
.editorconfig/.gitattributes, tightening ESLint setup and introducingyarn lint, and expanding GitHub Actions CI with version-tag verification, a dedicated lint job, frozen installs, and an npm publish step on tags.Written by Cursor Bugbot for commit 70e8673. This will update automatically on new commits. Configure here.