package sqlite as zip and extract at build time#26
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR migrates SQLite amalgamation handling from pre-extracted tar archives to build-time ZIP extraction. A new Node.js utility extracts ZIP files with idempotency guarantees, the build system is refactored to extract during compilation, and the version bump script is updated to manage zip artifacts instead of extracted directories. ChangesSQLite Build Pipeline: ZIP-Based Extraction Migration
Sequence DiagramsequenceDiagram
participant Build as Build System
participant ActionBefore as action_before_build
participant Extractor as deps/extract.js
participant IntDir as SHARED_INTERMEDIATE_DIR
participant Sqlite3 as sqlite3 target
Build->>ActionBefore: trigger before build
ActionBefore->>Extractor: run with zip path
Extractor->>IntDir: write sqlite3.c + headers
Sqlite3->>IntDir: compile from extracted sources
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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. 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 |
![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)
memory-bank/project-overview.md (1)
134-134:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove stale
tardependency from runtime dependencies list.According to
memory-bank/decisionLog.mdlines 45 and 55, thetarnpm dependency was removed as part of the ZIP migration. This line should be deleted to reflect the current dependencies.📝 Proposed fix
### Runtime - `node-addon-api`: ^8.7.0 - C++ NAPI wrapper - `node-gyp-build`: ^4.8.4 - Native addon binary loader (resolves prebuilt or falls back to source build) -- `tar`: ^7.5.13 - Tarball handling🤖 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 `@memory-bank/project-overview.md` at line 134, Remove the stale runtime dependency entry "`tar`: ^7.5.13 - Tarball handling" from memory-bank/project-overview.md to reflect the ZIP migration decision; locate the line containing that exact dependency string and delete it so the dependencies list matches the decisionLog (see references to the removed tar dependency).
♻️ Duplicate comments (1)
memory-bank/progress.md (1)
98-98:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest count inconsistency across documentation files.
This line states "281 tests passing" but
memory-bank/activeContext.mdline 7 reports 339 total tests (277 CJS + 62 ESM). These counts should be consistent across all documentation files. This comment is linked to the verification script in the activeContext.md review.🤖 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 `@memory-bank/progress.md` at line 98, The test count in progress.md ("281 tests passing") is inconsistent with activeContext.md's reported totals (339 tests: 277 CJS + 62 ESM); update the test summary in progress.md to match the canonical counts (339 total with the breakdown) or re-run the test-summary/generation script referenced by activeContext.md to regenerate both docs so they stay in sync; ensure the updated line in progress.md reflects the same total and breakdown as activeContext.md and any verification script output.
🤖 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 `@deps/extract.js`:
- Around line 57-63: The current write logic uses outFileName directly and can
be exploited for Zip Slip; change the path handling so you compute outPath via
path.join(destDir, outFileName), then resolve and validate it is inside destDir
before writing: use path.resolve(destDir) and path.resolve(outPath) (or
path.relative) to ensure resolvedOutPath starts with resolvedDestDir + path.sep
(reject absolute outFileName or any that resolve outside destDir), skip or error
on unsafe entries, and only then create directories and call fs.writeFileSync on
the validated outPath; refer to symbols outFileName, outPath, destDir in
extract.js.
In `@tools/bin/bump-sqlite.sh`:
- Around line 427-432: The dry-run cleanup messages and the actual cleanup
target are inconsistent: when DRY_RUN is true the script logs removing
deps/${old_dir} but the real cleanup targets ${PROJECT_ROOT}/build/${old_dir};
update the dry-run messages and the actual removal to use the same path used by
the build outputs (prefer deps/${old_dir} to match the logged messages and where
extracted zips are placed), i.e. change the log_dry and rm/cleanup calls that
reference ${PROJECT_ROOT}/build/${old_dir} to use deps/${old_dir}, and apply the
same fix to the second cleanup block (the one analogous to lines 550-553);
ensure variables DRY_RUN, log_dry, new_zip, old_zip, and old_dir are used
consistently.
---
Outside diff comments:
In `@memory-bank/project-overview.md`:
- Line 134: Remove the stale runtime dependency entry "`tar`: ^7.5.13 - Tarball
handling" from memory-bank/project-overview.md to reflect the ZIP migration
decision; locate the line containing that exact dependency string and delete it
so the dependencies list matches the decisionLog (see references to the removed
tar dependency).
---
Duplicate comments:
In `@memory-bank/progress.md`:
- Line 98: The test count in progress.md ("281 tests passing") is inconsistent
with activeContext.md's reported totals (339 tests: 277 CJS + 62 ESM); update
the test summary in progress.md to match the canonical counts (339 total with
the breakdown) or re-run the test-summary/generation script referenced by
activeContext.md to regenerate both docs so they stay in sync; ensure the
updated line in progress.md reflects the same total and breakdown as
activeContext.md and any verification script output.
🪄 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
Run ID: 3942fafa-5327-4636-a0b2-0e2ac4f10a77
⛔ Files ignored due to path filters (1)
deps/sqlite-amalgamation-3530100.zipis excluded by!**/*.zip
📒 Files selected for processing (12)
deps/extract.jsdeps/sqlite-amalgamation-3530100/shell.cdeps/sqlite-amalgamation-3530100/sqlite3.cdeps/sqlite-amalgamation-3530100/sqlite3.hdeps/sqlite-amalgamation-3530100/sqlite3ext.hdeps/sqlite3.gypmemory-bank/activeContext.mdmemory-bank/build-system.mdmemory-bank/decisionLog.mdmemory-bank/progress.mdmemory-bank/project-overview.mdtools/bin/bump-sqlite.sh
💤 Files with no reviewable changes (1)
- deps/sqlite-amalgamation-3530100/sqlite3ext.h
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 8 8
Lines 344 344
=========================================
Hits 344 344 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit