Skip to content

feat(cli): add --session to run a notebook in an interactive session - #433

Open
jamesbhobbs wants to merge 9 commits into
mainfrom
feat/cli-sessions
Open

feat(cli): add --session to run a notebook in an interactive session#433
jamesbhobbs wants to merge 9 commits into
mainfrom
feat/cli-sessions

Conversation

@jamesbhobbs

@jamesbhobbs jamesbhobbs commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

What

deepnote run --cloud triggers a one-shot run of the notebook in your workspace. --session instead runs it in an interactive session, which executes its own copy, so the notebook's content is never modified — no outputs written back, no execution counts bumped.

DEEPNOTE_TOKEN=... deepnote run my-project.deepnote --cloud --session

What it isolates, and what it doesn't

The guarantee is narrower than it first sounds, and the docs now say so literally. Verified against a live workspace:

isolated?
Notebook blocks / content ✅ byte-identical before and after
Notebook updatedAt ✅ unchanged
Project files ⚠️ shared — the CLI passes storageMode: readonly, so writes fail, but reads hit the same files
Databases, integrations, external APIs ❌ fully live
lastRunAt / lastRunId on the source notebook updated
The source notebook's run history the run appears there

So --session means "do not change the notebook's content", not "do this unobserved" — anyone looking at the notebook will see it was just run.

When to reach for it

  • Running a shared or production notebook to see what it does, without leaving your outputs on it.
  • Colleagues have it open and you'd rather not change the code or outputs under them.
  • CI smoke checks — "does this still execute?" on a schedule, without the workspace copy changing each run.

Use plain --cloud when you want the run recorded against the notebook itself. That stays the normal case.

Limitations

  • Not faster. The CLI creates a session, runs, stops it — nothing is reused between invocations. The warm kernel only benefits programmatic callers holding a session across submissions (a @deepnote/cloud capability, not a CLI one).
  • Doesn't push local edits. It isolates execution, not content.
  • --block is rejected with --session. See below.

Why --session --block is refused

A session starts running the whole notebook the moment it's created, and the API offers no way to create an idle one. Interrupting that and submitting a single block does work mechanically — which is exactly why it's refused rather than shipped. Measured on a probe notebook whose first block sets a variable:

result
--cloud --block <third block> leftover state = NONE — clean kernel, as --block means
session, interrupted immediately, then that block leftover state = BLOCK1_RAN — the first block had already run

The interrupt was issued in the same instant as session creation and the first block still finished. How much of the notebook gets in depends on how fast its early blocks are, so the same command answers differently on different runs — silently, because the block still succeeds, just against a kernel other blocks have written to. Waiting turns a visible race into an invisible one.

Translating block ids is not the blocker: every block in the session copy carries metadata.source_block_id, so that part is a lookup. The blocker is ordering. One field on POST /v2/sessions (autoRun: false, or accepting blockIds) would make this correct and trivial; today that endpoint is additionalProperties: false over {notebookId, inputs, storageMode} with no kernel-reset endpoint anywhere. The reasoning is recorded at the guard, in the CLI reference, and in sessions.ts.

API surface

Every endpoint is in the public spec today (api.deepnote.com/v2/openapi.json, Deepnote Public API 2.0.0) — nothing here waits on a backend:

Export Endpoint
createSession POST /v2/sessions
submitSessionRun POST /v2/sessions/{id}/runs
getSessionStatus GET /v2/sessions/{id}/status
interruptSession POST /v2/sessions/{id}/interrupt
stopSession DELETE /v2/sessions/{id}

POST /v2/sessions/{id}/execute (arbitrary code in the kernel, documented as a debug endpoint) is deliberately not wrapped.

Worth reviewing

Session lifecycle. A session holds a live machine, so stopSession runs on every exit path — success, failure, and SIGINT/SIGTERM. Signal cleanup runs on a 5s deadline (not the default 30s, which reads as a hang) and prints which session it's stopping plus that a second Ctrl-C skips the wait. A cleanup timeout is reported as "not confirmed, almost certainly stopped" rather than as a failure: stopping a session means shutting a machine down, and it routinely outlasts that deadline while still succeeding.

http.ts — the one piece of scope beyond --session. sessions.ts needs request plumbing and packages/cloud already had two near-identical private copies (cloud-runs.ts, create-project.ts), so rather than add a third this extracts the schema-validated request helper into http.ts. It fixes two defects in that copy:

  • forbiddenMessage was unreachable — parseApiErrorMessage falls back to "<fallback>: HTTP <status>" and never returns empty, so the || chain never reached it.
  • signal ?? timeout dropped the deadline for any caller passing a signal, so those requests could hang forever.

The deadline is wired by hand rather than with AbortSignal.any([caller, timeout]), which has a history of dropping the timeout when the composed signal is collected mid-flight (nodejs/node#57736). Doing it manually avoids pinning a Node floor on every consumer of the package, and lets the timer be cleared once the request settles instead of leaving one pending per request. http.ts also documents the real error union — timeouts, aborts and network failures escape as themselves, so catching only ApiError misses them.

Verification

Live workspace, end to end. Every path below was run against a real workspace using a disposable project, since a session executes real code:

Path Result
--session happy path ✅ run pollable via GET /v2/runs/{id}, snapshot downloaded with the right output
Source notebook isolation ✅ blocks and updatedAt byte-identical; lastRunAt/history do change (see above)
Notebook that raises ✅ exit 1, partial snapshot with the real traceback
-o json ✅ stdout stays pure JSON
--input ✅ override reaches the run
--notebook-id, no local file
storageMode: readonly ✅ proven A/B — same write fails under --session, succeeds without
Ctrl-C mid-run ✅ exit 130, session stopped, ~5s
Session cleanup /status returns 404 afterwards

Automated. pnpm test — 2686 passed, 1 skipped, exit 0. Typecheck, biome, prettier, cspell clean. 16 sessions-client tests, 17 http.ts tests, 35 CLI tests. The timeout tests use a fetch that stays pending until its signal aborts, verified to fail against signal ?? timeout rather than passing regardless.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds shared authenticated HTTP handling with deadlines, error mapping, and schema validation. It introduces typed interactive-session APIs and exports them from the Cloud package. The CLI gains --session parsing, completion, validation, documentation, and cloud execution support. Session runs create an isolated notebook copy, poll the initial run, handle signals, and clean up on success or failure. Tests cover HTTP behavior, session APIs, CLI integration, and lifecycle handling.

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

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant runInDeepnoteCloud
  participant DeepnoteSessionsAPI
  CLI->>runInDeepnoteCloud: invoke cloud run with --session
  runInDeepnoteCloud->>DeepnoteSessionsAPI: create session
  DeepnoteSessionsAPI-->>runInDeepnoteCloud: initial run
  runInDeepnoteCloud->>DeepnoteSessionsAPI: poll initial run
  runInDeepnoteCloud->>DeepnoteSessionsAPI: stop session
Loading

Possibly related PRs

Suggested reviewers: dinohamzic

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 57.69% 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
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 PASS: OSS docs now cover --session in the CLI and cloud READMEs, plus the cli-run reference; I couldn’t inspect the private landing-page repo, so please update it separately.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding --session support to deepnote run for interactive sessions.

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

@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.18310% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.54%. Comparing base (d339651) to head (99b32ce).

Files with missing lines Patch % Lines
packages/cli/src/utils/run-in-cloud.ts 91.83% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #433      +/-   ##
==========================================
+ Coverage   87.36%   87.54%   +0.18%     
==========================================
  Files         181      183       +2     
  Lines        9494     9609     +115     
  Branches     2699     2729      +30     
==========================================
+ Hits         8294     8412     +118     
+ Misses       1199     1196       -3     
  Partials        1        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: 4

🤖 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/run-in-cloud.test.ts`:
- Around line 258-268: Update the test “does not ask before pushing into a
session copy, which is discarded anyway” to explicitly mock or pin
process.stdin.isTTY to false for the test duration. Restore the original value
afterward so the test remains isolated, ensuring promptForBooleanField cannot
depend on the runner’s TTY state.

In `@packages/cli/src/utils/run-in-cloud.ts`:
- Around line 388-397: Move the createSession flow in the options.session branch
into the existing try/catch or otherwise ensure its rejection reaches the same
error-handling path that calls spinner?.fail. Preserve session and sessionRun
assignment and debug logging on success, while stopping the spinner when
createSession fails.
- Around line 415-430: Update the push target and confirmation logic around
pushLocalNotebook so a session is treated as safe only when
session?.sessionNotebookId is present. When that identifier is absent, target
the real notebook and keep confirmation enabled; only set skipConfirmation when
pushing to a valid session copy.
- Around line 455-458: Update the session submission flow in run-in-cloud.ts so
the submitSessionRun call in the options.push/blockIds branch includes the
parsed inputs alongside notebookId and blockIds. Preserve inputs for --session
--push and --session --block, and refactor the non-submission branch to avoid
the SubmittedRun cast by retaining the session/run pair together.
🪄 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: cc9f9be7-fbbb-4f79-a09d-8537db4c93a3

📥 Commits

Reviewing files that changed from the base of the PR and between befbb2c and 05a1f3b.

📒 Files selected for processing (14)
  • packages/cli/README.md
  • packages/cli/src/cli.test.ts
  • packages/cli/src/cli.ts
  • packages/cli/src/commands/run.ts
  • packages/cli/src/completions.ts
  • packages/cli/src/utils/push-to-cloud.test.ts
  • packages/cli/src/utils/push-to-cloud.ts
  • packages/cli/src/utils/run-in-cloud.test.ts
  • packages/cli/src/utils/run-in-cloud.ts
  • packages/cloud/README.md
  • packages/cloud/src/index.ts
  • packages/cloud/src/sessions.test.ts
  • packages/cloud/src/sessions.ts
  • skills/deepnote/references/cli-run.md

Comment thread packages/cli/src/utils/run-in-cloud.test.ts Outdated
Comment thread packages/cli/src/utils/run-in-cloud.ts
Comment thread packages/cli/src/utils/run-in-cloud.ts Outdated
Comment thread packages/cli/src/utils/run-in-cloud.ts Outdated
jamesbhobbs added a commit that referenced this pull request Jul 26, 2026
- **Critical:** `sessionNotebookId` is optional on the API's session object, and
  `pushTarget` fell back to the *source* notebook when it was absent — while
  `skipConfirmation` stayed true because a session existed. That is an
  unconfirmed destructive sync into the user's real notebook, which is the one
  outcome `--session` exists to rule out. It now refuses with a usage error, and
  a test asserts no block write is issued.
- `submitSessionRun` dropped `inputs`, so `-i key=value` was lost on
  `--session --push`: the session was created with them, but the post-push
  submission is a new run and does not inherit them.
- A `createSession` failure escaped with the ora spinner still running, unlike
  every other failure path.
- The "does not ask before pushing into a session copy" test relied on the
  runner having no TTY. On a machine where Vitest inherits one it would have
  reached the real prompt and hung rather than failed, so it now pins
  `stdin.isTTY` and restores it.

Also drops an avoidable `as SubmittedRun` cast by narrowing on the session/run
pair together.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jamesbhobbs added a commit that referenced this pull request Jul 26, 2026
- **Critical:** `sessionNotebookId` is optional on the API's session object, and
  `pushTarget` fell back to the *source* notebook when it was absent — while
  `skipConfirmation` stayed true because a session existed. That is an
  unconfirmed destructive sync into the user's real notebook, which is the one
  outcome `--session` exists to rule out. It now refuses with a usage error, and
  a test asserts no block write is issued.
- `submitSessionRun` dropped `inputs`, so `-i key=value` was lost on
  `--session --push`: the session was created with them, but the post-push
  submission is a new run and does not inherit them.
- A `createSession` failure escaped with the ora spinner still running, unlike
  every other failure path.
- The "does not ask before pushing into a session copy" test relied on the
  runner having no TTY. On a machine where Vitest inherits one it would have
  reached the real prompt and hung rather than failed, so it now pins
  `stdin.isTTY` and restores it.

Also drops an avoidable `as SubmittedRun` cast by narrowing on the session/run
pair together.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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

🤖 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/run-in-cloud.test.ts`:
- Around line 222-241: Update the test case around runInDeepnoteCloud to capture
the POST /v2/blocks request index from calls and assert it occurs before the
POST /v2/sessions/session-1/runs submission. Keep the existing creation,
interruption, and copy-read ordering assertions unchanged.
- Around line 212-220: Update runInDeepnoteCloud so the initial session-creation
run is interrupted whenever a block-scoped replacement run will be submitted,
including the --session --block path rather than only --push. Preserve the
existing no-replacement behavior, and extend the tests around the session run
assertions to verify interruption and prevent the unscoped run from executing
the whole notebook.
🪄 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: fec3ea13-891d-4ee8-aa7b-3263b8ecd986

📥 Commits

Reviewing files that changed from the base of the PR and between 3a24eb7 and 1aa007e.

📒 Files selected for processing (14)
  • packages/cli/README.md
  • packages/cli/src/cli.test.ts
  • packages/cli/src/cli.ts
  • packages/cli/src/commands/run.ts
  • packages/cli/src/completions.ts
  • packages/cli/src/utils/push-to-cloud.test.ts
  • packages/cli/src/utils/push-to-cloud.ts
  • packages/cli/src/utils/run-in-cloud.test.ts
  • packages/cli/src/utils/run-in-cloud.ts
  • packages/cloud/README.md
  • packages/cloud/src/index.ts
  • packages/cloud/src/sessions.test.ts
  • packages/cloud/src/sessions.ts
  • skills/deepnote/references/cli-run.md
🚧 Files skipped from review as they are similar to previous changes (11)
  • packages/cloud/src/index.ts
  • packages/cli/src/commands/run.ts
  • packages/cli/src/cli.ts
  • packages/cli/src/utils/push-to-cloud.ts
  • packages/cli/src/completions.ts
  • skills/deepnote/references/cli-run.md
  • packages/cloud/README.md
  • packages/cloud/src/sessions.ts
  • packages/cli/README.md
  • packages/cli/src/utils/run-in-cloud.ts
  • packages/cloud/src/sessions.test.ts

Comment thread packages/cli/src/utils/run-in-cloud.test.ts Outdated
Comment thread packages/cli/src/utils/run-in-cloud.test.ts Outdated
jamesbhobbs added a commit that referenced this pull request Jul 26, 2026
- **Critical:** `sessionNotebookId` is optional on the API's session object, and
  `pushTarget` fell back to the *source* notebook when it was absent — while
  `skipConfirmation` stayed true because a session existed. That is an
  unconfirmed destructive sync into the user's real notebook, which is the one
  outcome `--session` exists to rule out. It now refuses with a usage error, and
  a test asserts no block write is issued.
- `submitSessionRun` dropped `inputs`, so `-i key=value` was lost on
  `--session --push`: the session was created with them, but the post-push
  submission is a new run and does not inherit them.
- A `createSession` failure escaped with the ora spinner still running, unlike
  every other failure path.
- The "does not ask before pushing into a session copy" test relied on the
  runner having no TTY. On a machine where Vitest inherits one it would have
  reached the real prompt and hung rather than failed, so it now pins
  `stdin.isTTY` and restores it.

Also drops an avoidable `as SubmittedRun` cast by narrowing on the session/run
pair together.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jamesbhobbs added a commit that referenced this pull request Jul 26, 2026
- **Critical:** `sessionNotebookId` is optional on the API's session object, and
  `pushTarget` fell back to the *source* notebook when it was absent — while
  `skipConfirmation` stayed true because a session existed. That is an
  unconfirmed destructive sync into the user's real notebook, which is the one
  outcome `--session` exists to rule out. It now refuses with a usage error, and
  a test asserts no block write is issued.
- `submitSessionRun` dropped `inputs`, so `-i key=value` was lost on
  `--session --push`: the session was created with them, but the post-push
  submission is a new run and does not inherit them.
- A `createSession` failure escaped with the ora spinner still running, unlike
  every other failure path.
- The "does not ask before pushing into a session copy" test relied on the
  runner having no TTY. On a machine where Vitest inherits one it would have
  reached the real prompt and hung rather than failed, so it now pins
  `stdin.isTTY` and restores it.

Also drops an avoidable `as SubmittedRun` cast by narrowing on the session/run
pair together.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 26, 2026
@jamesbhobbs
jamesbhobbs changed the base branch from feat/cli-push-blocks to main July 27, 2026 16:14
@jamesbhobbs
jamesbhobbs dismissed coderabbitai[bot]’s stale review July 27, 2026 16:14

The base branch was changed.

@jamesbhobbs jamesbhobbs changed the title feat(cli): add --session to run in an interactive session on a scratch copy feat(cli): add --session to run a notebook in an interactive session Jul 27, 2026

@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: 3

🧹 Nitpick comments (2)
packages/cli/src/utils/run-in-cloud.test.ts (1)

235-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Nothing pins which run id gets polled.

The fallback mock answers every GET with run-auto, so a regression that polled the session's initial run instead of the block-scoped replacement would still pass. Asserting the polled URL carries run-scoped closes that.

♻️ Suggested addition
     expect(calls.find(c => c.url.endsWith('/v2/sessions/session-1/runs'))?.body?.blockIds).toEqual(['blk-1'])
+    // The replacement run is the one reported, so it is the one that must be polled.
+    expect(calls.some(c => c.method === 'GET' && c.url.includes('run-scoped'))).toBe(true)
🤖 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/run-in-cloud.test.ts` around lines 235 - 251,
Strengthen the test around runInDeepnoteCloud by asserting that the polling
request targets the block-scoped replacement run, specifically that its URL
includes the run-scoped identifier such as run-scoped. Update the session fetch
mock or call inspection so polling the initial run cannot satisfy the test,
while preserving the existing interrupt-before-submit and blockIds assertions.
packages/cloud/src/http.ts (1)

5-13: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Docstring overstates the error contract.

The module doc says every failure becomes an ApiError, but fetch() itself (line 76) and response.text() (line 90) aren't wrapped — network failures, aborts, and timeouts propagate as native TypeError/DOMException, not ApiError (confirmed by the test expecting { name: 'TimeoutError' } at http.test.ts line 162). A caller catching only ApiError would miss these. Either narrow the docstring's claim to "every response received" or wrap transport-level failures too.

Also applies to: 74-108

🤖 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/http.ts` around lines 5 - 13, The module docstring
overstates the error contract: update the documentation around the shared HTTP
request flow to limit the ApiError guarantee to failures from received
responses, explicitly excluding transport failures from fetch and response.text
such as network errors, aborts, and timeouts. Do not change the existing error
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/cloud/src/http.ts`:
- Around line 40-49: Update the package’s Node.js engine requirement from
>=22.14.0 to >=22.16.0 so combineSignals can safely use AbortSignal.any with
AbortSignal.timeout. Ensure all relevant package metadata and supported-runtime
declarations advertise the new floor consistently.

In `@skills/deepnote/references/cli-run.md`:
- Line 28: Update the `--session` option description in the CLI reference table
to state that it runs only with `--cloud`, preserving the existing behavior
description while adding “(with --cloud)”.
- Around line 114-115: Update the documentation around the --session behavior to
describe both supported workflows: use deepnote sync to push local edits to the
regular saved content, or combine --session with --push to synchronize edits
into the disposable session copy before execution.

---

Nitpick comments:
In `@packages/cli/src/utils/run-in-cloud.test.ts`:
- Around line 235-251: Strengthen the test around runInDeepnoteCloud by
asserting that the polling request targets the block-scoped replacement run,
specifically that its URL includes the run-scoped identifier such as run-scoped.
Update the session fetch mock or call inspection so polling the initial run
cannot satisfy the test, while preserving the existing interrupt-before-submit
and blockIds assertions.

In `@packages/cloud/src/http.ts`:
- Around line 5-13: The module docstring overstates the error contract: update
the documentation around the shared HTTP request flow to limit the ApiError
guarantee to failures from received responses, explicitly excluding transport
failures from fetch and response.text such as network errors, aborts, and
timeouts. Do not change the existing error 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: 87b1e286-6ba1-4aed-b535-9bc6fa3de5b2

📥 Commits

Reviewing files that changed from the base of the PR and between 1aa007e and e35a3d5.

📒 Files selected for processing (15)
  • packages/cli/README.md
  • packages/cli/src/cli.test.ts
  • packages/cli/src/cli.ts
  • packages/cli/src/commands/run.ts
  • packages/cli/src/completions.ts
  • packages/cli/src/utils/run-in-cloud.test.ts
  • packages/cli/src/utils/run-in-cloud.ts
  • packages/cloud/README.md
  • packages/cloud/src/create-project.ts
  • packages/cloud/src/http.test.ts
  • packages/cloud/src/http.ts
  • packages/cloud/src/index.ts
  • packages/cloud/src/sessions.test.ts
  • packages/cloud/src/sessions.ts
  • skills/deepnote/references/cli-run.md
🚧 Files skipped from review as they are similar to previous changes (9)
  • packages/cli/src/commands/run.ts
  • packages/cloud/src/index.ts
  • packages/cli/src/cli.ts
  • packages/cli/README.md
  • packages/cli/src/cli.test.ts
  • packages/cli/src/completions.ts
  • packages/cloud/README.md
  • packages/cloud/src/sessions.test.ts
  • packages/cloud/src/sessions.ts

Comment thread packages/cloud/src/http.ts
Comment thread skills/deepnote/references/cli-run.md Outdated
Comment thread skills/deepnote/references/cli-run.md Outdated
jamesbhobbs and others added 3 commits July 27, 2026 17:39
`deepnote run --cloud` triggers a one-shot run of the notebook in your
workspace. `--session` instead runs it in an interactive session, which
executes its **own copy** — the notebook in the workspace is never modified —
and keeps the kernel warm between submissions.

    deepnote run my-project.deepnote --cloud --session

All six endpoints are in the live public API (Deepnote Public API 2.0.0):
POST /v2/sessions, POST/GET on {id}/runs, /status, /interrupt, and
DELETE /v2/sessions/{id}. Nothing here is speculative.

@deepnote/cloud
- New `sessions.ts`: createSession, submitSessionRun, getSessionStatus,
  interruptSession, stopSession. `stopSession` treats a 404 as success — it is
  cleanup, usually on a failure path, and "the session is gone" is what was
  asked for. `/execute` (arbitrary code, documented as debug) is deliberately
  not wrapped.
- New `http.ts`: the schema-validated `request` helper, extracted from
  create-project.ts so sessions.ts does not add a third copy of the same
  plumbing. It also fixes two things in that copy: `forbiddenMessage` was
  unreachable (`parseApiErrorMessage` never returns empty), and
  `signal ?? timeout` dropped the deadline for any caller passing a signal, so
  those requests could hang forever.

@deepnote/cli
- `--session` runs via a session and always stops it, including when the run
  fails, so no machine is left holding resources.
- Creating a session immediately starts a run of the whole copy. With `--block`
  that run is interrupted and a block-scoped one submitted in its place, or the
  notebook would execute in full alongside the run being reported. The
  interrupt and the submission share one condition so they cannot drift apart.
- Inputs are repeated on a block-scoped submission: a new submission does not
  inherit the ones the session was created with.

Session runs are polled with the existing getRun/pollRunUntilComplete, on the
assumption that a session run is an ordinary run addressable by id. That
assumption, like the rest of this, is verified against the OpenAPI spec and
stubbed fetch — not a live workspace.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The docs explained the mechanism but never said why anyone would use it.

- cli-run.md: the cases it is for (running someone else's or a shared notebook,
  colleagues have it open, CI smoke checks, scratch execution of one block),
  when to use plain --cloud instead (you want the run recorded), and two things
  it explicitly does not do — it is not faster, since the CLI creates and stops
  a session per invocation, and it does not push local edits.
- cloud README: separates the two properties, since they serve different
  callers — own-copy isolation, and a warm kernel that only a caller holding a
  session across submissions benefits from. Adds a worked example of the
  interactive-tool case (submit, poll status, interrupt, resubmit, stop).
- sessions.ts module doc: the same split, where a maintainer will see it.

Claims stay inside what the spec documents: sessions guarantee the source
notebook is never modified. `POST /v2/runs` has no description in the spec, so
nothing is asserted about what a plain run does to the notebook.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every finding checked against the live OpenAPI spec first; all six held up.

**--session --block is removed, not fixed.** The spec is explicit that
`blockIds` must belong to the session's copy and that "source-notebook block
IDs are rejected", so the flag was forwarding ids the API refuses. Translating
them would mean pairing blocks across two notebooks positionally — guesswork —
and it would not help anyway: creating a session immediately starts a run of
the whole copy, so interrupting it races that run. A single-block session run
cannot be guaranteed until the API can create an idle session. The combination
is now rejected with a usage error saying why, which also removes the
swallowed-interrupt path entirely.

**storageMode: readonly.** The API defaults to `read_write`, so the CLI was
letting a session write to project storage while claiming isolation. A session
isolates the notebook document and nothing else.

**Cleanup survives Ctrl-C.** SIGINT/SIGTERM handlers stop the session and exit
128+signal; they are removed on every normal path so they cannot leak. A failed
DELETE now warns on stderr instead of hiding behind --debug — the session
outlives the command and costs money until it expires.

**Narrowed the claims.** "Safe to run someone else's notebook" and "invisible
to everyone else" were both wrong. The guarantee covers the notebook document:
databases, integrations and external APIs are still live, and the spec says
each run persists as a run snapshot, so it is recorded. Docs now say what is
and is not protected.

**http.ts error contract.** It promised ApiError for every failure, but
timeouts, aborts and network failures escape as themselves. Documented the
actual union rather than normalizing, since TimeoutError vs AbortError is
information a caller wants.

**Tests model the contract.** The session mock now rejects source-domain block
ids, echoes back whichever run was polled so the wrong run cannot pass, and
asserts storageMode, signal-handler cleanup, and the stop-failure warning.

Still unverified against a live workspace.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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: 3

🧹 Nitpick comments (2)
packages/cli/src/utils/run-in-cloud.ts (1)

349-403: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Handler leak on the createSession-throws path is avoided, but consider process.once. Minor hardening: a second Ctrl-C during endSession() re-enters nothing today (handlers are removed first), so this is fine as-is — noting only that once would express the intent.

🤖 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/run-in-cloud.ts` around lines 349 - 403, Update
installSignalHandlers to register each SIGINT and SIGTERM callback with
process.once instead of process.on, while preserving removeSignalHandlers and
the existing endSession cleanup and exit-code behavior.
packages/cli/src/utils/run-in-cloud.test.ts (1)

137-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Dead mock branches. --session --block is now rejected up front, so the interrupt and scoped-run handlers are unreachable; the suite only asserts they are never hit. Keep them as executable documentation of the API contract, or drop them — your call.

🤖 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/run-in-cloud.test.ts` around lines 137 - 152, The
interrupt and scoped-run mock handlers in the test are unreachable because
--session --block is rejected before those requests. Remove these dead branches,
or retain them only if the tests explicitly exercise them to document the API
contract; do not leave unreachable handlers that are merely asserted not to run.
🤖 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/README.md`:
- Line 80: Add getNotebook to the import block in the README example so the
existing call using session.sessionNotebookId resolves correctly when copied.
- Around line 67-68: Update the README wording near the storageMode discussion
to explicitly distinguish notebook-document isolation from shared project files
and external systems: state that the notebook document is isolated, while shared
files, databases, integrations, and other systems it accesses may remain
accessible or be affected. Keep the existing run-snapshot behavior intact.

In `@skills/deepnote/references/cli-run.md`:
- Line 28: Update the --session CLI documentation row to state that it requires
--cloud, matching the behavior enforced by assertCloudOnlyFlagsRequireCloud and
the neighboring cloud-only option descriptions.

---

Nitpick comments:
In `@packages/cli/src/utils/run-in-cloud.test.ts`:
- Around line 137-152: The interrupt and scoped-run mock handlers in the test
are unreachable because --session --block is rejected before those requests.
Remove these dead branches, or retain them only if the tests explicitly exercise
them to document the API contract; do not leave unreachable handlers that are
merely asserted not to run.

In `@packages/cli/src/utils/run-in-cloud.ts`:
- Around line 349-403: Update installSignalHandlers to register each SIGINT and
SIGTERM callback with process.once instead of process.on, while preserving
removeSignalHandlers and the existing endSession cleanup and exit-code 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: d5dbcb70-a9fd-4a99-9023-d9f544ca51bd

📥 Commits

Reviewing files that changed from the base of the PR and between 67cf338 and 30325f0.

📒 Files selected for processing (15)
  • packages/cli/README.md
  • packages/cli/src/cli.test.ts
  • packages/cli/src/cli.ts
  • packages/cli/src/commands/run.ts
  • packages/cli/src/completions.ts
  • packages/cli/src/utils/run-in-cloud.test.ts
  • packages/cli/src/utils/run-in-cloud.ts
  • packages/cloud/README.md
  • packages/cloud/src/create-project.ts
  • packages/cloud/src/http.test.ts
  • packages/cloud/src/http.ts
  • packages/cloud/src/index.ts
  • packages/cloud/src/sessions.test.ts
  • packages/cloud/src/sessions.ts
  • skills/deepnote/references/cli-run.md
🚧 Files skipped from review as they are similar to previous changes (10)
  • packages/cloud/src/index.ts
  • packages/cli/README.md
  • packages/cli/src/completions.ts
  • packages/cli/src/commands/run.ts
  • packages/cli/src/cli.ts
  • packages/cloud/src/http.test.ts
  • packages/cloud/src/create-project.ts
  • packages/cloud/src/http.ts
  • packages/cloud/src/sessions.test.ts
  • packages/cloud/src/sessions.ts

Comment thread packages/cloud/README.md Outdated
Comment thread packages/cloud/README.md
Comment thread skills/deepnote/references/cli-run.md Outdated
`AbortSignal.any([caller, AbortSignal.timeout(ms)])` has a history of dropping
the timeout: the composed signal can be collected while the request is in
flight, so the deadline never fires and the request hangs — the exact failure
the deadline exists to prevent (nodejs/node#57736, fixed by nodejs/node#57867).

Raising the package's Node floor would fix it, but that is a breaking
constraint on every consumer to work around an implementation detail. Wiring
the controller by hand behaves identically on every supported version, and
lets the timer be cleared once the request settles rather than leaving one
pending per request for its full timeout. The body read moved inside the
deadline too — headers can arrive promptly while the body stalls.

Also from review:

- Docs: `--session` is cloud-only, so the flag table says so.
- Docs: the isolation wording implied `storageMode: 'readonly'` removed
  project-file interaction. It stops *writes*; the session still reads the same
  shared files. Split into what is and is not isolated.
- Docs: the sessions example called `getNotebook` without importing it, and
  passed the optional `sessionNotebookId` where a string is required — neither
  would have compiled if copied.
- Tests: the pending-fetch helper now rejects immediately for an
  already-aborted signal, as real fetch does. Added cases for timer cleanup on
  both the success and failure paths, and for an already-aborted caller signal.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 27, 2026
Ran `deepnote run examples/1_hello_world.deepnote --cloud --session` against a
real workspace. The core flow works — session created, run pollable via
GET /v2/runs/{id}, snapshot downloaded with the right output, session stopped
(subsequent status returns 404). The blocks and `updatedAt` on the source
notebook were byte-identical before and after.

But two documented claims were wrong:

- The source notebook's `lastRunAt` and `lastRunId` **do** change — they point
  at the session's run. Only the notebook's *content* is isolated.
- The run appears in the *source* notebook's run history
  (GET /v2/notebooks/{id}/runs), attributed to the session's copy. So anyone
  looking at the notebook can see it was just run.

The docs said the run 'leaves that document untouched' and framed the
not-private caveat as an inference from the spec's 'each run persists as a run
snapshot'. Both now say what was actually observed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jamesbhobbs

Copy link
Copy Markdown
Contributor Author

Verified against a live workspace

Ran it for real (workspace Deepnote, examples/1_hello_world.deepnoteprint("Hello world!"), no integrations). This was the outstanding item keeping the PR in draft.

The critical assumption holds. A session run is pollable via GET /v2/runs/{id}:

[debug] Created session 2260ab93-… (copy 82464908d08a4c9a8917e35ea803c163)
[debug] Started run 32ee15b3-… for notebook 7061f86dec6e4e11893288f295a82017
✓ Run 32ee15b3-… completed (success)
Snapshot saved to examples/snapshots/hello-world_…_latest.snapshot.deepnote

Exit 0. Snapshot contained the real Hello world! stdout. Session cleanup worked — GET /v2/sessions/{id}/status afterwards returns 404. The --session --block guard refuses before making any API call.

Two documented claims were wrong, corrected in 3621afc.

Source notebook before vs after:

before after
blocks 1 block identical ✅ isolated
updatedAt 2025-11-04T05:24:57.117Z unchanged ✅ isolated
lastRunAt null 2026-07-27T17:29:38.031Z changed
lastRunId null 32ee15b3-… changed

And the run appears in the source notebook’s history:

GET /v2/notebooks/7061f86d…/runs
{"runs":[{"runId":"32ee15b3-…","notebookId":"82464908…","status":"success",}]}

— attributed to the copy, but listed under the source notebook. So the isolation covers the notebook’s content, not the fact that it ran: anyone looking at the notebook sees it was just run. The docs previously said the run "leaves that document untouched", and framed the not-private caveat as an inference from the spec rather than an observation. Both now state what was measured.

Test artifacts cleaned up; the repo is unchanged. The API key used for this has been reported back for revocation.

Nothing outstanding from my side now — this is ready to come out of draft whenever you are happy with it.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 27, 2026
Interrupting the CLI awaited stopSession on the default 30s request deadline,
so an unresponsive API made Ctrl-C look like a hang. Someone waiting on that
reaches for kill -9, which strands the machine anyway — the exact outcome the
handler exists to prevent.

Cleanup now runs on a 5s deadline, and the handler prints which session it is
stopping and that a second Ctrl-C skips the wait. That second press already
worked (the handler removes itself first, so it falls through to Node's
default), but nothing said so.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 27, 2026
…--block reasoning

Both found by testing against a live workspace rather than by reading.

**A cleanup timeout is not a cleanup failure.** With the 5s deadline the signal
path uses, `DELETE /v2/sessions/{id}` regularly times out — stopping a session
means shutting a machine down. The session was gone every time (`/status`
answered 404), but the CLI printed "could not stop … it will keep running;
stop it from the Deepnote UI", sending people to clean up something that had
already cleaned itself up. A timeout now says the stop was not confirmed and
almost certainly succeeded; genuine failures keep the strong warning.

**`--session --block` is refused for one reason, not two.** The code claimed
translating source block ids to the copy's would be positional guesswork. It
would not: every block in the session copy carries
`metadata.source_block_id` naming the block it came from, so the translation is
a lookup. The refusal still stands on the other ground — a session starts
running the whole notebook on creation, so interrupting it to run one block is
a race the notebook can win — but the stated reasoning was wrong and is now
accurate. Noted in `SubmitSessionRunBody.blockIds` for callers who do need the
mapping, flagged as observed-not-published.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jamesbhobbs

Copy link
Copy Markdown
Contributor Author

Second live pass — non-happy paths

Ran against a real workspace using a disposable project I created and deleted (zz-cli-session-test), so no existing notebooks were touched. Six paths, all previously unverified.

Path Result
--session + --notebook-id, no local file ✅ works
Notebook that raises ✗ error, exit 1, partial snapshot with the real ValueError traceback
-o json with --session ✅ stdout stays pure JSON — no session chatter leaks in
--input with --session greeting is OVERRIDDEN-VALUE — override reaches the run
storageMode: readonly proven by A/B, see below
Ctrl-C mid-run ✅ exit 130, session stopped, ~5s

readonly is real. Same notebook, same block (open("readonly_probe.txt","w")):

  • with --sessionOSError: [Errno 30] Read-only file system: 'readonly_probe.txt'
  • without --session → succeeds

Two defects this found — both mine, both fixed in 90a149d

1. The Ctrl-C fix I added yesterday was reporting false alarms. DELETE /v2/sessions/{id} routinely takes longer than the 5s signal deadline, because stopping a session means shutting a machine down. The session was gone every time (/status → 404), but the CLI printed "could not stop … it will keep running; stop it from the Deepnote UI". A timeout now says the stop was not confirmed and almost certainly worked; real failures keep the strong warning.

2. My justification for refusing --session --block was half wrong. I wrote that translating source block ids to the copy would be "positional guesswork". It is not — every block in the session copy carries metadata.source_block_id naming its origin, so it is a lookup. The refusal still stands, but on one ground rather than two: a session starts running the whole notebook on creation, so interrupting it to run a single block is a race the notebook can win. Corrected in the code comment and the docs, and noted in SubmitSessionRunBody.blockIds for anyone who does need the mapping (flagged as observed, not published schema).

Process note

The first Ctrl-C run appeared to pass — exit 130, session cleaned up — but the binary was stale: I had never rebuilt dist/ after the fix, so I was testing the previous version. The missing log line gave it away. Rebuilt and re-ran; that is when the false-alarm bug surfaced.

2686 tests green, exit 0. Scratch project deleted, repo clean. Everything I listed as unverified is now verified.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 27, 2026
Comments and prose only; no behaviour change.

Interrupting the session's automatic run, waiting for the kernel to report
idle, then submitting the block is the obvious fix, and it looks like it works
— the run succeeds. It is still wrong, and the failure is silent, so the
measurement is recorded where someone would go to "fix" this.

Probe notebook: [1] marker = "BLOCK1_RAN"  [2] sleep(120)  [3] print(marker)

  --session, interrupt in the same tick as create, then block 3
      -> "BLOCK3_RAN; leftover state = BLOCK1_RAN"
  --cloud --block 3, no session
      -> "BLOCK3_RAN; leftover state = NONE"

An instant interrupt still let block 1 finish, and its state leaked into the
block-scoped run. How much leaks depends on how fast the early blocks are, so
the same command answers differently on different runs while continuing to
report success. Waiting turns a visible race into an invisible one.

Waiting for the initial run to *finish* would be deterministic, but that is
"run everything, then re-run this block" — a different feature that should not
be spelled --block. The fix is upstream: a way to create a session that does
not auto-run. POST /v2/sessions is additionalProperties: false over
{notebookId, inputs, storageMode}, and there is no kernel-reset endpoint.

Noted in three places: the guard in run-in-cloud.ts, the CLI reference, and
sessions.ts for library callers, who can hit the same trap directly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jamesbhobbs
jamesbhobbs marked this pull request as ready for review July 27, 2026 18:24
@jamesbhobbs
jamesbhobbs requested a review from a team as a code owner July 27, 2026 18:24
@jamesbhobbs
jamesbhobbs requested review from dinohamzic and voyti July 27, 2026 18:24

@dinohamzic dinohamzic 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.

AI review before manual testing:

I’d request changes on PR #433. The internal comparison invalidates the CLI flag’s main premise and exposes two lifecycle blockers.

Findings

  1. [P1] Full-notebook --cloud already provides the advertised notebook isolation. cloud-runs.ts relies on the API’s default detached: true; internal then duplicates the notebooks and executes the copy. Both ordinary detached and session runs update source run history. The real unique behavior here is read-only project storage, which ordinary /v2/runs already supports via detachedRunStorageMode: 'readonly' (internal contract). Exposing that field would avoid an interactive session’s overhead, feature gate, and cleanup lifecycle. --cloud --block is the exception because it switches to live mode.

  2. [P1] Ctrl-C or timeout during session creation can orphan a machine. run-in-cloud.ts awaits createSession() before installing signal handlers or retaining the session ID. If the process exits or the 30-second HTTP deadline fires while internal is still copying/dispatching, no DELETE can be issued and the session runs until expiry.

  3. [P1] Snapshot handling can turn successful sessions into failures. Internal commits terminal status before asynchronously finalizing the snapshot, but the CLI performs only one immediate retry at run-in-cloud.ts. There is also a deterministic case: empty/markdown-only sessions create a successful no-op run with no snapshot at all. Both paths become exit 1. Snapshot availability needs bounded polling, plus an explicit no-op artifact policy.

  4. [P2] The HTTP extraction leaves the exact timeout bug it claims to fix. cloud-runs.ts and line 330 still use options.signal ?? AbortSignal.timeout(...), so supplying a caller signal disables requestTimeoutMs. getRun and listNotebookRuns should migrate to the shared helper.

  5. [P2] Error-body reads suppress cancellation. http.ts catches every response.text() rejection. If a non-2xx body stalls, a timeout or caller abort is converted into an ApiError, contradicting the documented cancellation contract.

  6. [P2] The README imports a nonexistent getNotebook. packages/cloud/README.md cannot compile against this branch because that function is neither implemented nor exported.

  7. [P2] The README interrupts a submission before establishing its state. It queues a run, immediately interrupts, then claims the next run inherits the first submission’s state. Internal interrupt drains both running and pending runs for that notebook. The example must poll the submitted run to success first.

  8. [P3] Only Bash completion includes --session. The zsh and fish generators still omit it in completions.ts.

The remaining session endpoint paths, schemas, ID domains, statuses, and storageMode values match deepnote-internal.

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