feat(cli): implement deepnote run --cloud --push to sync local blocks before running - #432
feat(cli): implement deepnote run --cloud --push to sync local blocks before running#432jamesbhobbs wants to merge 2 commits into
deepnote run --cloud --push to sync local blocks before running#432Conversation
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds typed Deepnote block API operations with response validation and normalization. Adds local block conversion, synchronization planning, ordered mutation execution, move planning, progress reporting, and recreated-block ID remapping. Adds Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to The PR adds local notebook synchronization before cloud runs, with confirmation and dry-run safeguards; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fix/cloud-run-reliability #432 +/- ##
=============================================================
+ Coverage 88.38% 88.57% +0.18%
=============================================================
Files 192 196 +4
Lines 10748 11066 +318
Branches 3014 3101 +87
=============================================================
+ Hits 9500 9802 +302
- Misses 1246 1262 +16
Partials 2 2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
packages/cloud/README.md (1)
61-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTypes row is now stale.
BlockDetail,NotebookDetail,NotebookBlock,CreateBlockParams,UpdateBlockPatch,BlockPlacementare exported but unlisted.🤖 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/README.md` at line 61, Update the Types row in the cloud package README to include the exported types BlockDetail, NotebookDetail, NotebookBlock, CreateBlockParams, UpdateBlockPatch, and BlockPlacement alongside the existing entries.packages/local-runner/src/sync-notebook-content.ts (2)
344-344: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
includesinsidefilteris O(n²).- const survivingRemote = remote.blocks.map(b => b.id).filter(id => survivingLocal.includes(id)) + const survivingLocalSet = new Set(survivingLocal) + const survivingRemote = remote.blocks.map(b => b.id).filter(id => survivingLocalSet.has(id))🤖 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/local-runner/src/sync-notebook-content.ts` at line 344, Update the survivingRemote calculation to avoid repeatedly calling survivingLocal.includes inside the filter. Build a lookup set from survivingLocal and use it for membership checks while preserving the existing remote block order and resulting IDs.
290-290: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
as BlockSpechides a real invariant.Three casts paper over
Map.getreturningundefined. A single lookup helper that throws with the block id would keep strict typing and give a better failure than a laterundefined.type.As per coding guidelines: "Prefer type safety over convenience - avoid
anytypes and use proper type definitions".Also applies to: 416-416, 434-434
🤖 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/local-runner/src/sync-notebook-content.ts` at line 290, Replace the non-null assertions/casts around specs.get in the sync notebook flow, including the lookups near block handling and the locations corresponding to lines 416 and 434, with one typed helper that retrieves a BlockSpec by block id and throws an error including that id when absent. Reuse the helper for all three lookups so strict typing is preserved and downstream code receives a guaranteed BlockSpec.Source: Coding guidelines
packages/cloud/src/blocks.test.ts (1)
168-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo 403 case anywhere in the suite.
That's exactly the path where
forbiddenMessageis (supposed to be) used — see thehttp.tsline 46-53 comment. Worth a test once that precedence is fixed.🤖 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/blocks.test.ts` around lines 168 - 178, Add a test alongside the existing createBlock error tests that mocks a 403 response and verifies the rejected error uses the expected forbiddenMessage rather than the API response message, covering the precedence documented in http.ts.packages/cloud/src/blocks.ts (1)
189-191: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAn array metadata would be cast to
Record<string, unknown>.
typeof [] === 'object', so a stray array slips through the cast. Cheap guard:- metadata: - block.metadata && typeof block.metadata === 'object' ? (block.metadata as Record<string, unknown>) : undefined, + metadata: + block.metadata && typeof block.metadata === 'object' && !Array.isArray(block.metadata) + ? (block.metadata as Record<string, unknown>) + : undefined,🤖 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/blocks.ts` around lines 189 - 191, Update the metadata normalization in the block mapping to reject arrays before casting to Record<string, unknown>; retain object metadata and return undefined for null, primitive, or array values.packages/cli/src/utils/push-to-cloud.test.ts (1)
150-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo failure-path test.
syncNotebookContentrejecting (spinnerfail, error rethrown) is untested; that's the path users hit when the API 403s mid-push.🤖 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/push-to-cloud.test.ts` around lines 150 - 172, Add a failure-path test for pushLocalNotebook that makes runnerMock.syncNotebookContent reject, then assert the spinner failure handling occurs and the rejection is propagated to the caller. Use the existing runnerMock and BASE test setup, and verify the error from an API failure such as a 403 is preserved.packages/local-runner/src/sync-notebook-content.test.ts (1)
305-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCreates and moves are never exercised together.
Every ordering test has either an insertion or a reorder, never both — which is precisely the case flagged in
sync-notebook-content.tslines 339-345/415-432.🤖 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/local-runner/src/sync-notebook-content.test.ts` around lines 305 - 364, The syncNotebookContent tests do not cover ordering when a block is both created and existing blocks must be reordered. Add a test combining a new block with changed ordering, then assert the createBlock and reorderBlocks requests occur in the planned sequence after content synchronization and verify the resulting move count.packages/local-runner/src/block-spec.ts (1)
1-74: 📐 Maintainability & Code Quality | 🔵 TrivialExtraction looks correct.
Faithful move of
toBlockSpec/mapBlockIdsout ofrun-in-cloud.ts; logic unchanged.Optional: since this module now has its own non-trivial UUID-validation/warning branch, a dedicated
block-spec.test.tsnext to it would pin down behavior independent of the two call sites — good-to-have, not blocking.🤖 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/local-runner/src/block-spec.ts` around lines 1 - 74, Add a dedicated block-spec.test.ts covering toBlockSpec’s UUID validation, metadata extraction, warning behavior, and mapBlockIds mapping/error cases, so the extracted logic is tested independently of its callers.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 `@packages/cli/src/utils/push-to-cloud.ts`:
- Around line 124-141: Replace the plain Error thrown by the non-TTY or
machine-output confirmation guard in the push flow with CloudRunUsageError,
preserving the existing message and confirmation behavior.
In `@packages/cloud/src/http.ts`:
- Around line 46-53: Update the 403 handling in the HTTP request flow to
preserve and use the caller-provided forbiddenMessage when the response body
contains no useful API message. Adjust the parseApiErrorMessage usage or
fallback selection so its generic HTTP fallback does not mask forbiddenMessage,
while retaining the existing parsed message and default access-denied behavior.
- Line 64: Update the request signal construction in the HTTP request options to
always enforce the timeout while respecting a caller-provided signal. Create the
timeout signal from options.timeoutMs or DEFAULT_REQUEST_TIMEOUT_MS, then
combine it with options.signal using AbortSignal.any when both are present,
preserving timeout-only behavior when no caller signal is supplied.
In `@packages/local-runner/src/sync-notebook-content.ts`:
- Around line 405-408: Update the progress reporting loops around the ordered
changes and the additional moves to use a total of ordered.length plus
moves.length, matching the CLI spinner’s total. Ensure both affected onProgress
calls report this combined total so progress does not reach completion before
move requests finish.
- Around line 339-345: The move-planning flow around planMoves must include
created blocks in the final post-create order instead of filtering them out; map
target ids through idRemap as needed, while preserving the existing create
positioning behavior. In packages/local-runner/src/sync-notebook-content.ts
lines 339-345, update the surviving-order inputs accordingly. In
packages/local-runner/src/sync-notebook-content.test.ts lines 305-364, add
coverage for one reordered surviving block plus one inserted block and assert
the resulting final order.
---
Nitpick comments:
In `@packages/cli/src/utils/push-to-cloud.test.ts`:
- Around line 150-172: Add a failure-path test for pushLocalNotebook that makes
runnerMock.syncNotebookContent reject, then assert the spinner failure handling
occurs and the rejection is propagated to the caller. Use the existing
runnerMock and BASE test setup, and verify the error from an API failure such as
a 403 is preserved.
In `@packages/cloud/README.md`:
- Line 61: Update the Types row in the cloud package README to include the
exported types BlockDetail, NotebookDetail, NotebookBlock, CreateBlockParams,
UpdateBlockPatch, and BlockPlacement alongside the existing entries.
In `@packages/cloud/src/blocks.test.ts`:
- Around line 168-178: Add a test alongside the existing createBlock error tests
that mocks a 403 response and verifies the rejected error uses the expected
forbiddenMessage rather than the API response message, covering the precedence
documented in http.ts.
In `@packages/cloud/src/blocks.ts`:
- Around line 189-191: Update the metadata normalization in the block mapping to
reject arrays before casting to Record<string, unknown>; retain object metadata
and return undefined for null, primitive, or array values.
In `@packages/local-runner/src/block-spec.ts`:
- Around line 1-74: Add a dedicated block-spec.test.ts covering toBlockSpec’s
UUID validation, metadata extraction, warning behavior, and mapBlockIds
mapping/error cases, so the extracted logic is tested independently of its
callers.
In `@packages/local-runner/src/sync-notebook-content.test.ts`:
- Around line 305-364: The syncNotebookContent tests do not cover ordering when
a block is both created and existing blocks must be reordered. Add a test
combining a new block with changed ordering, then assert the createBlock and
reorderBlocks requests occur in the planned sequence after content
synchronization and verify the resulting move count.
In `@packages/local-runner/src/sync-notebook-content.ts`:
- Line 344: Update the survivingRemote calculation to avoid repeatedly calling
survivingLocal.includes inside the filter. Build a lookup set from
survivingLocal and use it for membership checks while preserving the existing
remote block order and resulting IDs.
- Line 290: Replace the non-null assertions/casts around specs.get in the sync
notebook flow, including the lookups near block handling and the locations
corresponding to lines 416 and 434, with one typed helper that retrieves a
BlockSpec by block id and throws an error including that id when absent. Reuse
the helper for all three lookups so strict typing is preserved and downstream
code receives a guaranteed BlockSpec.
🪄 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: bd201455-2d47-40ce-bf4a-dd8247ca16d3
📒 Files selected for processing (22)
packages/cli/README.mdpackages/cli/src/cli.test.tspackages/cli/src/cli.tspackages/cli/src/commands/run.tspackages/cli/src/completions.tspackages/cli/src/utils/push-to-cloud.test.tspackages/cli/src/utils/push-to-cloud.tspackages/cli/src/utils/run-in-cloud.test.tspackages/cli/src/utils/run-in-cloud.tspackages/cloud/README.mdpackages/cloud/src/blocks.test.tspackages/cloud/src/blocks.tspackages/cloud/src/create-project.tspackages/cloud/src/http.tspackages/cloud/src/index.tspackages/local-runner/README.mdpackages/local-runner/src/block-spec.tspackages/local-runner/src/index.tspackages/local-runner/src/run-in-cloud.tspackages/local-runner/src/sync-notebook-content.test.tspackages/local-runner/src/sync-notebook-content.tsskills/deepnote/references/cli-run.md
Five findings, all verified against the code first. - `http.ts`: `forbiddenMessage` was dead. `parseApiErrorMessage` falls back to `"<fallback>: HTTP <status>"`, so `message` is never empty and the `||` chain never reached it. Compare against that generic string instead, so a message the API actually sent still wins. - `http.ts`: `signal ?? timeout` dropped the deadline for every caller passing a signal, so those requests could hang forever. Combine both with `AbortSignal.any` (Node 22.14+ is already required). - `sync-notebook-content.ts`: created blocks were excluded from move planning while being positioned by final-order index against a still-unordered notebook, so inserting into a notebook that also needed reordering landed the new block in the wrong slot with nothing to correct it. Moves are now planned over the post-create order, and re-derived from a re-read of the notebook before being applied — which also drops the reliance on `position` inserting exactly where we assumed. Covered by a test that replays the moves and asserts the final order. - `sync-notebook-content.ts`: the progress total omitted the moves, so the caller's bar finished while reorder requests were still going out. - `push-to-cloud.ts`: the non-TTY confirmation guard threw a plain `Error`, giving CI a runtime-failure exit code for what is a usage error. It now throws `CloudRunUsageError` (exit 2). The class moved to its own module, since `run-in-cloud` imports `push-to-cloud` and defining it in either would make the pair circular; `run-in-cloud` re-exports it so callers are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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/cloud/src/blocks.test.ts`:
- Around line 190-206: Update the test “still times out when the caller supplies
its own signal” to keep the mocked fetch pending until init.signal aborts, then
advance timers beyond the 5 ms requestTimeoutMs deadline and assert the combined
signal aborts and the request is cancelled. Preserve the existing caller-abort
propagation assertion while exercising the timeout path.
🪄 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: a44af8bc-15df-4c81-aac5-eaae34434cbf
📒 Files selected for processing (8)
packages/cli/src/utils/cloud-run-usage-error.tspackages/cli/src/utils/push-to-cloud.test.tspackages/cli/src/utils/push-to-cloud.tspackages/cli/src/utils/run-in-cloud.tspackages/cloud/src/blocks.test.tspackages/cloud/src/http.tspackages/local-runner/src/sync-notebook-content.test.tspackages/local-runner/src/sync-notebook-content.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/cloud/src/http.ts
- packages/cli/src/utils/push-to-cloud.ts
- packages/cli/src/utils/push-to-cloud.test.ts
- packages/cli/src/utils/run-in-cloud.ts
Five of the eight were still valid; three were already fixed in 08a8308. - `sync-notebook-content.ts`: replaced three `as BlockSpec` casts with a `specFor` lookup that throws naming the block, instead of deferring the failure to an `undefined.type` several lines later. - `blocks.ts`: `typeof [] === 'object'`, so an array metadata slipped through the cast to `Record<string, unknown>`. Guarded with `Array.isArray`. - `packages/cloud/README.md`: the Types row omitted every type this PR added. - `block-spec.test.ts` (new): the extracted module had no tests of its own, and its UUID-validation/warning branch was only covered incidentally through two call sites. 11 cases over `toBlockSpec` and `mapBlockIds`. - `push-to-cloud.test.ts`: added the missing failure-path case — a sync that rejects mid-push must rethrow, which is what a user hits on a 403 partway through. Skipped as already addressed: the O(n²) `includes` in `filter` (now a Set lookup), the missing 403 tests, and the creates-plus-moves ordering test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@coderabbitai Worked through the 8 nitpicks in 7bf0ccf. Five were still valid; three were already handled in 08a8308. Fixed
Already fixed in 08a8308, skipping
2739 tests passing across the stack, typecheck/lint/spell clean. Both PRs staying in draft: nothing here has been run against a live workspace yet, which remains the main open risk on #432. |
|
Tip For best results, initiate chat on the files or code changes.
Keeping |
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/local-runner/src/block-spec.test.ts`:
- Around line 5-6: Update the block fixture helper and its tests around block()
and toBlockSpec to use a type-safe metadata shape that can represent arrays
without double-casting the entire DeepnoteBlock. Add an invalid-metadata test
passing an array and assert that toBlockSpec rejects it, ensuring arrays are not
passed through as valid metadata.
🪄 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: 3db366a2-0f7f-432e-ad7a-c3b8e75abfbb
📒 Files selected for processing (5)
packages/cli/src/utils/push-to-cloud.test.tspackages/cloud/README.mdpackages/cloud/src/blocks.tspackages/local-runner/src/block-spec.test.tspackages/local-runner/src/sync-notebook-content.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/cloud/README.md
- packages/local-runner/src/sync-notebook-content.ts
- packages/cli/src/utils/push-to-cloud.test.ts
- packages/cloud/src/blocks.ts
- blocks.ts: CRUD client for /v2/blocks and /v2/notebooks endpoints - block-spec.ts: convert .deepnote blocks to API-ready BlockSpec - sync-notebook-content.ts: diff local vs remote blocks, plan minimal mutations using longest-increasing-subsequence for reorder moves - push-to-cloud.ts: CLI orchestration for --push flag - Wire up exports from @deepnote/cloud and @deepnote/local-runner Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f650d61 to
40b3273
Compare
|
|
||
| let result: SyncResult | ||
| try { | ||
| result = await syncNotebookContent(file, localNotebookId, notebookId, { |
There was a problem hiding this comment.
(agent observation) the approved plan isn't the applied plan: syncNotebookContent re-plans after the confirmation prompt, so unapproved deletes can execute and all read requests happen twice. Make syncNotebookContent accept the DetailedSyncPlan the caller already has (the type exists for exactly this), and fix the now-inaccurate comment at push-to-cloud.ts:92-94.
| } | ||
|
|
||
| const contentChanged = (spec.content ?? '') !== found.content | ||
| const integrationChanged = detail !== undefined && (spec.integrationId ?? undefined) !== detail.integrationId |
There was a problem hiding this comment.
(agent observation) a remote-bound/local-unbound integration is flagged as changed on every push but the patch can never send the change: perpetual phantom "integration changed" update with a no-op content PATCH, entered by following the tool's own warning advice. Either ignore local-undefined (match metadataMatches semantics) or support clearing
voyti
left a comment
There was a problem hiding this comment.
Additional small suggestion about PR description — note that --push still throws "not yet implemented" after this merges (run-in-cloud.ts:307), and that the wiring PR must also drop dryRun from INCOMPATIBLE_FLAGS (run-in-cloud.ts:76) for the documented --dry-run preview to work
Summary
Adds
deepnote run --cloud --pushto sync local.deepnoteblocks to the Deepnote notebook before running it, so the run executes what is on disk rather than what was last saved in Deepnote.Stacked on #448 (cloud run reliability + storage-mode) — merge that first.
New modules
@deepnote/cloudblocks.ts/v2/blocksand/v2/notebooksendpoints@deepnote/local-runnerblock-spec.ts.deepnoteblocks to API-readyBlockSpec@deepnote/local-runnersync-notebook-content.ts@deepnote/clipush-to-cloud.ts--pushflagHow it works
.deepnotefile and the remote notebook's blocks--yes)Test plan
deepnote run --cloud --push --dry-run notebook.deepnotedeepnote run --cloud --push --yes notebook.deepnote🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests