feat(local-runner): @deepnote/local-runner — run a .deepnote with edited inputs, locally or in Deepnote Cloud - #419
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant serveStatic
participant runWithInputs
participant ExecutionEngine
participant Snapshot
Browser->>serveStatic: POST /api/run with inputs
serveStatic->>runWithInputs: Run notebook
runWithInputs->>ExecutionEngine: Start and execute project
ExecutionEngine-->>runWithInputs: Outputs and block results
runWithInputs->>Snapshot: Build snapshot YAML
Snapshot-->>serveStatic: Snapshot and summary
serveStatic-->>Browser: JSON execution result
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 @@
## main #419 +/- ##
==========================================
+ Coverage 86.85% 87.36% +0.50%
==========================================
Files 166 181 +15
Lines 8766 9494 +728
Branches 2478 2624 +146
==========================================
+ Hits 7614 8294 +680
- Misses 1151 1199 +48
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
packages/local-runner/src/serve-static.test.ts (2)
10-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
ts-dedentfor the multiline fixture.This raw multiline template literal violates the repository TypeScript rule and makes indentation changes easier to introduce.
As per coding guidelines, TypeScript files should use
ts-dedentfor clean multiline template strings.🤖 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/serve-static.test.ts` around lines 10 - 31, Replace the raw multiline NOTEBOOK template literal with the repository’s ts-dedent utility, importing it as needed and wrapping the fixture content with dedent so indentation remains clean and consistent.Source: Coding guidelines
71-116: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd regression coverage for rejected-request branches.
Add tests for invalid
inputsshapes, oversized bodies, malformed percent-encoding, and symlink escapes so the server’s validation and traversal guarantees remain enforced.🤖 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/serve-static.test.ts` around lines 71 - 116, Add regression tests to the serveStatic suite covering rejected requests: invalid `inputs` shapes for POST /api/run, request bodies exceeding the configured size limit, malformed percent-encoded paths, and static-file symlinks escaping the serving root. Use the existing rawStatus/handle helpers and temporary fixture setup as appropriate, asserting the expected 400, 413, 400, and 403 responses respectively.
🤖 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/run-with-inputs.ts`:
- Around line 67-73: Validate the input’s persistSnapshot setting immediately
after loadDeepnoteFile and before constructing or starting ExecutionEngine;
reject persistSnapshot: true with the existing unsupported-configuration error.
Update the related test to assert validation occurs before engine startup and
that no execution or side effects occur.
In `@packages/local-runner/src/serve-static.ts`:
- Around line 77-86: Validate the parsed request’s inputs before calling runner
in the POST /api/run handler: accept only non-null, non-array objects, return a
400 response for null, arrays, strings, or other invalid values, and pass the
validated record to runner instead of relying on the erased type assertion.
- Around line 106-108: Handle malformed percent-encoding in serveFile by
catching URIError specifically around decodeURIComponent and responding with
HTTP 400, rather than allowing it to reach the outer handler and become a 500;
preserve existing behavior for other errors.
- Around line 115-120: Update the static-file serving logic to validate and read
the target before calling res.writeHead(200), ensuring directories are rejected
and readFile errors are handled by the existing error path without sending
headers first. Use the handler’s target validation and readFile flow around
sendJson and res.end to preserve the 404 response for invalid entries and only
send 200 after a successful file read.
- Around line 106-118: The path-traversal protection in serveFile is only
lexical and allows symlinks under rootDir to escape the serving directory.
Resolve rootDir and target to real paths, then verify the real target remains
within the real root before reading or serving it; return 403 for escapes and
preserve appropriate not-found handling for missing paths.
- Around line 128-137: Update readBody to track received byte length rather than
string length, and explicitly reject with a payload-too-large error when the
limit is exceeded before destroying the request. Handle req.aborted and
req.close events so the Promise cannot remain pending, while avoiding duplicate
settlement; ensure the /api/run caller can identify this limit error and respond
with HTTP 413.
---
Nitpick comments:
In `@packages/local-runner/src/serve-static.test.ts`:
- Around line 10-31: Replace the raw multiline NOTEBOOK template literal with
the repository’s ts-dedent utility, importing it as needed and wrapping the
fixture content with dedent so indentation remains clean and consistent.
- Around line 71-116: Add regression tests to the serveStatic suite covering
rejected requests: invalid `inputs` shapes for POST /api/run, request bodies
exceeding the configured size limit, malformed percent-encoded paths, and
static-file symlinks escaping the serving root. Use the existing
rawStatus/handle helpers and temporary fixture setup as appropriate, asserting
the expected 400, 413, 400, and 403 responses respectively.
🪄 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: f1cdcf9a-f056-41a6-8578-addfd4a80885
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
packages/local-runner/README.mdpackages/local-runner/package.jsonpackages/local-runner/src/apply-input-overrides.test.tspackages/local-runner/src/apply-input-overrides.tspackages/local-runner/src/execution.integration.test.tspackages/local-runner/src/index.tspackages/local-runner/src/load-file.tspackages/local-runner/src/run-with-inputs.test.tspackages/local-runner/src/run-with-inputs.tspackages/local-runner/src/serve-static.test.tspackages/local-runner/src/serve-static.tspackages/local-runner/tsdown.config.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/local-runner/src/run-with-inputs.test.ts (1)
121-128: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert persisted snapshot contents, not only file existence.
Read
result.snapshotPathand verify it contains the expected serialized output (or matchesresult.snapshotYaml). This catches write-path and serialization regressions that the current existence checks miss.🤖 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/run-with-inputs.test.ts` around lines 121 - 128, Update the test “persists a snapshot next to a path input by default (like deepnote run)” to read the file at result.snapshotPath and assert its contents contain the expected serialized output, preferably matching result.snapshotYaml. Keep the existing path and existence assertions while adding validation of the persisted snapshot data.
🤖 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.
Nitpick comments:
In `@packages/local-runner/src/run-with-inputs.test.ts`:
- Around line 121-128: Update the test “persists a snapshot next to a path input
by default (like deepnote run)” to read the file at result.snapshotPath and
assert its contents contain the expected serialized output, preferably matching
result.snapshotYaml. Keep the existing path and existence assertions while
adding validation of the persisted snapshot data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3907b81a-4ec2-4f6b-8586-a20139bffac5
📒 Files selected for processing (4)
packages/local-runner/README.mdpackages/local-runner/src/run-with-inputs.test.tspackages/local-runner/src/run-with-inputs.tspackages/local-runner/src/serve-static.ts
✅ Files skipped from review due to trivial changes (1)
- packages/local-runner/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/local-runner/src/serve-static.ts
feb76b0 to
ee6e6a7
Compare
85f975b to
7965b78
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/local-runner/src/run-in-cloud.ts (1)
105-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared input-coercion helper.
coerceInputsrepeats the same input-block lookup andcoerceInputVariableValuecall used byapplyInputOverrides; extracting that shared logic would keep the cloud and local-run paths from drifting.🤖 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/run-in-cloud.ts` around lines 105 - 121, Extract the input-block lookup and coerceInputVariableValue logic from coerceInputs and applyInputOverrides into a shared helper, then have both callers reuse it while preserving each function’s existing input/output behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/run-cloud.ts`:
- Around line 306-309: Update the defensive refetch in the post-poll handling
around getRun so failures are caught and represented as missing snapshot content
rather than escaping. Preserve the terminal runId and route the failed refetch
through the existing snapshot-error handling path, allowing the command to
render its structured result.
- Around line 214-218: Sanitize the remote runId before interpolating it into
snapshotPath in the fallback write flow. Ensure the resulting filename cannot
contain path separators or traversal segments and remains within the snapshots
directory, using an existing filename-safe identifier or generated name while
preserving the current snapshot extension and write behavior.
In `@packages/local-runner/src/run-in-cloud.ts`:
- Around line 79-91: Update runInCloud to defensively re-fetch the terminal run
with getRun when the successful polled result lacks an inline snapshot, then use
the refreshed run for fetchSnapshotContent and output extraction while
preserving existing failure handling. Add the corresponding getRun mock in
run-in-cloud.test.ts so this path is covered.
---
Nitpick comments:
In `@packages/local-runner/src/run-in-cloud.ts`:
- Around line 105-121: Extract the input-block lookup and
coerceInputVariableValue logic from coerceInputs and applyInputOverrides into a
shared helper, then have both callers reuse it while preserving each function’s
existing input/output 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: bdd426d1-356f-4d72-988c-4d2620dde6f7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
packages/cli/package.jsonpackages/cli/src/commands/run-cloud.tspackages/cloud/README.mdpackages/cloud/package.jsonpackages/cloud/src/cloud-runs.test.tspackages/cloud/src/cloud-runs.tspackages/cloud/src/index.tspackages/cloud/tsdown.config.tspackages/local-runner/README.mdpackages/local-runner/package.jsonpackages/local-runner/src/apply-input-overrides.test.tspackages/local-runner/src/apply-input-overrides.tspackages/local-runner/src/execution.integration.test.tspackages/local-runner/src/index.tspackages/local-runner/src/load-file.tspackages/local-runner/src/run-in-cloud.test.tspackages/local-runner/src/run-in-cloud.tspackages/local-runner/src/run-with-inputs.test.tspackages/local-runner/src/run-with-inputs.tspackages/local-runner/src/serve-static.test.tspackages/local-runner/src/serve-static.tspackages/local-runner/tsdown.config.ts
✅ Files skipped from review due to trivial changes (2)
- packages/cloud/README.md
- packages/cloud/tsdown.config.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/local-runner/src/execution.integration.test.ts
- packages/local-runner/src/apply-input-overrides.test.ts
- packages/local-runner/tsdown.config.ts
- packages/local-runner/src/serve-static.test.ts
- packages/local-runner/src/index.ts
- packages/local-runner/src/apply-input-overrides.ts
- packages/local-runner/src/run-with-inputs.ts
- packages/local-runner/README.md
- packages/local-runner/src/run-with-inputs.test.ts
- packages/local-runner/src/load-file.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/local-runner/src/run-in-cloud.ts (1)
105-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared input-coercion helper.
coerceInputsrepeats the same input-block lookup andcoerceInputVariableValuecall used byapplyInputOverrides; extracting that shared logic would keep the cloud and local-run paths from drifting.🤖 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/run-in-cloud.ts` around lines 105 - 121, Extract the input-block lookup and coerceInputVariableValue logic from coerceInputs and applyInputOverrides into a shared helper, then have both callers reuse it while preserving each function’s existing input/output behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/run-cloud.ts`:
- Around line 306-309: Update the defensive refetch in the post-poll handling
around getRun so failures are caught and represented as missing snapshot content
rather than escaping. Preserve the terminal runId and route the failed refetch
through the existing snapshot-error handling path, allowing the command to
render its structured result.
- Around line 214-218: Sanitize the remote runId before interpolating it into
snapshotPath in the fallback write flow. Ensure the resulting filename cannot
contain path separators or traversal segments and remains within the snapshots
directory, using an existing filename-safe identifier or generated name while
preserving the current snapshot extension and write behavior.
In `@packages/local-runner/src/run-in-cloud.ts`:
- Around line 79-91: Update runInCloud to defensively re-fetch the terminal run
with getRun when the successful polled result lacks an inline snapshot, then use
the refreshed run for fetchSnapshotContent and output extraction while
preserving existing failure handling. Add the corresponding getRun mock in
run-in-cloud.test.ts so this path is covered.
---
Nitpick comments:
In `@packages/local-runner/src/run-in-cloud.ts`:
- Around line 105-121: Extract the input-block lookup and
coerceInputVariableValue logic from coerceInputs and applyInputOverrides into a
shared helper, then have both callers reuse it while preserving each function’s
existing input/output 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: bdd426d1-356f-4d72-988c-4d2620dde6f7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
packages/cli/package.jsonpackages/cli/src/commands/run-cloud.tspackages/cloud/README.mdpackages/cloud/package.jsonpackages/cloud/src/cloud-runs.test.tspackages/cloud/src/cloud-runs.tspackages/cloud/src/index.tspackages/cloud/tsdown.config.tspackages/local-runner/README.mdpackages/local-runner/package.jsonpackages/local-runner/src/apply-input-overrides.test.tspackages/local-runner/src/apply-input-overrides.tspackages/local-runner/src/execution.integration.test.tspackages/local-runner/src/index.tspackages/local-runner/src/load-file.tspackages/local-runner/src/run-in-cloud.test.tspackages/local-runner/src/run-in-cloud.tspackages/local-runner/src/run-with-inputs.test.tspackages/local-runner/src/run-with-inputs.tspackages/local-runner/src/serve-static.test.tspackages/local-runner/src/serve-static.tspackages/local-runner/tsdown.config.ts
✅ Files skipped from review due to trivial changes (2)
- packages/cloud/README.md
- packages/cloud/tsdown.config.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/local-runner/src/execution.integration.test.ts
- packages/local-runner/src/apply-input-overrides.test.ts
- packages/local-runner/tsdown.config.ts
- packages/local-runner/src/serve-static.test.ts
- packages/local-runner/src/index.ts
- packages/local-runner/src/apply-input-overrides.ts
- packages/local-runner/src/run-with-inputs.ts
- packages/local-runner/README.md
- packages/local-runner/src/run-with-inputs.test.ts
- packages/local-runner/src/load-file.ts
🛑 Comments failed to post (3)
packages/cli/src/commands/run-cloud.ts (2)
214-218: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n 'runId|NormalizedRun|triggerNotebookRun|getRun' packages/cloud packages/cliRepository: deepnote/deepnote
Length of output: 7747
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- outline: packages/cli/src/commands/run-cloud.ts ---' ast-grep outline packages/cli/src/commands/run-cloud.ts --view expanded || true echo echo '--- lines 200-230 ---' sed -n '200,230p' packages/cli/src/commands/run-cloud.ts echo echo '--- lines 288-320 ---' sed -n '288,320p' packages/cli/src/commands/run-cloud.ts echo echo '--- relevant tests around snapshot path ---' sed -n '250,340p' packages/cli/src/commands/run-cloud.test.tsRepository: deepnote/deepnote
Length of output: 7873
Sanitize
runIdbefore using it in the fallback filename.runIdcomes from the remote response, so a path separator or..segment can escape./snapshots; use a filename-safe ID or a generated name here.🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 217-217: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(snapshotPath, bytes, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').(detect-non-literal-fs-filename-typescript)
🤖 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/run-cloud.ts` around lines 214 - 218, Sanitize the remote runId before interpolating it into snapshotPath in the fallback write flow. Ensure the resulting filename cannot contain path separators or traversal segments and remains within the snapshots directory, using an existing filename-safe identifier or generated name while preserving the current snapshot extension and write behavior.Source: Linters/SAST tools
306-309: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not let the defensive refetch bypass post-run handling.
If
getRunfails after polling has already returned a terminalrunId, the exception escapes before the command renders its structured result. Treat the refetch failure as missing snapshot content and let the existing snapshot-error path handle it.🤖 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/run-cloud.ts` around lines 306 - 309, Update the defensive refetch in the post-poll handling around getRun so failures are caught and represented as missing snapshot content rather than escaping. Preserve the terminal runId and route the failed refetch through the existing snapshot-error handling path, allowing the command to render its structured result.packages/local-runner/src/run-in-cloud.ts (1)
79-91: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing defensive re-fetch when the terminal run lacks an inline snapshot.
packages/cli/src/commands/run-cloud.tshits this exact scenario and explicitly re-fetches viagetRunwhen!finalRun.snapshotafter polling, with the comment noting some deployments only attach the snapshot once terminal.runInCloudskips that guard —fetchSnapshotContentreturnsnullimmediately whenrun.snapshotis absent, so a genuinely successful run can silently come back withsuccess: true,outputs: [],snapshotYaml: null.🔧 Proposed fix: mirror the CLI's defensive re-fetch
import { describeRunError, fetchSnapshotContent, + getRun, isSuccessStatus, type PollOptions, pollRunUntilComplete, triggerNotebookRun, } from '`@deepnote/cloud`' @@ - const run = await pollRunUntilComplete(baseUrl, token, started.runId, { snapshotDelivery: 'inline', ...options.poll }) + let run = await pollRunUntilComplete(baseUrl, token, started.runId, { snapshotDelivery: 'inline', ...options.poll }) + // Some deployments only attach the snapshot once the run is confirmed terminal. + if (!run.snapshot) { + run = await getRun(baseUrl, token, run.runId, { snapshotDelivery: 'inline' }) + } const success = isSuccessStatus(run.status)Note:
run-in-cloud.test.ts'svi.mock('@deepnote/cloud', ...)will need to add agetRunmock for this path to be testable.📝 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.let run = await pollRunUntilComplete(baseUrl, token, started.runId, { snapshotDelivery: 'inline', ...options.poll }) if (!run.snapshot) { run = await getRun(baseUrl, token, run.runId, { snapshotDelivery: 'inline' }) } const success = isSuccessStatus(run.status) const snapshotYaml = success ? await fetchSnapshotContent(run, { baseUrl, token }) : null return { runId: run.runId, status: run.status, success, outputs: snapshotYaml ? extractOutputs(snapshotYaml) : [], snapshotYaml, error: success ? undefined : describeRunError(run), }🤖 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/run-in-cloud.ts` around lines 79 - 91, Update runInCloud to defensively re-fetch the terminal run with getRun when the successful polled result lacks an inline snapshot, then use the refreshed run for fetchSnapshotContent and output extraction while preserving existing failure handling. Add the corresponding getRun mock in run-in-cloud.test.ts so this path is covered.Source: Linked repositories
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/local-runner/src/run-in-cloud.test.ts (1)
127-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test covers domain/baseUrl propagation into the upload fallback.
Given the upload-if-missing path silently drops any custom
baseUrl/domain (seeopen-in-cloud.ts), a test assertinguploadNotebookreceives the expected domain would catch that regression.🤖 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/run-in-cloud.test.ts` around lines 127 - 140, Extend the upload-if-missing test around runInCloud to pass a custom baseUrl/domain and assert cloudMock.uploadNotebook receives that value. Preserve the existing missing-notebook result assertions while verifying domain propagation through the fallback path.packages/cloud/src/import.ts (1)
49-49: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNo runtime validation of the init response shape.
initis blindly cast (as InitImportResponse) from JSON; a malformed/renamed field on the server side would surface as an opaque failure later (e.g.fetch(init.uploadUrl, ...)withundefined). Consider a minimal shape check before use.🤖 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/import.ts` at line 49, Validate the parsed response in the import initialization flow before using it as InitImportResponse. Add a minimal runtime check that confirms the required initialization fields, especially uploadUrl, exist and have the expected shape, and fail immediately with a clear error when validation fails; keep subsequent fetch usage unchanged for valid responses.
🤖 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/import.ts`:
- Around line 51-55: Update the presigned upload request in the import flow to
pass an AbortSignal timeout using the existing timeout configuration, matching
the init POST behavior. Ensure the PUT fetch for fileBytes cannot block
indefinitely while preserving its current method, headers, and body.
In `@packages/local-runner/src/open-in-cloud.ts`:
- Around line 21-30: The runInCloud not-found fallback drops the custom domain
when delegating to openInCloud, causing uploads to use the default endpoint.
Update the fallback call in run-in-cloud.ts to pass the domain derived from
baseUrl alongside inputs, and add coverage in
packages/local-runner/src/run-in-cloud.test.ts for a custom baseUrl/domain
asserting uploadNotebook receives that domain; the openInCloud function already
forwards options.domain and requires no direct change.
---
Nitpick comments:
In `@packages/cloud/src/import.ts`:
- Line 49: Validate the parsed response in the import initialization flow before
using it as InitImportResponse. Add a minimal runtime check that confirms the
required initialization fields, especially uploadUrl, exist and have the
expected shape, and fail immediately with a clear error when validation fails;
keep subsequent fetch usage unchanged for valid responses.
In `@packages/local-runner/src/run-in-cloud.test.ts`:
- Around line 127-140: Extend the upload-if-missing test around runInCloud to
pass a custom baseUrl/domain and assert cloudMock.uploadNotebook receives that
value. Preserve the existing missing-notebook result assertions while verifying
domain propagation through the fallback 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: 19b9d0df-5044-49f6-a7ce-a8a099ebc629
📒 Files selected for processing (7)
packages/cloud/src/import.tspackages/cloud/src/index.tspackages/local-runner/src/index.tspackages/local-runner/src/open-in-cloud.tspackages/local-runner/src/run-in-cloud.test.tspackages/local-runner/src/run-in-cloud.tspackages/local-runner/src/serve-static.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/local-runner/src/index.ts
- packages/local-runner/src/run-in-cloud.ts
- packages/local-runner/src/serve-static.ts
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/projects.ts`:
- Around line 58-66: Update findNotebookId so that when query.notebookName is
provided, it only returns a notebook with that exact name while iterating
through projects; continue searching older projects if the current project has
no match. Retain the fallback to the first notebook only when no notebookName
was requested.
🪄 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: 4d2cc5dc-e023-4402-b3fe-2fd19a2d504c
📒 Files selected for processing (5)
packages/cloud/src/cloud-runs.tspackages/cloud/src/index.tspackages/cloud/src/projects.tspackages/local-runner/src/run-in-cloud.test.tspackages/local-runner/src/run-in-cloud.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cloud/src/index.ts
- packages/local-runner/src/run-in-cloud.test.ts
- packages/local-runner/src/run-in-cloud.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/cloud/src/projects.ts (1)
63-76: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFallback to
notebooks[0]still short-circuits the search for an exactnotebookNamematch.When
query.notebookNameis provided but absent from the current project,match ?? notebooks[0]falls back to that project's first notebook and returns immediately — it never checks older matching projects for the exact name. This is the same behavior flagged in a prior review (marked "Addressed"), but the current code still exhibits it.🐛 Proposed fix
- for (const project of projects) { - const notebooks = project.notebooks ?? [] - const match = query.notebookName ? notebooks.find(notebook => notebook.name === query.notebookName) : undefined - const notebook = match ?? notebooks[0] - if (notebook) { - return { notebookId: notebook.id, projectId: project.id } - } - } - return undefined + if (query.notebookName) { + for (const project of projects) { + const match = (project.notebooks ?? []).find(notebook => notebook.name === query.notebookName) + if (match) { + return { notebookId: match.id, projectId: project.id } + } + } + return undefined + } + const first = projects[0]?.notebooks?.[0] + return first ? { notebookId: first.id, projectId: projects[0].id } : 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/projects.ts` around lines 63 - 76, Update the project/notebook search loop so that when query.notebookName is provided, projects without an exact notebook match are skipped rather than falling back to notebooks[0]; return the matching notebook from any matching project, while retaining the first-notebook fallback only when no notebook name is requested.
🧹 Nitpick comments (1)
examples/local-runner-demo/index.html (1)
163-168: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueAvoid
innerHTMLfor the view-URL link.
viewUrlis server-controlled today, but building viainnerHTMLtemplate literal is an easy-to-copy anti-pattern. Building the anchor via DOM APIs sidesteps it entirely.🔧 Proposed fix
- if (data.viewUrl) { - const s = $('`#status`'); s.className = 'status ok' - s.innerHTML = `☁ cloud run ${data.status} — <a href="${data.viewUrl}" target="_blank" rel="noopener">view runs in Deepnote →</a>` - } else { + if (data.viewUrl) { + const s = $('`#status`'); s.className = 'status ok' + s.textContent = `☁ cloud run ${data.status} — ` + const a = el('a'); a.href = data.viewUrl; a.target = '_blank'; a.rel = 'noopener'; a.textContent = 'view runs in Deepnote →' + s.appendChild(a) + } else {🤖 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 `@examples/local-runner-demo/index.html` around lines 163 - 168, Replace the innerHTML assignment in the data.viewUrl branch with DOM API construction: clear the status element, create an anchor element, set its text, href, target, and rel properties, then append the surrounding status text and anchor while preserving the existing status class and displayed message.Source: Linters/SAST tools
🤖 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 `@examples/local-runner-demo/index.html`:
- Around line 190-201: Update the text/html branch in renderOutput to sanitize
notebook HTML before assigning it to innerHTML, using an established sanitizer
if available and preserving rich HTML rendering. Ensure untrusted or shared
notebook output cannot execute scripts or otherwise inject unsafe content.
---
Duplicate comments:
In `@packages/cloud/src/projects.ts`:
- Around line 63-76: Update the project/notebook search loop so that when
query.notebookName is provided, projects without an exact notebook match are
skipped rather than falling back to notebooks[0]; return the matching notebook
from any matching project, while retaining the first-notebook fallback only when
no notebook name is requested.
---
Nitpick comments:
In `@examples/local-runner-demo/index.html`:
- Around line 163-168: Replace the innerHTML assignment in the data.viewUrl
branch with DOM API construction: clear the status element, create an anchor
element, set its text, href, target, and rel properties, then append the
surrounding status text and anchor while preserving the existing status class
and displayed message.
🪄 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: d12ba3d8-86cc-491a-ac0a-04b7cc28e57c
📒 Files selected for processing (7)
examples/local-runner-demo/README.mdexamples/local-runner-demo/index.htmlexamples/local-runner-demo/serve.mjspackages/cloud/src/projects.tspackages/local-runner/src/run-in-cloud.test.tspackages/local-runner/src/run-in-cloud.tspackages/local-runner/src/serve-static.ts
✅ Files skipped from review due to trivial changes (1)
- examples/local-runner-demo/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/local-runner/src/run-in-cloud.test.ts
- packages/local-runner/src/run-in-cloud.ts
- packages/local-runner/src/serve-static.ts
0ab08e1 to
a86964c
Compare
`deepnote run -i <slider>=N` silently dropped the execution snapshot: the
override value was written to `deepnote_variable_value` verbatim (a number),
but the block schema requires a string, so `serializeDeepnoteSnapshot` threw
`Expected string, received number` and the best-effort save swallowed it.
Add a type-aware `coerceInputVariableValue(block, value)` schema-normalization
helper to @deepnote/blocks (slider/text/textarea/date/file → string; checkbox
strict boolean; select shape-only respecting multi-value; date-range arity),
and apply it in the CLI's `applyInputOverrides`. The kernel-injection payload
passed to `runProject({ inputs })` intentionally keeps native user values.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ee6e6a7 to
ae895f2
Compare
Adds a `--cloud` flag to the `run` command that triggers a cloud run of an
existing Deepnote notebook via the public API (`POST /v2/runs`), polls it to
completion (`GET /v2/runs/{runId}`), and downloads the resulting snapshot into
the local `snapshots/` convention — so `deepnote diff` and the MCP snapshot
tools work on it immediately.
The notebook to run is resolved from `--notebook-id`, else the local
`.deepnote` file's notebook (`--notebook <name>` or the single/main notebook).
The notebook must already exist in Deepnote; uploading local content
(`--push`) is a hidden, not-yet-implemented follow-up.
- New `utils/cloud-runs.ts`: Bearer-auth client mirroring `fetchIntegrations`
with drift-tolerant polling (429/5xx backoff, per-request + total timeout,
`RunTimeoutError` carrying the runId) and cross-origin-safe snapshot download
(no bearer on presigned S3 URLs).
- New `commands/run-cloud.ts`: orchestration — notebook-id resolution, `.env`
load, blank-token-as-missing, snapshot parse (snapshot-doc → full-file split
→ raw fallback), timestamped + latest writes, terminal-failure → exit 1 while
preserving runId/status/snapshotPath, `-o json`/`-o toon` output.
- `utils/parse-inputs.ts`: extracted shared helper (avoids a run↔run-cloud cycle).
- `run.ts` / `cli.ts`: options, early dispatch, incompatible-flag guard, help.
- Docs: `skills/deepnote/references/cli-run.md` + `packages/cli/README.md`.
The runs API is in preview, so response schemas are intentionally permissive
and the exact snapshot field names / `detached` requirement should be confirmed
against a live token.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… origins Cap per-request timeout, backoff, and interval sleeps to the remaining deadline so `timeoutMs` is a true total wait, and re-check the deadline before each request. Compare URL origins (not hosts) before attaching the bearer token to a snapshot download, so a same-host http:// URL is treated as cross-origin. (CodeRabbit) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e saved Downloading the snapshot is the command's contract, so a successful run whose snapshot is missing, unretrievable, or unwritable now exits 1 with success:false and an error (rather than best-effort success). A run that already failed keeps a missing snapshot non-fatal. (CodeRabbit) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
These can hold secrets or PII and were emitted to stderr in debug mode; log only presence/count now, keeping token redaction. (CodeRabbit) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Validate the whole string and require a safe positive integer so `1.5` and `10s` are rejected instead of silently truncated. (CodeRabbit) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…re for cloud runs Use the full `https://api.deepnote.com` default in the README, note that `-o llm` resolves to `toon`, and document that a successful run whose snapshot can't be saved exits 1. (CodeRabbit) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…/cloud package
Move the Deepnote Cloud runs client (trigger/poll/get/fetch-snapshot) out of
cli/utils/cloud-runs.ts into a new @deepnote/cloud package so it can be shared,
and fix snapshot fetching along the way: GET /v2/runs/{runId} returns the
snapshot on flat run.snapshotContent / run.snapshotDownloadUrl fields, not
nested under `snapshot`, so normalizeRun now reads them and `deepnote run
--cloud` actually retrieves outputs. run-cloud imports from @deepnote/cloud;
parseApiErrorMessage is inlined so the package has no CLI dependency.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/run.ts (1)
583-599: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicate
applyInputOverridesimplementation.This function is functionally identical to
applyInputOverridesinpackages/local-runner/src/apply-input-overrides.ts(same loop, same guard, samecoerceInputVariableValuecall, near-identical comments). Two independent copies of the same coercion loop will silently drift if either package extends this logic later (e.g. new guard conditions, error handling).Consider exporting a single
applyInputOverridesfrom@deepnote/blocks(which already hostscoerceInputVariableValue) and having bothpackages/cli/src/commands/run.tsandpackages/local-runner/src/apply-input-overrides.tsimport it, rather than maintaining two copies.🤖 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/run.ts` around lines 583 - 599, Consolidate the duplicate applyInputOverrides implementations by exporting a shared applyInputOverrides from `@deepnote/blocks` alongside coerceInputVariableValue. Replace the local loop in both run.ts and local-runner’s apply-input-overrides.ts with imports of that shared function, preserving the existing metadata override and coercion 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 `@examples/local-runner-demo/index.html`:
- Around line 163-168: Replace the template-literal assignment to s.innerHTML in
the data.viewUrl branch with DOM API construction: create the anchor element,
set its href and text content, apply target and rel attributes, and append it
alongside the status text. Preserve the existing status styling and displayed
cloud-run status without interpolating data.viewUrl or other dynamic values into
HTML.
---
Outside diff comments:
In `@packages/cli/src/commands/run.ts`:
- Around line 583-599: Consolidate the duplicate applyInputOverrides
implementations by exporting a shared applyInputOverrides from `@deepnote/blocks`
alongside coerceInputVariableValue. Replace the local loop in both run.ts and
local-runner’s apply-input-overrides.ts with imports of that shared function,
preserving the existing metadata override and coercion 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: eadfe3fe-2486-4824-9300-be75e4e5905b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
examples/local-runner-demo/README.mdexamples/local-runner-demo/index.htmlexamples/local-runner-demo/serve.mjspackages/blocks/src/blocks/input-blocks.test.tspackages/blocks/src/blocks/input-blocks.tspackages/blocks/src/index.tspackages/cli/package.jsonpackages/cli/src/commands/run-cloud.tspackages/cli/src/commands/run.test.tspackages/cli/src/commands/run.tspackages/cloud/README.mdpackages/cloud/package.jsonpackages/cloud/src/cloud-runs.test.tspackages/cloud/src/cloud-runs.tspackages/cloud/src/import.tspackages/cloud/src/index.tspackages/cloud/src/projects.tspackages/cloud/tsdown.config.tspackages/local-runner/README.mdpackages/local-runner/package.jsonpackages/local-runner/src/apply-input-overrides.test.tspackages/local-runner/src/apply-input-overrides.tspackages/local-runner/src/execution.integration.test.tspackages/local-runner/src/index.tspackages/local-runner/src/load-file.tspackages/local-runner/src/open-in-cloud.tspackages/local-runner/src/run-in-cloud.test.tspackages/local-runner/src/run-in-cloud.tspackages/local-runner/src/run-with-inputs.test.tspackages/local-runner/src/run-with-inputs.tspackages/local-runner/src/serve-static.test.tspackages/local-runner/src/serve-static.tspackages/local-runner/tsdown.config.ts
✅ Files skipped from review due to trivial changes (7)
- packages/blocks/src/index.ts
- packages/cloud/package.json
- packages/cloud/src/index.ts
- packages/cloud/README.md
- packages/cli/src/commands/run-cloud.ts
- examples/local-runner-demo/README.md
- packages/local-runner/README.md
🚧 Files skipped from review as they are similar to previous changes (21)
- examples/local-runner-demo/serve.mjs
- packages/local-runner/src/execution.integration.test.ts
- packages/cloud/tsdown.config.ts
- packages/cli/package.json
- packages/local-runner/src/load-file.ts
- packages/local-runner/package.json
- packages/local-runner/src/index.ts
- packages/local-runner/src/apply-input-overrides.test.ts
- packages/local-runner/src/open-in-cloud.ts
- packages/cloud/src/import.ts
- packages/local-runner/tsdown.config.ts
- packages/local-runner/src/run-with-inputs.test.ts
- packages/local-runner/src/apply-input-overrides.ts
- packages/cloud/src/cloud-runs.ts
- packages/local-runner/src/serve-static.test.ts
- packages/cloud/src/projects.ts
- packages/local-runner/src/run-in-cloud.ts
- packages/local-runner/src/run-with-inputs.ts
- packages/local-runner/src/serve-static.ts
- packages/local-runner/src/run-in-cloud.test.ts
- packages/cloud/src/cloud-runs.test.ts
a86964c to
f8e6c67
Compare
jamesbhobbs
changed the title
Addresses CodeRabbit review on #417: - A path-like `runId` from the runs API could escape ./snapshots when used in the fallback filename via resolve(); sanitize it (allow [A-Za-z0-9_-], cap length) and build the path with join(). Adds a regression test with a malicious `../` runId. - Assert the cross-origin snapshot download omits the Authorization header (the presigned S3 URL is a different origin; the bearer must not leak). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The /v1/import flow answers with `error`; the /v2 API answers with `message`.
Reading only the first meant every v2 failure reached the caller as the raw
JSON body — `{"message":"Notebook not found"}` where "Notebook not found" was
the whole point. Both keys are now read, `error` first so v1 is untouched.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… creating A .deepnote SQL block keeps its connection in metadata.sql_integration_id. Deepnote rejects that key outright — a 400, not a silent strip — and takes the connection as a top-level integrationId, which it then writes into that very key itself. We forwarded the metadata untouched and looked for a top-level integrationId that the block schema has no field for, so the check was dead code and every SQL block would have failed. Two of the shipped examples have one. The value is now lifted out of the metadata; an id Deepnote cannot accept (it must be a UUID, which the built-in dataframe connection is not) is dropped with a warning, since an unbound block is the only shape the API will take. The "not found" recovery matched /not found/i, which is three different failures wearing one phrase. `Notebook not found` is the only one worth recovering from; `Block not found in notebook` is a bad block id, and the endpoint's only 404 is a bare `Not found` meaning the token's owner has left the workspace. Both of those used to be answered by looking the notebook up and creating a duplicate project — a typo, or an expired membership, and you get a new project. The rest is moving decisions ahead of the first request, since all of them are facts about the local file: which notebook is the target, whether the requested blocks are in it, and whether an input name is one the notebook defines. Deepnote has no kernel injection, so an unmatched name is a 400 there — unlike runWithInputs, where it is a variable and stays one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Run history belongs to one notebook, but listCloudRuns quietly took the first notebook of the file whenever there were several — answering with a real, plausible history that simply belongs to something else. It now resolves the notebook the same way runInCloud does, and refuses the ambiguity rather than picking, so the two agree about which notebook a file means. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lint-staged ran `sort -o .gitignore .gitignore` on every commit that touched the file. Order is significant in a .gitignore — the last matching pattern wins — so sorting hoisted `!packages/**/*.png` above `*.png` and quietly un-did the exception. It also left every line duplicated. Restoring the file alone would have lasted until the next commit that touched it, so the rule goes too. There is no safe way to sort a file whose semantics depend on order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@dinohamzic thanks — this was a genuinely useful review. I verified each finding against Where the verified behaviour changed the fixPage through project lookup — Reuse the seeded Notebook 1 — confirmed the seed is the literal string Translate SQL integration metadata — worse than described. It is a hard 400 from a Match only notebook-not-found — worse than described, in a way I'd flag. Preserve the completion timestamp — Two I implemented differently — flagging for your callReject undefined Cloud inputs — confirmed exactly ( Remap notebook-function targets — real, but I did not build the remap, and I'd like your view. Deepnote's own project duplication doesn't remap Duplicate names — the local half of that findingCloud enforces per-project name uniqueness; a Two root causes the review reached the symptom of
Error messages — Full suite green (5127 passing), typecheck and spell-check clean. Ready for another look. |
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/projects.ts`:
- Around line 74-88: Update the response parsing in the project-listing flow
around projectsSchema.safeParse so await response.json() is wrapped in error
handling that converts malformed or non-JSON bodies into ApiError with status
502. Preserve the existing schema-validation ApiError behavior for valid JSON
that does not match projectsSchema, and keep successful parsing unchanged.
🪄 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: 79696c28-cc87-4a19-9d5a-057e8a69a15f
📒 Files selected for processing (16)
.gitignorepackage.jsonpackages/cli/src/utils/deepnote-api.test.tspackages/cli/src/utils/run-in-cloud.tspackages/cloud/src/cloud-runs.test.tspackages/cloud/src/cloud-runs.tspackages/cloud/src/create-project.test.tspackages/cloud/src/create-project.tspackages/cloud/src/index.tspackages/cloud/src/parse-api-error.tspackages/cloud/src/projects.test.tspackages/cloud/src/projects.tspackages/local-runner/src/cloud-runs.test.tspackages/local-runner/src/cloud-runs.tspackages/local-runner/src/run-in-cloud.test.tspackages/local-runner/src/run-in-cloud.ts
💤 Files with no reviewable changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/cloud/src/create-project.ts
- packages/cloud/src/projects.test.ts
- packages/local-runner/src/cloud-runs.ts
- packages/cloud/src/index.ts
- packages/local-runner/src/cloud-runs.test.ts
…Error findNotebook now promises to throw rather than report absence when it cannot read a response, but `await response.json()` was unguarded — so a non-JSON body (an HTML error page from a proxy, say) escaped as a raw SyntaxError, which is neither that contract nor the ApiError callers of this package catch. `create-project.ts` already guards exactly this, so the guard is shared here and applied to getWorkspace too rather than leaving one of the two unguarded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dinohamzic
left a comment
dinohamzic
left a comment
There was a problem hiding this comment.
@jamesbhobbs thanks, second round, not much left:
-
[P1] Block-targeted cloud runs always fail. Deepnote defaults runs to
detached: true, but rejectsblockIdsin detached mode. Both direct and newly-created targeted runs sendblockIdswithoutdetached: false, producing HTTP 400. EmptyblockIdsarrays should also be omitted because the API requires a non-empty array. -
[P2] Valid notebook-function projects cannot use create-if-missing. Rejecting internal
function_notebook_idreferences prevents stale IDs, but it also rejects valid multi-notebook files despite the API promising to create missing projects. Create all notebooks first, build an old-to-new notebook ID map, then rewrite these references while creating blocks. -
[P2] Project lookup can still falsely report absence. The cloud contract always returns pagination, but the client makes it optional. It also silently stops after 20 pages even when another page token exists. Either case can return
undefinedbefore completing the lookup, causingcreateIfMissingto create a duplicate project. Require pagination and throw if the safety limit is reached with a remaining token.
…s them `POST /v2/runs` defaults every run to `detached: true` and rejects `blockIds` on one — `blockIds is not supported for detached runs`, a 400. So every targeted run failed: the direct one, and the one started right after creating the notebook. Deepnote only runs selected blocks in live mode, so a body carrying blockIds now says `detached: false` for itself. Done in `triggerNotebookRun` rather than at each call site, so the rule lives once next to the endpoint that has it. An empty `blockIds` is dropped there too — the API takes a non-empty list, and "no blocks in particular" is what omitting it means.
…her than refuse it A notebook-function block names the notebook it invokes in `function_notebook_id`, and creating the file gives that notebook a new id the reference would not follow — so `createFromFile` refused any file holding one. That kept stale ids out, but it also turned down valid multi-notebook files that create-if-missing promises to handle, and told the user to go and create the project by hand. The id is knowable, just not while notebooks and blocks are created in lockstep: `createProject` now creates every notebook first, builds `sourceId` → created id, and offers it to a `rewriteBlock` hook as each block goes out. The client stays domain-agnostic — it echoes ids it never reads — and `local-runner` uses that to re-point references into the file at the notebooks Deepnote just made. A reference out of the file already names a real Deepnote notebook, so it is left alone.
…e early `findNotebook` returning `undefined` is what sends `createIfMissing` off to create a project, so it has to mean "looked everywhere, not there". Two things could make it mean less than that: `pagination` was optional in the client though the endpoint always sends it, so a body without one read exactly like a last page; and the page walk stopped at `MAX_PROJECT_PAGES` even with a token still in hand, quietly answering from a lookup that had not finished. Pagination is now required, and a walk that runs out of pages with more to read throws. Both cases were a duplicate project waiting to happen.
…keys A deck that only moves on arrow keys is a deck nobody can present from a trackpad, and clicking the slide is what every projector deck does. The listener sits on `.stage` rather than the document, so the chrome needs no exceptions: the gallery link and the dots are fixed siblings, not descendants, and their clicks never reach it. Forward only. Back stays on the arrow keys and the dots, which already reach any slide — a left-half-goes-back zone is invention, not a fix. A click that ends a text selection is skipped: the last slide is the agent readout as selectable prose, and finishing a copy-drag should not jump off what you just selected.
2 participants
What
A new workspace package,
@deepnote/local-runner, that extracts a reusable mechanism: run a.deepnotenotebook with edited inputs and get outputs + a snapshot back — the building block a static page needs to drive execution. It runs a notebook two ways behind one small API: a local Python kernel, or Deepnote Cloud. Two example apps live underexamples/local-runner/.API
runWithInputs(input, inputs, options?)— run locally.inputis a path, raw.deepnoteYAML, or aDeepnoteFile. Reuses@deepnote/blocks(parse +coerceInputVariableValue),@deepnote/runtime-core(ExecutionEngine), and@deepnote/convert(snapshots) in-process — no shelling out. Returns per-block outputs in execution order, anExecutionSummary, and a snapshot that is persisted next to the file by default (persistSnapshot: falseto skip), likedeepnote run.runInCloud(input, inputs, options?)— run the same notebook in Deepnote Cloud via@deepnote/cloud. Resolves the notebook id from the file, falls back to finding it by project/notebook name, and if it isn't in the cloud yet creates it there — project, notebook, blocks — and runs it in the same call, reportingcreated: true. One click, no browser step: a token is required either way, so there is nothing a logged-in session could add. PasscreateIfMissing: falseto fail instead. On success it returns the parsed outputs plus aviewUrldeep-link to the notebook's runs sidebar. Inputs are coerced to the shape the API expects before sending.serveStatic({ dir, notebookPath, cloudToken? })— a deliberately smallnode:httphelper exposingGET /api/info,POST /api/run(local),POST /api/run-cloud(cloud), and static files with a path-traversal guard. Binds to127.0.0.1. No WebSocket/watch/theme/rendering.listCloudRuns(input, options?)/getCloudRun(runId, options?)— a notebook's run history in Deepnote, and any past run's outputs read back from its snapshot without re-running it. They live incloud-runs.ts; nothing there executes anything.openInCloud,applyInputOverrides,listInputBlocks,loadDeepnoteFile.New
@deepnote/cloudhelpersThis PR adds to the package #417 introduced (it doesn't duplicate the runs client):
create-project.ts—createProject(baseUrl, token, spec)builds a project, its notebooks and their blocks over the authenticated API (POST /v2/projects,/v2/notebooks,/v2/blocks) and returns the ids Deepnote assigned. It takes a plainProjectSpecrather than aDeepnoteFile, so the client stays thin anddeepnote run --cloud --pushcan build on it rather than reinvent it.cloud-runs.ts— also gainslistNotebookRuns(GET /v2/notebooks/{id}/runs).import.ts—uploadNotebook(bytes, fileName, { domain })→{ importId, launchUrl }: the unauthenticatedPOST /v1/import/init+ presigned PUT. Untouched, and still the right flow fordeepnote open, which has no token and genuinely wants a browser. It was only ever wrong forrunInCloud, which always has one.projects.ts—findNotebook(locate a notebook by project/notebook name viaGET /v2/projects),getWorkspace(GET /v2/me), andnotebookUrl(build the runs deep-link).Design notes
(file, outputs, timing).summary.failedBlocks; only infra/config errors throw (no Python env, missing toolkit, invalid file, missing cloud token, orpersistSnapshotwithout a path).deepnote serveCLI command are intentionally out of scope (future work).Demo
examples/local-runner/— two small reference apps sharing a visual language. run-app builds a control per input and runs the notebook live (a local Python kernel, or Run in cloud viarunInCloud), rendering the returned outputs, with a Cloud runs sidebar listing the notebook's run history in Deepnote — click any run to render its snapshot without re-running it. snapshot-viewer is a fully static page that renders an already-run snapshot — outputs, a chart, and a precomputed agent readout — with no server or kernel. Each is a one-commandpnpm example:*away.Tests
Unit tests mock
ExecutionEngineand@deepnote/cloud(no Python, no network): input coercion/apply, therunWithInputsinvariant + snapshot validity + failure passthrough, the fullrunInCloudflow (by-id, find-by-name, create-if-missing, block-id remapping, refusing to guess which notebook a cloud id means, lookup failures not being read as absence, viewUrl, token-required),createProject(ordering, placeholder cleanup, progress, auth, non-JSON bodies), andlistCloudRuns/getCloudRun, and theserveStaticroutes incl./api/run-cloud, bad-body error JSON, content-type, and an encoded path-traversal rejection. A real end-to-end local test is gated behindDEEPNOTE_TOOLKIT_PYTHON; the cloud path was verified live against the API.@deepnote/local-runner+@deepnote/cloud+@deepnote/cli+@deepnote/blockssuites green (1 skipped: the gated integration test); typecheck + biome clean.🤖 Generated with Claude Code
Summary by CodeRabbit