Skip to content

feat(cli): deepnote sync — mirror workspace projects with the local filesystem (GRN-5421) - #429

Closed
voyti wants to merge 2 commits into
mainfrom
wojtek1/grn-5421-deepnote-sync-cli
Closed

feat(cli): deepnote sync — mirror workspace projects with the local filesystem (GRN-5421)#429
voyti wants to merge 2 commits into
mainfrom
wojtek1/grn-5421-deepnote-sync-cli

Conversation

@voyti

@voyti voyti commented Jul 22, 2026

Copy link
Copy Markdown

Implements the CLI half of GRN-5421 ("Sync deepnote.com projects with local filesystem (and git)"). The server half is deepnote-internal #20511 (export/import/download endpoints + folder info on the projects list).

Note: based on #419 (feat/local-runner) so it builds on the packages/cloud work there instead of duplicating projects.ts/import.ts. Once #419 merges, this can be retargeted to main. Draft until the live staging round-trip is verified.

What this adds

deepnote sync [dir]

Clones the workspace into a local directory — every project becomes <folder path>/<project name>.deepnote, following the workspace folder tree — and pushes local edits back on the next sync.

$ deepnote sync workspace
↓ pulled    Analytics/Sales report.deepnote
↑ pushed    Drafts/Experiment.deepnote — Main: overwritten
· unchanged Onboarding.deepnote

1 pulled, 1 pushed, 1 unchanged

Flags: --all-files (download working-directory files, incremental), --on-conflict ask|skip|override, --delete-missing-notebooks, --prune, --dry-run, -o json, plus the usual --url/--token (DEEPNOTE_TOKEN).

@deepnote/cloud additions (src/sync.ts)

listAllProjects (walks pagination to exhaustion — unlike #419's name-narrowed findNotebook walk, sync needs the complete list), getProjectDetail (file inventory), exportProject, importProject, downloadProjectFile. Same conventions as the existing modules: global fetch, ApiError, permissive zod schemas.

Manifest design

Sync state lives in .deepnote-sync.json at the sync root (plain JSON, sorted keys, safe to commit). Per project id:

  • path — local file path. Identity is always the project id, never a name: neither project nor folder names are unique in Deepnote. Cloud renames become local file moves; two same-named folders merge into one local directory; path collisions (case-insensitive, so macOS/Windows are safe) are resolved deterministically by suffixing every collision-group member with a short id — a new project can never silently steal an existing project's clean path.
  • modifiedAt — the export's metadata.modifiedAt at last sync; sent back as baseModifiedAt on every push (lost-update protection).
  • contentHash — SHA-256 of the last-synced bytes. The server export is deterministic, so comparing the local file and a fresh export against this hash cleanly separates local edit / cloud edit / both with no clocks involved.
  • files — per-path size/updatedAt for incremental --all-files downloads (into <name>.files/ beside the .deepnote file; download-only, no upload).

Conflict UX

  • Only local changed → push (POST /import with baseModifiedAt). A 409 means the cloud moved concurrently → prompt: overwrite cloud (force=true retry) or skip.
  • Both changed (or an untracked local file differs from the cloud) → prompt: overwrite local with the cloud version, or skip.
  • --on-conflict skip|override answers up front; ask degrades to skip when there's no TTY (cron/CI, -o json), so nothing ever hangs on a prompt.
  • A skipped project is skipped entirely (including its --all-files downloads). Per-project errors don't abort the rest of the workspace; exit 1 if any project failed.

Explicitly noted

  • requirements.txt precedence: the server never applies project name, integrations, or settings.requirements from a pushed document — requirements.txt in the project files is the source of truth, settings.requirements is a lossy projection. After every successful push the local file is rewritten from a fresh export so those never-applied fields (and server-assigned notebook ids) don't drift.
  • Folder-name non-uniqueness: handled via id-keyed manifest + deterministic collision suffixes (see above).
  • Git is a no-op here: sync writes ordinary files; the user runs git themselves. Also: sync never creates or deletes cloud projects (local-only .deepnote files are reported and left alone), and never deletes local files without --prune.

Testing

  • packages/cloud/src/sync.test.ts — endpoint contracts, pagination walk + runaway guard, error mapping (409 carries status for the conflict flow).
  • packages/cli/src/utils/sync-paths.test.ts — sanitization (Windows reserved names, control chars, trailing dots), deterministic collision handling, traversal-safe inventory paths.
  • packages/cli/src/utils/sync-manifest.test.ts — round-trip, corrupt-manifest rejection, byte-stable output.
  • packages/cli/src/commands/sync.test.ts — end-to-end against a mocked API + real temp dirs: first pull, no-op re-sync, pull, push (asserts baseModifiedAt + canonical re-export write-back), 409 skip/override, both-sides conflict, --all-files incremental download + unsafe-path skip, --prune, rename→move, untracked files, dry-run, per-project error isolation.

All repo checks green: lintAndFormat, typecheck, test (2684 passed), spell-check, build.

Remaining before un-draft

  • Live round-trip verified 2026-07-28 against review app ra-20511.deepnote-staging.com (#20511 branch): pull of a 58-project workspace with folder tree → no-op re-pull → local edit → push (notebooks overwritten/created, canonical re-export written back) → no-op re-pull. Also verified live: both-sides conflict skip + --on-conflict override, --all-files incremental download (byte-correct), cloud-deletion kept-then---prune, and per-project error isolation on a malformed local document.

🤖 Generated with Claude Code

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Added the deepnote sync [dir] command to mirror cloud projects locally and sync local changes back.
    • Added options for conflict handling (ask, skip, override), --dry-run, --prune, and --all-files, including machine-readable output.
    • Introduced deterministic local path mapping and sync tracking via .deepnote-sync.json.
  • Documentation

    • Expanded CLI and reference docs with command behavior, options, examples, and safety/conflict details.
  • Bug Fixes

    • Improved safety when --delete-missing-notebooks is used with projects whose local document contains no notebooks.
  • Tests / Chores

    • Added comprehensive test coverage for sync planning, manifest handling, and the cloud sync client; updated spell checker configuration.

@linear-code

linear-code Bot commented Jul 22, 2026

Copy link
Copy Markdown

GRN-5421

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds bidirectional deepnote sync support backed by new cloud APIs. Projects are exported as deterministic YAML, mapped to sanitized local paths, and tracked in .deepnote-sync.json. The workflow supports pull, push, no-op, conflict handling, pruning, optional file downloads, dry runs, JSON output, and lost-update protection. It includes API, manifest, path, integration tests, and CLI documentation.

Estimated code review effort: 4 (Complex) | ~60 minutes

Suggested reviewers: dinohamzic, m1so, jamesbhobbs

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant SyncWorkspace
  participant DeepnoteCloud
  participant LocalWorkspace
  CLI->>SyncWorkspace: execute sync options
  SyncWorkspace->>DeepnoteCloud: list projects and export YAML
  DeepnoteCloud-->>SyncWorkspace: project data
  SyncWorkspace->>LocalWorkspace: compare hashes and manifest records
  SyncWorkspace->>DeepnoteCloud: import changed YAML with concurrency fields
  DeepnoteCloud-->>SyncWorkspace: canonical export or conflict
  SyncWorkspace->>LocalWorkspace: update files and manifest
  SyncWorkspace-->>CLI: report outcomes and exit status
Loading
🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 69.23% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the new deepnote sync CLI feature and its main filesystem mirroring behavior.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Updates Docs ✅ Passed OSS docs were updated in the CLI/cloud READMEs and Deepnote sync references; I couldn't verify the private roadmap page, so please confirm that separately.

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.97821% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.48%. Comparing base (126ae4e) to head (15d5b33).

Files with missing lines Patch % Lines
packages/cli/src/commands/sync.ts 87.07% 37 Missing and 1 partial ⚠️
packages/cli/src/cli.ts 25.00% 3 Missing ⚠️
packages/cloud/src/sync.ts 96.47% 3 Missing ⚠️
packages/cli/src/utils/sync-manifest.ts 96.66% 1 Missing ⚠️
packages/cli/src/utils/sync-paths.ts 97.82% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #429      +/-   ##
==========================================
+ Coverage   87.36%   87.48%   +0.12%     
==========================================
  Files         181      185       +4     
  Lines        9494     9953     +459     
  Branches     2624     2768     +144     
==========================================
+ Hits         8294     8707     +413     
- Misses       1199     1244      +45     
- Partials        1        2       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
packages/cli/src/utils/sync-manifest.ts (1)

94-106: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Use code-unit ordering here. localeCompare() without an explicit locale can sort differently across environments, so the manifest can churn between machines.

🤖 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 94 - 106, Replace the
locale-dependent comparators in the sortedProjects project and record.files
ordering with deterministic code-unit comparisons, preserving the existing
ascending key order and manifest structure.
🤖 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/utils/sync-paths.ts`:
- Around line 26-33: Update the cleaning chain for name so .slice(0,
MAX_SEGMENT_LENGTH) runs before .replace(/[. ]+$/, ''). Keep the existing
normalization, illegal-character replacement, trimming, and length limit while
ensuring the final segment cannot end in a dot or space.

In `@skills/deepnote/references/cli-sync.md`:
- Line 47: Update the inline code span describing the deterministic
path-collision suffix so the leading space is outside the backticks, preserving
the displayed ` (<short id>)` meaning while satisfying MD038.

---

Nitpick comments:
In `@packages/cli/src/utils/sync-manifest.ts`:
- Around line 94-106: Replace the locale-dependent comparators in the
sortedProjects project and record.files ordering with deterministic code-unit
comparisons, preserving the existing ascending key order and manifest structure.
🪄 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: 3c2dc592-b92f-4c59-9c6b-37299d83c850

📥 Commits

Reviewing files that changed from the base of the PR and between 3103f8a and 2f377f0.

📒 Files selected for processing (15)
  • cspell.json
  • packages/cli/README.md
  • packages/cli/src/cli.ts
  • packages/cli/src/commands/sync.test.ts
  • packages/cli/src/commands/sync.ts
  • packages/cli/src/utils/sync-manifest.test.ts
  • packages/cli/src/utils/sync-manifest.ts
  • packages/cli/src/utils/sync-paths.test.ts
  • packages/cli/src/utils/sync-paths.ts
  • packages/cloud/README.md
  • packages/cloud/src/index.ts
  • packages/cloud/src/sync.test.ts
  • packages/cloud/src/sync.ts
  • skills/deepnote/SKILL.md
  • skills/deepnote/references/cli-sync.md

Comment on lines +26 to +33
const cleaned = name
.normalize('NFC')
.replace(ILLEGAL_CHARACTERS, '_')
.trim()
// Windows silently strips trailing dots and spaces, which would desync the manifest's idea of
// the path from what the filesystem actually created.
.replace(/[. ]+$/, '')
.slice(0, MAX_SEGMENT_LENGTH)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

slice after the trailing-strip can re-add a trailing dot/space.

Truncating at MAX_SEGMENT_LENGTH runs after .replace(/[. ]+$/, ''), so a name longer than the limit that lands a . or space on the boundary keeps it — the Windows silent-strip this guards against, desyncing the manifest path from the file actually created. Strip after slicing.

🐛 Reorder slice before the trailing strip
   const cleaned = name
     .normalize('NFC')
     .replace(ILLEGAL_CHARACTERS, '_')
     .trim()
+    .slice(0, MAX_SEGMENT_LENGTH)
     // Windows silently strips trailing dots and spaces, which would desync the manifest's idea of
     // the path from what the filesystem actually created.
     .replace(/[. ]+$/, '')
-    .slice(0, MAX_SEGMENT_LENGTH)
📝 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
const cleaned = name
.normalize('NFC')
.replace(ILLEGAL_CHARACTERS, '_')
.trim()
// Windows silently strips trailing dots and spaces, which would desync the manifest's idea of
// the path from what the filesystem actually created.
.replace(/[. ]+$/, '')
.slice(0, MAX_SEGMENT_LENGTH)
const cleaned = name
.normalize('NFC')
.replace(ILLEGAL_CHARACTERS, '_')
.trim()
.slice(0, MAX_SEGMENT_LENGTH)
// Windows silently strips trailing dots and spaces, which would desync the manifest's idea of
// the path from what the filesystem actually created.
.replace(/[. ]+$/, '')
🤖 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.ts` around lines 26 - 33, Update the
cleaning chain for name so .slice(0, MAX_SEGMENT_LENGTH) runs before
.replace(/[. ]+$/, ''). Keep the existing normalization, illegal-character
replacement, trimming, and length limit while ensuring the final segment cannot
end in a dot or space.

State lives in `.deepnote-sync.json` in the synced directory: a map of project id → local path,
last-synced `metadata.modifiedAt`, and a content hash. Projects are tracked by id because names
(projects and folders) are **not unique** in Deepnote — cloud renames become local file moves, and
path collisions get a deterministic ` (<short id>)` suffix.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

MD038: leading space inside the code span. Move the space out so lint passes without losing meaning.

✏️ Fix
-path collisions get a deterministic ` (<short id>)` suffix.
+path collisions get a deterministic `(<short id>)` suffix (preceded by a space).
📝 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
path collisions get a deterministic ` (<short id>)` suffix.
path collisions get a deterministic `(<short id>)` suffix (preceded by a space).
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)

[warning] 47-47: Spaces inside code span elements

(MD038, no-space-in-code)

🤖 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 `@skills/deepnote/references/cli-sync.md` at line 47, Update the inline code
span describing the deterministic path-collision suffix so the leading space is
outside the backticks, preserving the displayed ` (<short id>)` meaning while
satisfying MD038.

Source: Linters/SAST tools

Base automatically changed from feat/local-runner to main July 24, 2026 08:13

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 298-304: Update the syncWorkspace invocation in the test “skips
pushing a no-notebook document under --delete-missing-notebooks instead of
wiping the project” to explicitly pass onConflict: 'skip' alongside the existing
baseOptions and deleteMissingNotebooks settings, ensuring the test does not
depend on TTY behavior.
🪄 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: 8b19b0ff-1cb8-4299-b54c-7bbb27776ed1

📥 Commits

Reviewing files that changed from the base of the PR and between 2f377f0 and 57246ca.

📒 Files selected for processing (6)
  • packages/cli/src/commands/sync.test.ts
  • packages/cli/src/commands/sync.ts
  • packages/cloud/README.md
  • packages/cloud/src/sync.test.ts
  • packages/cloud/src/sync.ts
  • skills/deepnote/references/cli-sync.md
🚧 Files skipped from review as they are similar to previous changes (5)
  • skills/deepnote/references/cli-sync.md
  • packages/cloud/src/sync.ts
  • packages/cloud/README.md
  • packages/cloud/src/sync.test.ts
  • packages/cli/src/commands/sync.ts

Comment thread packages/cli/src/commands/sync.test.ts
voyti added 2 commits July 29, 2026 10:20
…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
@voyti
voyti force-pushed the wojtek1/grn-5421-deepnote-sync-cli branch from 57246ca to 15d5b33 Compare July 29, 2026 08:27

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
packages/cli/src/commands/sync.ts (1)

432-440: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Dry run mutates the manifest on noop but not on pull. Harmless today because saveSyncManifest is skipped, but it makes the in-memory manifest state depend on the branch. Consider gating the noop commit on !ctx.dryRun too.

🤖 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 432 - 440, Gate the
commitRecord call in the step === 'noop' branch on !ctx.dryRun, matching the
existing behavior in the step === 'pull' branch. Keep the unchanged outcome
construction and non-dry-run commit behavior intact.
packages/cli/src/utils/sync-manifest.ts (1)

94-108: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

localeCompare undercuts the determinism goal. Its ordering depends on the runtime's locale/ICU data, so the same manifest can serialize differently on two machines — exactly the git churn the sorting exists to prevent. Use a plain codepoint comparison.

Also: the write is not atomic. An interrupted fs.writeFile leaves a truncated manifest, which loadSyncManifest then rejects outright, forcing a full re-sync. Write to a temp file and rename.

♻️ Proposed change
+const byKey = ([a]: [string, unknown], [b]: [string, unknown]): number => (a < b ? -1 : a > b ? 1 : 0)
+
 export async function saveSyncManifest(rootDir: string, manifest: SyncManifest): Promise<void> {
   const sortedProjects = Object.fromEntries(
     Object.entries(manifest.projects)
-      .sort(([a], [b]) => a.localeCompare(b))
+      .sort(byKey)
       .map(([id, record]) => [
         id,
         {
           ...record,
           ...(record.files
-            ? { files: Object.fromEntries(Object.entries(record.files).sort(([a], [b]) => a.localeCompare(b))) }
+            ? { files: Object.fromEntries(Object.entries(record.files).sort(byKey)) }
             : {}),
         },
       ])
   )
   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`
+  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 94 - 108, Update the
sorting in the manifest serialization flow to use a locale-independent plain
codepoint comparator for both project IDs and file paths instead of
localeCompare. In the write flow, stage the serialized content in a temporary
file and rename it to SYNC_MANIFEST_FILENAME only after the write completes,
using the existing sync-manifest symbols and preserving the final manifest
format.
🤖 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.ts`:
- Around line 227-230: Validate manifest record paths with
isSafeRelativeFilePath before using them in the prune deletion flow, including
the logic around toAbsolute and the .deepnote/.files replacement near
record.path. Skip or reject unsafe paths before any fs.rm call, and ensure
non-.deepnote paths cannot cause recursive deletion of the raw manifest path
outside ctx.rootDir.

In `@skills/deepnote/references/cli-sync.md`:
- Line 21: Remove the unsupported `llm` value from the output-format
documentation for the sync command in `skills/deepnote/references/cli-sync.md`,
leaving only `json` unless the implementation is explicitly expanded to support
`llm`.

---

Nitpick comments:
In `@packages/cli/src/commands/sync.ts`:
- Around line 432-440: Gate the commitRecord call in the step === 'noop' branch
on !ctx.dryRun, matching the existing behavior in the step === 'pull' branch.
Keep the unchanged outcome construction and non-dry-run commit behavior intact.

In `@packages/cli/src/utils/sync-manifest.ts`:
- Around line 94-108: Update the sorting in the manifest serialization flow to
use a locale-independent plain codepoint comparator for both project IDs and
file paths instead of localeCompare. In the write flow, stage the serialized
content in a temporary file and rename it to SYNC_MANIFEST_FILENAME only after
the write completes, using the existing sync-manifest symbols and preserving the
final manifest format.
🪄 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: 7aa5132e-8320-485b-90f2-340bf3a9d9ad

📥 Commits

Reviewing files that changed from the base of the PR and between 57246ca and 15d5b33.

📒 Files selected for processing (15)
  • cspell.json
  • packages/cli/README.md
  • packages/cli/src/cli.ts
  • packages/cli/src/commands/sync.test.ts
  • packages/cli/src/commands/sync.ts
  • packages/cli/src/utils/sync-manifest.test.ts
  • packages/cli/src/utils/sync-manifest.ts
  • packages/cli/src/utils/sync-paths.test.ts
  • packages/cli/src/utils/sync-paths.ts
  • packages/cloud/README.md
  • packages/cloud/src/index.ts
  • packages/cloud/src/sync.test.ts
  • packages/cloud/src/sync.ts
  • skills/deepnote/SKILL.md
  • skills/deepnote/references/cli-sync.md
🚧 Files skipped from review as they are similar to previous changes (8)
  • packages/cloud/src/index.ts
  • cspell.json
  • packages/cli/src/utils/sync-paths.ts
  • packages/cli/src/cli.ts
  • packages/cloud/src/sync.test.ts
  • packages/cli/src/utils/sync-paths.test.ts
  • packages/cloud/src/sync.ts
  • packages/cli/README.md

Comment on lines +227 to +230
/** Join a manifest-style POSIX relative path onto the sync root for filesystem access. */
function toAbsolute(ctx: SyncContext, relativePath: string): string {
return path.join(ctx.rootDir, ...relativePath.split('/'))
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Validate manifest paths before deleting with them. record.path is only typed as string by the manifest schema, and toAbsolute happily joins ../.. segments. A hand-edited or committed-and-cloned .deepnote-sync.json can therefore point --prune at a recursive fs.rm outside the sync root. syncProjectFiles already guards inventory paths with isSafeRelativeFilePath; the manifest path deserves the same treatment. Also note line 587: if record.path does not end in .deepnote, the .files replace is a no-op and the recursive delete targets record.path itself.

🛡️ Proposed guard
 function toAbsolute(ctx: SyncContext, relativePath: string): string {
+  if (!isSafeRelativeFilePath(relativePath)) {
+    throw new Error(`Unsafe path in the sync manifest: ${relativePath}`)
+  }
   return path.join(ctx.rootDir, ...relativePath.split('/'))
 }
     if (ctx.options.prune) {
       if (!ctx.dryRun) {
         await fs.rm(toAbsolute(ctx, record.path), { force: true })
-        await fs.rm(toAbsolute(ctx, record.path.replace(/\.deepnote$/, '.files')), { recursive: true, force: true })
+        if (record.path.endsWith('.deepnote')) {
+          await fs.rm(toAbsolute(ctx, record.path.replace(/\.deepnote$/, '.files')), { recursive: true, force: true })
+        }
         delete manifest.projects[projectId]
       }

Also applies to: 583-589

🤖 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 227 - 230, Validate manifest
record paths with isSafeRelativeFilePath before using them in the prune deletion
flow, including the logic around toAbsolute and the .deepnote/.files replacement
near record.path. Skip or reject unsafe paths before any fs.rm call, and ensure
non-.deepnote paths cannot cause recursive deletion of the raw manifest path
outside ctx.rootDir.

| `--delete-missing-notebooks` | When pushing, delete cloud notebooks removed from the local file |
| `--prune` | Delete local files for projects/files that no longer exist in the cloud |
| `--dry-run` | Show what would be synced without writing anything |
| `-o, --output <format>` | Output format: `json`, `llm` |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

llm is not a supported output format here. SyncOptions.output is typed 'json' and createSyncAction only branches on 'json', falling through to the human summary otherwise — while still suppressing progress lines. Drop llm from the doc (or implement it).

✏️ Fix
-| `-o, --output <format>`      | Output format: `json`, `llm`                                            |
+| `-o, --output <format>`      | Output format: `json`                                                   |

As per coding guidelines, "When changing the .deepnote file format, CLI commands, or MCP tools, update the corresponding reference files under skills/deepnote/."

📝 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
| `-o, --output <format>` | Output format: `json`, `llm` |
| `-o, --output <format>` | Output format: `json` |
🤖 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 `@skills/deepnote/references/cli-sync.md` at line 21, Remove the unsupported
`llm` value from the output-format documentation for the sync command in
`skills/deepnote/references/cli-sync.md`, leaving only `json` unless the
implementation is explicitly expanded to support `llm`.

Source: Path instructions

@jamesbhobbs

Copy link
Copy Markdown
Contributor

Superseded by #451

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants