feat(cli): deepnote sync — ZIP-per-notebook export + inverse-ZIP push (GRN-5421) - #451
Conversation
…cal filesystem (GRN-5421)
…etes Aligns the CLI with the server sync API's latest review rounds: - pushes now send baseContentHash (the manifest's sha256 of the base export bytes) alongside baseModifiedAt - the timestamp only sees structural changes, so editor block edits made after the base export were previously overwritten without a 409; the content hash closes that lost-update gap (and applies even when metadata.modifiedAt could not be read, where the timestamp check silently disarmed) - the import API now accepts a no-notebook document (a no-op, or a delete-every-notebook under deleteMissingNotebooks); since an empty local file is more often an accident than an intent to wipe the project, the destructive combination is confirmed like a conflict (ask prompts, override proceeds, skip - and no-TTY ask - skips) - capture project.modifiedAt from the import response (the post-import fingerprint, usable to chain pushes) and document the import's 413 and naming-rule 422 responses
The merged export endpoint (deepnote-internal #20596) returns a ZIP of one
`.deepnote` document per notebook — not the single project document the sync
CLI was built against. Rework the pull side to match, and defer push.
- @deepnote/cloud: `exportProject` unzips the export into `{ filename, content }[]`
(adds `fflate`). `folder.path` is now `{ id, name }` segments plus
`isPathComplete`, matching the shipped projects API.
- sync-paths: plan a directory per project (one `.deepnote` per notebook),
not a single file; `.files/` lives inside it.
- sync-manifest: track `dir` + the notebook filenames + a canonical content
hash computed over the exploded documents. The documents are deterministic;
the ZIP container is not, so it is never hashed.
- sync command: pull writes one file per notebook and removes files for
notebooks deleted in the cloud; rename moves the whole directory; conflict
detection is unchanged. Push is detected but DEFERRED: `POST
/v2/projects/{id}/import` is not yet deployed and the `baseContentHash`
contract over a multi-notebook document is unsettled, so a local-only edit is
reported as `push-deferred` and left untouched.
Docs and `--help` updated for the directory layout and the deferred push.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds cloud APIs for project listing, details, ZIP export/import, and working-directory file operations. Adds validated sync-manifest storage and deterministic local path planning. Adds the bidirectional Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant SyncWorkspace
participant CloudAPI
participant LocalFiles
User->>CLI: Run deepnote sync
CLI->>SyncWorkspace: Pass options and directory
SyncWorkspace->>CloudAPI: List, export, import, and transfer files
CloudAPI-->>SyncWorkspace: Return project and file results
SyncWorkspace->>LocalFiles: Compare hashes and apply changes
LocalFiles-->>SyncWorkspace: Return sync outcomes
SyncWorkspace-->>CLI: Return aggregate result
CLI-->>User: Print summary or JSON
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
packages/cloud/src/sync.test.ts (1)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSlice the view before exposing
.buffer.
.bufferreturns the whole backing store and ignoresbyteOffsetandbyteLength. AUint8Arraythat views part of a larger buffer then yields extra bytes to the code under test. Copy the view instead.♻️ Proposed change
- arrayBuffer: () => Promise.resolve((init.bytes ?? new Uint8Array()).buffer), + arrayBuffer: () => Promise.resolve((init.bytes ?? new Uint8Array()).slice().buffer),🤖 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/cloud/src/sync.test.ts` at line 29, Update the arrayBuffer implementation in the test response fixture to copy the Uint8Array view before returning its buffer, preserving the view’s byteOffset and byteLength rather than exposing the entire backing store. Keep the existing empty-byte fallback behavior.packages/cli/src/utils/sync-manifest.ts (1)
113-114: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWrite the manifest atomically.
fs.writeFiletruncates the target first. If the process stops mid-write, the manifest is left partial.loadSyncManifestthen throws and the user must delete the file, which discards all sync state. Write to a sibling temp file and rename it.♻️ Proposed change
const content = `${JSON.stringify({ version: manifest.version, projects: sortedProjects }, null, 2)}\n` - await fs.writeFile(path.join(rootDir, SYNC_MANIFEST_FILENAME), content, 'utf-8') + const target = path.join(rootDir, SYNC_MANIFEST_FILENAME) + const temp = `${target}.tmp-${process.pid}` + await fs.writeFile(temp, content, 'utf-8') + await fs.rename(temp, target)🤖 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/cli/src/utils/sync-manifest.ts` around lines 113 - 114, Update the manifest-writing flow in the sync manifest utility to write content to a sibling temporary file first, then rename that temporary file to the path used by SYNC_MANIFEST_FILENAME. Ensure the rename occurs only after the write completes and clean up or safely handle the temporary file on failure.packages/cli/src/utils/sync-manifest.test.ts (1)
74-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
emptySyncManifesttest out of thesaveSyncManifestblock.This test exercises
emptySyncManifest, notsaveSyncManifest. Put it in its owndescribe()block.As per coding guidelines: "organize related tests with
describe()and clear test names".🤖 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/cli/src/utils/sync-manifest.test.ts` around lines 74 - 76, Move the test for emptySyncManifest out of the saveSyncManifest describe block and place it in a dedicated describe block for emptySyncManifest, preserving its existing assertion and clear test name.Source: Coding guidelines
packages/cli/src/utils/sync-paths.test.ts (1)
4-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd truncation coverage.
No test drives a name longer than
MAX_SEGMENT_LENGTH. Add cases that assert the exact segment for a 200-character name, including one whose 120th character is.or a space.As per coding guidelines: "Write comprehensive tests covering new features, edge cases, error handling, special characters, and exact output".
🤖 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/cli/src/utils/sync-paths.test.ts` around lines 4 - 28, Extend the sanitizePathSegment tests with names exceeding MAX_SEGMENT_LENGTH, including a 200-character input and a 200-character input whose 120th character is a dot or space. Assert the exact truncated and sanitized segment output, including the required final-character behavior after truncation.Source: Coding guidelines
packages/cli/src/commands/sync.test.ts (1)
196-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for an unsafe notebook filename in the export archive.
The
--all-filestest covers the hostile inventory path (../escape.txt). The notebook guard inwriteProjectNotebooks(packages/cli/src/commands/sync.tslines 264-267) has no equivalent test. A ZIP entry named../escape.deepnoteshould be skipped and nothing should be written outside the project directory.As per coding guidelines: "Write comprehensive tests covering new features, edge cases, error handling, special characters".
🤖 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/cli/src/commands/sync.test.ts` around lines 196 - 221, Add a test covering an unsafe notebook filename such as ../escape.deepnote in the export archive, targeting writeProjectNotebooks. Verify the entry is skipped and no file is created outside the project directory, while preserving normal notebook export behavior.Source: Coding guidelines
packages/cli/src/commands/sync.ts (1)
556-576: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
namecarries a directory, not a project name.For projects that left the cloud, the local name is unknown, so
name: record.dirfills the field with a path. The-o jsonconsumer then sees a path innameand the same value inpath. Consider omittingnameor storing the last known project name in the manifest.🤖 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/cli/src/commands/sync.ts` around lines 556 - 576, The missing-in-cloud outcome in the manifest project loop should not populate name from record.dir, since that value is the directory path. Remove the name field from the base outcome (or use a separately persisted last-known project name if the manifest provides one) while preserving projectId and path.
🤖 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/cli/src/cli.ts`:
- Line 492: Update the --delete-missing-notebooks option definition in the CLI
configuration to explicitly state that it is reserved for push and currently
inert while push is deferred, matching the existing README and cli-sync
documentation; do not change SyncOptions.deleteMissingNotebooks or sync
behavior.
In `@packages/cli/src/commands/sync.ts`:
- Around line 182-197: Update syncOneProject to detect caught errors whose name
is 'ExitPromptError' and re-throw them before converting other failures into an
error outcome. Preserve the existing handling for non-exit errors so
syncWorkspace can stop on Ctrl+C while normal project failures continue through
the current flow.
In `@packages/cli/src/utils/sync-paths.ts`:
- Around line 28-35: Update sanitizePathSegment in
packages/cli/src/utils/sync-paths.ts:28-35 to apply slice(0, MAX_SEGMENT_LENGTH)
before removing trailing dots and spaces, ensuring truncated segments are
filesystem-safe. Add tests in packages/cli/src/utils/sync-paths.test.ts:4-28
covering names exceeding 120 characters where truncation ends on a dot and on a
space, asserting the exact sanitized segments.
In `@skills/deepnote/references/cli-sync.md`:
- Line 51: Update the inline code span in the description of cloud renames so
the leading space is outside the backticks, while keeping the deterministic
`(<short id>)` suffix unchanged.
---
Nitpick comments:
In `@packages/cli/src/commands/sync.test.ts`:
- Around line 196-221: Add a test covering an unsafe notebook filename such as
../escape.deepnote in the export archive, targeting writeProjectNotebooks.
Verify the entry is skipped and no file is created outside the project
directory, while preserving normal notebook export behavior.
In `@packages/cli/src/commands/sync.ts`:
- Around line 556-576: The missing-in-cloud outcome in the manifest project loop
should not populate name from record.dir, since that value is the directory
path. Remove the name field from the base outcome (or use a separately persisted
last-known project name if the manifest provides one) while preserving projectId
and path.
In `@packages/cli/src/utils/sync-manifest.test.ts`:
- Around line 74-76: Move the test for emptySyncManifest out of the
saveSyncManifest describe block and place it in a dedicated describe block for
emptySyncManifest, preserving its existing assertion and clear test name.
In `@packages/cli/src/utils/sync-manifest.ts`:
- Around line 113-114: Update the manifest-writing flow in the sync manifest
utility to write content to a sibling temporary file first, then rename that
temporary file to the path used by SYNC_MANIFEST_FILENAME. Ensure the rename
occurs only after the write completes and clean up or safely handle the
temporary file on failure.
In `@packages/cli/src/utils/sync-paths.test.ts`:
- Around line 4-28: Extend the sanitizePathSegment tests with names exceeding
MAX_SEGMENT_LENGTH, including a 200-character input and a 200-character input
whose 120th character is a dot or space. Assert the exact truncated and
sanitized segment output, including the required final-character behavior after
truncation.
In `@packages/cloud/src/sync.test.ts`:
- Line 29: Update the arrayBuffer implementation in the test response fixture to
copy the Uint8Array view before returning its buffer, preserving the view’s
byteOffset and byteLength rather than exposing the entire backing store. Keep
the existing empty-byte fallback behavior.
🪄 Autofix
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: e8db11f5-1f69-491d-8f33-e6471812a11e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
cspell.jsonpackages/cli/README.mdpackages/cli/package.jsonpackages/cli/src/cli.tspackages/cli/src/commands/sync.test.tspackages/cli/src/commands/sync.tspackages/cli/src/utils/sync-manifest.test.tspackages/cli/src/utils/sync-manifest.tspackages/cli/src/utils/sync-paths.test.tspackages/cli/src/utils/sync-paths.tspackages/cloud/README.mdpackages/cloud/package.jsonpackages/cloud/src/index.tspackages/cloud/src/sync.test.tspackages/cloud/src/sync.tsskills/deepnote/SKILL.mdskills/deepnote/references/cli-sync.md
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #451 +/- ##
==========================================
+ Coverage 88.11% 88.23% +0.12%
==========================================
Files 187 191 +4
Lines 9936 10688 +752
Branches 2847 2993 +146
==========================================
+ Hits 8755 9431 +676
- Misses 1180 1255 +75
- Partials 1 2 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Push is no longer deferred. A project edited only locally is re-uploaded as
the same ZIP of `.deepnote` documents the export produced — no re-merge, no
re-serialization — to `POST /v2/projects/{id}/import`, with baseModifiedAt +
baseContentHash for lost-update protection (409 → override/skip). Because the
unit is identical in both directions, the content-hash contract is symmetric
and unambiguous: both sides hash the exploded documents the same way.
- @deepnote/cloud: importProject now takes the notebook documents and POSTs a
ZIP (application/zip); adds uploadProjectFile (POST /v2/files) and
deleteProjectFile (DELETE /v2/files, tolerates 404) for working-directory
files; import result carries the post-import contentHash.
- cli sync: un-defers push (import → re-export → write back); resolves 409 as
override/skip; an empty local project is confirmed before a delete-all.
--all-files now uploads changed local files on push (delete-then-upload,
since POST /v2/files won't overwrite). A 404/501 from import degrades to
push-deferred so nothing is lost where the endpoint isn't deployed yet.
- docs: packages/cloud/docs/project-import-contract.md defines the endpoint
for the server authors — ZIP-in, reconciliation semantics, the exact
canonical content-hash algorithm, params, responses, errors. README/help/
skill updated for working push.
Full suite green (2846 passed); typecheck, biome, prettier, cspell clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/cli.ts (1)
485-485: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe both
--all-filesdirections.The option uploads files on push as well as downloading them on pull. Update the text to state both behaviors.
🤖 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/cli/src/cli.ts` at line 485, Update the help text for the --all-files option in the CLI option definition to describe both downloading working-directory files on pull and uploading them on push, while preserving the existing option behavior.
🤖 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/cli/src/commands/sync.test.ts`:
- Around line 63-67: Update the ImportCall mock and its recording logic to
retain decoded notebook entries in a notebooks map keyed by filename, rather
than recording filenames only. In the local-push test, assert that
cloud.importCalls[0].notebooks['main.deepnote'] equals localEdit, while
preserving existing import-call assertions.
In `@packages/cli/src/commands/sync.ts`:
- Around line 502-504: Update the sync logic around the prev/next manifest
handling so equal file sizes are not treated as unchanged without validation.
Compute and compare a reliable local content fingerprint for same-size files
before reusing prev, upload and replace the manifest record when the fingerprint
differs, and add a test covering an edit that preserves file size.
- Around line 624-634: Update the file-sync branch around uploadProjectFiles and
syncProjectFiles to classify working-directory files independently from the
notebook outcome. Compare files against the manifest, upload local-only changes,
resolve file conflicts, and skip downloading files when the notebook action is
push-deferred; preserve the skipped-conflict behavior and use the
manifest/current record for file state.
In `@packages/cloud/docs/project-import-contract.md`:
- Line 14: Update the fenced code blocks in project-import-contract.md,
including the blocks at the referenced locations, to specify suitable language
identifiers such as text or http instead of leaving the fences untagged.
Preserve each block’s existing content.
---
Outside diff comments:
In `@packages/cli/src/cli.ts`:
- Line 485: Update the help text for the --all-files option in the CLI option
definition to describe both downloading working-directory files on pull and
uploading them on push, while preserving the existing option behavior.
🪄 Autofix
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: b958de9e-9c5f-4622-9199-a192daa0dda9
📒 Files selected for processing (11)
packages/cli/README.mdpackages/cli/src/cli.tspackages/cli/src/commands/sync.test.tspackages/cli/src/commands/sync.tspackages/cloud/README.mdpackages/cloud/docs/project-import-contract.mdpackages/cloud/src/index.tspackages/cloud/src/sync.test.tspackages/cloud/src/sync.tsskills/deepnote/SKILL.mdskills/deepnote/references/cli-sync.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/cli/README.md
- packages/cloud/src/index.ts
- packages/cloud/README.md
- packages/cloud/src/sync.test.ts
slice(0, MAX) ran after the trailing dot/space strip, so a truncated segment could still end in '.' or ' ' — which Windows silently drops, desyncing the manifest path from the created directory. Reorder and cover with tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Re-throw @inquirer/prompts' ExitPromptError from syncOneProject so a Ctrl+C on a conflict prompt stops the whole sync instead of becoming a per-project error. - --all-files upload compared only size, so a same-size local edit never uploaded. Track a content hash per file and compare it. Tests added for both, plus asserting the pushed import ZIP carries the actual local edit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ages Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai Addressed the outside-diff note on |
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cli/src/commands/sync.test.ts`:
- Around line 517-548: Extend the test around syncWorkspace to include a second
cloud project after the initial setup, then assert that its local directory does
not exist after the conflict prompt rejects with ExitPromptError. Keep the
existing rejection and select-call assertions, ensuring the added project would
only be created if synchronization continued after cancellation.
🪄 Autofix
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: dfd4654c-5f36-4df2-838b-029772ccb5d2
📒 Files selected for processing (7)
packages/cli/src/cli.tspackages/cli/src/commands/sync.test.tspackages/cli/src/commands/sync.tspackages/cli/src/utils/sync-manifest.tspackages/cli/src/utils/sync-paths.test.tspackages/cli/src/utils/sync-paths.tspackages/cloud/docs/project-import-contract.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/cli/src/cli.ts
- packages/cli/src/utils/sync-paths.test.ts
- packages/cloud/docs/project-import-contract.md
- packages/cli/src/utils/sync-paths.ts
- packages/cli/src/commands/sync.ts
|
During my local testing session:
|
|
Ran local tests to verify and confirm findings (to degree feasible), nothing major came up. Ran two SOTA agents for findings, here's the compiled list: PR #451 review findings — Agent verification summaryAll 10 findings were verified against the code (plus the server-side counterpart and an Blockers — ideally fix before merge
Confirmed, cheap wins
Valid but deferrable
Bottom line: 10/10 findings verified real, none fabricated. Two blockers (prune data loss — |
|
Boiled down two more recommendations I think are reasonable from the agent - please see if you find them helpful:
|
Reworks the
deepnote syncCLI (#429, Wojtek's branch) for the export endpoint that shipped, and implements the push direction as the exact inverse of export.Why
#429 was built against the single-document export from the original enablement PR (deepnote-internal #20511). The endpoint that merged — deepnote-internal #20596 — instead returns a ZIP with one
.deepnotedocument per notebook. Every layer assumed one file per project, so that had to be reworked. Two other drifts fixed here:folder.pathis now{ id, name }segments +isPathComplete(notstring[]), and only the documents are deterministic (never the ZIP container), so the content hash is computed over the documents.Pull (commit 1)
@deepnote/cloudexportProjectunzips the export into{ filename, content }[](addsfflate); folder schema/types fixed.sync-paths<folder path>/<project name>/, one.deepnoteper notebook);.files/inside it.sync-manifest{ dir, notebooks[], contentHash, ... }, hash over the exploded documents.synccommandPush (commit 2) — the exact inverse of export
Rather than defer push behind an undecided contract, this defines the contract by writing the client for it. Push re-uploads the same ZIP of
.deepnotedocuments the export produced — no re-merge, no re-serialization — toPOST /v2/projects/{id}/import, withbaseModifiedAt+baseContentHashfor lost-update protection. Because the unit is identical in both directions, thebaseContentHashquestion dissolves: both sides hash the exploded documents the same way (algorithm specified in the contract doc).@deepnote/cloud:importProject(files, opts)POSTs a ZIP (application/zip); newuploadProjectFile/deleteProjectFile(real, already-deployedPOST/DELETE /v2/files).synccommand: local-only edit → import → re-export → write back; 409 → override/skip; empty local project confirmed before delete-all;--all-filesuploads changed local files on push (delete-then-upload, sincePOST /v2/fileswon't overwrite).POST /v2/projects/{id}/importis not deployed yet. A 404/501 degrades topush-deferred(edit kept, nothing lost), so the client ships safely ahead of the server.📋 The contract for the server authors
packages/cloud/docs/project-import-contract.mdis the spec to implement against: ZIP-in reconciliation semantics, the exact canonical content-hash algorithm (so both sides match), params, responses, and errors. The client in this PR is the reference implementation.Testing
pnpm test: 164 files, 2846 passed, 0 failures (nested worktree +.envparked per the local-env quirk).pnpm typecheck, biome, prettier, cspell all clean.push-deferred;--all-filesupload (delete-then-upload);uploadProjectFile/deleteProjectFile; canonical-hash order-independence.Not yet done: live verification against production (the export ZIP and the import endpoint may not be deployed to
api.deepnote.comyet).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
deepnote sync [dir]to mirror cloud projects locally in both directions.Documentation