feat(community): add bitsocial community export command - #65
Merged
Conversation
Exposes the pkc-js 0.0.41 community export API (pkcprotocol/pkc-js#100): - `bitsocial community export [ADDRESS]` triggers an export on the RPC server, streams progress to stderr, downloads the finished SQLite snapshot via GET /exports/<exportId>, verifies its sha256 against the export record (.partial file + atomic rename so a corrupted download never clobbers the destination), and prints the destination path. Flags: --name/--publicKey lookup, -o/--path, --includePrivateKey, --force, --quiet. Ctrl+C cancels the in-flight export server-side. - `bitsocial daemon --allowPrivateKeyExport` (negatable, default true, matching pkc-js) controls whether RPC clients may request exports that include the community signer's private key.
|
Warning Review limit reached
More reviews will be available in 13 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds ChangesCommunity Export Feature
Sequence Diagram(s)sequenceDiagram
participant CLI as Export Command
participant PKC as PKC RPC (WS)
participant HTTP as Snapshot HTTP Server
participant FS as Local File System
CLI->>PKC: connect(rpcUrl)
CLI->>PKC: community.export({includePrivateKey}, signal)
PKC-->>CLI: emits exportschange events (progress / error / url / sha256)
CLI->>PKC: wait until progress === 1 (or abort / error)
CLI->>HTTP: fetch(record.url, {signal})
HTTP-->>CLI: stream bytes
CLI->>FS: write to .partial + update SHA-256
CLI->>CLI: verify sha256 matches record.sha256 (if present)
CLI->>FS: rename .partial → destination
CLI->>PKC: destroy() on cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/cli/export.community.test.ts (1)
117-230: 🏗️ Heavy liftAdd a cancellation-path test for SIGINT-driven abort.
The new command behavior explicitly supports Ctrl+C cancellation, but this suite doesn’t assert that an in-flight export is aborted and exits cleanly when interrupted.
As per coding guidelines,
Add a test when you add a feature or fix a bug.🤖 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 `@test/cli/export.community.test.ts` around lines 117 - 230, Add a new test that simulates a SIGINT during an in-flight export: use runCliCommand to start `community export` (same pattern as other tests, e.g. destination path like "sigint.sqlite"), ensure the fake export server (exportFake / servedContent) is in a state that will block/stream so the command is mid-download, send a SIGINT to the child process (as runCliCommand exposes the spawned process or via the test helper), then assert the CLI exits with an error/aborted result, stderr contains an abort message (or "SIGINT"), no final destination file exists and no ".partial" file remains, and clean up by verifying destroyFake was called; follow the style of existing tests (expect(result.error).toBeDefined(), fs.stat(...) rejects, exportFake/destroyFake assertions).Source: Coding guidelines
🤖 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 `@src/cli/commands/community/export.ts`:
- Around line 185-189: The code currently calls fetch(record.url!) inside
_downloadAndVerify which trusts server-provided URLs; change it to construct the
download request against the trusted daemon origin instead of using record.url
directly. Locate the _downloadAndVerify method and replace the direct
fetch(record.url!) with building a URL using the daemon/RPC base origin (e.g.,
daemonBaseUrl or the configured RPC origin) plus the path/filename from
record.url (extract pathname/search/hash from record.url) and then fetch against
that constructed URL; ensure you validate or normalize the resulting URL and
preserve the same AbortSignal and headers when calling fetch.
- Around line 158-181: The promise returned by _runExport currently only
resolves/rejects via the exportschange listener (checkRecords) so if an external
abort/Ctrl+C occurs the promise can hang; modify _runExport to also listen to
the abort signal (or accept an AbortSignal) and on abort remove the
exportschange listener (community.removeListener("exportschange", checkRecords))
then reject the promise with an AbortError (or new Error('aborted')) so the
caller doesn't wait indefinitely; ensure exportId, checkRecords and the existing
this._printProgress logic remain unchanged while adding the abort cleanup and
rejection path.
---
Nitpick comments:
In `@test/cli/export.community.test.ts`:
- Around line 117-230: Add a new test that simulates a SIGINT during an
in-flight export: use runCliCommand to start `community export` (same pattern as
other tests, e.g. destination path like "sigint.sqlite"), ensure the fake export
server (exportFake / servedContent) is in a state that will block/stream so the
command is mid-download, send a SIGINT to the child process (as runCliCommand
exposes the spawned process or via the test helper), then assert the CLI exits
with an error/aborted result, stderr contains an abort message (or "SIGINT"), no
final destination file exists and no ".partial" file remains, and clean up by
verifying destroyFake was called; follow the style of existing tests
(expect(result.error).toBeDefined(), fs.stat(...) rejects,
exportFake/destroyFake assertions).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbfb0072-5766-4b33-bed8-22d418c3ee40
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.jsonsrc/cli/commands/community/export.tssrc/cli/commands/daemon.tssrc/webui/daemon-server.tstest/cli/export.community.test.ts
- Reject _runExport() on abort instead of waiting for the server's terminal ERR_EXPORT_CANCELLED record, which never arrives if the daemon died or the connection dropped. pkc-js's own abort listener (registered first inside community.export()) still dispatches cancelExport() to the server. - Validate the export record's download URL against the RPC server's http origin (ws[s]:// swapped to http[s]://, mirroring pkc-js's rpcHttpOrigin) and require a /exports/ path, so a misconfigured or compromised RPC server can't use the CLI to fetch arbitrary URLs. - Add tests for both: wrong-origin URL is refused with no request made, and an aborted export with a silent daemon rejects instead of hanging.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/commands/community/export.ts (1)
232-243:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed when the terminal export record omits
sha256.
record.sha256is optional here, so a terminal record without a checksum still gets written to disk and the CLI printsVerified sha256 (undefined). That silently drops the integrity guarantee this command advertises. Reject the export when the terminal record has no hash, and add a regression test for that path.Suggested fix
const downloadedSha256 = hash.digest("hex"); - if (record.sha256 && downloadedSha256 !== record.sha256) { + if (!record.sha256) { + throw new Error(`Export ${record.exportId} finished without a sha256 checksum`); + } + if (downloadedSha256 !== record.sha256) { throw new Error( `sha256 mismatch for downloaded export: expected ${record.sha256} but downloaded file hashes to ${downloadedSha256}` ); } await fs.rename(partialPath, destPath);As per coding guidelines,
Add a test when you add a feature or fix a bug.🤖 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 `@src/cli/commands/community/export.ts` around lines 232 - 243, The code currently allows writing a terminal export record with no checksum (record.sha256) and prints "Verified sha256 (undefined)"; change the logic in the download/verify block (the code that computes downloadedSha256, compares to record.sha256, renames partialPath to destPath, and calls this._printProgress) to explicitly reject and throw an error when record.sha256 is missing before attempting verification or file move, ensure partialPath is removed on error as already done, and add a regression test that simulates an export record with record.sha256 === undefined to assert the CLI fails rather than writing the file.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/cli/commands/community/export.ts`:
- Around line 232-243: The code currently allows writing a terminal export
record with no checksum (record.sha256) and prints "Verified sha256
(undefined)"; change the logic in the download/verify block (the code that
computes downloadedSha256, compares to record.sha256, renames partialPath to
destPath, and calls this._printProgress) to explicitly reject and throw an error
when record.sha256 is missing before attempting verification or file move,
ensure partialPath is removed on error as already done, and add a regression
test that simulates an export record with record.sha256 === undefined to assert
the CLI fails rather than writing the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a5f0220-5c64-4134-b2ec-6015e87c1444
📒 Files selected for processing (2)
src/cli/commands/community/export.tstest/cli/export.community.test.ts
5 tasks
…dress>_<datetime>.sqlite
The default -o/--path was ./<address>.sqlite (cwd-relative). Snapshots now
land in the exports directory inside the data path, with an ISO 8601
datetime (':' -> '-', matching the daemon log filename convention) so
repeated exports never collide and sort chronologically. -o still
overrides entirely.
Also closes the export command's test coverage gaps found in review:
- HTTP non-200 download response and mid-stream connection drop
(no destination file or .partial left behind)
- finished record with no download URL
- same-origin-but-non-/exports download URL refused (second half of the
URL validation), wss:// RPC URL -> https:// expected download origin
- record without sha256 skips verification (pins current behavior)
- default destination directory + filename pattern (XDG_DATA_HOME
pointed at a temp dir so tests never write into a real data path)
- intermediate destination directories created; destination-is-a-directory
with --force fails cleanly
- address + --name combined into a single lookup
- non-quiet mode: progress on stderr, stdout only the destination path;
--quiet asserted to suppress progress
- pkc.destroy() asserted on all failure paths (connection-leak coverage)
- _runExport unit tests: terminal success/error record already present
before listener attach, pre-aborted signal, records of other exportIds
ignored
…wnloads
Live `community export` against a real daemon always failed with HTTP 404
even though the export completed server-side. The daemon serves everything
on one port: startDaemonServer creates the http.Server via express().listen()
and hands it to PKCWsServer, which attaches a second request listener to
stream GET /exports/<exportId>. Both listeners get every request; express
finds no matching route and 404s synchronously while pkc-js's handler is
still doing async fs work, so express wins the race.
Fix: an express middleware that swallows /exports/<uuid> paths (responds
nothing, calls nothing) so pkc-js's listener owns the response. All other
/exports/* paths fall through to express's 404 because pkc-js ignores them
on a caller-supplied server and the request would otherwise hang.
The middleware is deliberately NOT mounted at app.use("/exports", ...):
a mounted middleware strips the mount prefix from the shared req.url while
the request is held, so pkc-js's listener would no longer recognize the
URL and the download would hang instead.
Regression test mimics pkc-js's listener answering after a short delay
(a synchronous fake wins the race and hides the bug); verified it fails
with 404 against the unfixed build. Also adds plumbing tests asserting
daemon --allowPrivateKeyExport reaches PKCWsServer verbatim
(true/false/omitted).
Member
Author
|
Two new commits since the CodeRabbit review: 52c5ac1 feat(community): default export destination to
0e40b4f fix(daemon): stop express's catch-all 404 from clobbering /exports downloads
Verified live end-to-end (export → download → sha256 verify → Analysis details in #64. |
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.
Closes #64
Summary
Upgrades
@pkcprotocol/pkc-js0.0.40 → 0.0.41 and exposes its new community export API (pkcprotocol/pkc-js#100) in the CLI.bitsocial community export [ADDRESS]Triggers an export on the RPC server (daemon), streams progress to stderr, downloads the finished SQLite snapshot via
GET /exports/<exportId>, and verifies its sha256 against the export record before reporting success.ADDRESS/--name/--publicKeycommunity get-o, --path./<address>.sqlite)--includePrivateKey--force--quietImplementation details:
<dest>.partialand atomically renames after the sha256 check, so a corrupted/interrupted download never clobbers the destination.AbortSignal→cancelExport()on the server; a second Ctrl+C force-exits.bitsocial daemon --allowPrivateKeyExportNegatable boolean (default
true, matching the pkc-js default), plumbed throughstartDaemonServer→PKCWsServer. With--no-allowPrivateKeyExport, RPC clients requesting--includePrivateKeygetERR_PRIVATE_KEY_EXPORT_NOT_ALLOWED.Tests
11 new tests in
test/cli/export.community.test.ts:--name,--publicKey), missing-identifier error--includePrivateKeypass-through tocommunity.export()--force(existing file untouched, export never starts) and overwrite with--force.partialbehindERR_PRIVATE_KEY_EXPORT_NOT_ALLOWED) fails the commandFull
npm run test:clisuite: 220 passed, 1 skipped (pre-existing), 29 files.Summary by CodeRabbit
New Features
Tests
Chores
@pkcprotocol/pkc-jsto 0.0.41