Repository navigation
feat(@deepnote/mcp): v1 - #273
Conversation
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (38.59%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #273 +/- ##
==========================================
- Coverage 91.98% 82.22% -9.76%
==========================================
Files 96 110 +14
Lines 5474 6696 +1222
Branches 1511 1849 +338
==========================================
+ Hits 5035 5506 +471
- Misses 439 1190 +751 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 26
🤖 Fix all issues with AI agents
In `@examples/mcp-demos/ab_test_evaluator.deepnote`:
- Around line 55-61: The demo cell assumes df exists — add a guard that checks
the dataframe variable (df) before running the exploration prints: verify df is
defined and not None (and optionally not empty) and if the check fails, print a
user-friendly message and skip the rest of the cell; otherwise continue with the
existing print statements (references: the df variable and the exploration
prints for shape, dtypes, isnull().sum()).
In `@examples/mcp-demos/api_analytics_client.deepnote`:
- Around line 89-100: The get method currently retries on any RequestException;
change it to only retry for transient errors: specifically, for HTTP errors
retry only when status is 429 or >=500, and for other RequestException types
(connection/timeout) allow retries; implement this by catching
requests.HTTPError separately (inspect e.response.status_code) inside get,
re-raising immediately for 4xx (except 429), and only sleeping/looping for 429
or >=500, while keeping the existing exponential backoff and final raise when
attempts exhausted; refer to the get method, session, response, and the
exception variable e to locate and update the logic.
In `@examples/mcp-demos/churn_prediction.deepnote`:
- Around line 125-145: The notebook uses plt in the confusion-matrix plotting
section (y_pred/model prediction block and lines where plt.figure, plt.xlabel,
plt.ylabel, plt.title, plt.show are called) but matplotlib.pyplot is not
imported; add an import statement for matplotlib.pyplot (e.g., import
matplotlib.pyplot as plt) at the top of that cell (near other imports such as
sklearn and seaborn) so plt is defined before the plotting code runs.
In `@examples/mcp-demos/etl_data_pipeline.deepnote`:
- Around line 104-130: The transform_data function uses the deprecated pandas
pattern fillna(method='ffill').fillna(method='bfill'); update the df_transformed
null-handling to use the direct methods ffill() and bfill() instead (e.g.,
replace the fillna calls on df_transformed with df_transformed.ffill().bfill())
so the null-forward/backward fills use the modern pandas API while keeping the
logic in transform_data and the df_transformed variable intact.
In `@packages/mcp/README.md`:
- Around line 145-149: The README.md has several fenced code blocks (e.g., the
example block containing "Use deepnote_scaffold with: - description: ... -
outputPath: ...") that lack language specifiers; update each affected fenced
block (lines noted in review: 145, 153, 160, 168, 176, 188, 197) by adding a
language identifier such as "text" or "plaintext" after the opening triple
backticks (e.g., ```text) so the blocks render consistently without syntax
highlighting.
In `@packages/mcp/src/index.ts`:
- Around line 1-17: Update the top-of-file doc comment in the MCP package to
accurately list only the exported/implemented tools: replace the current
enumerated lists with a trimmed API surface that mentions read, cat, validate,
diff, create, add_block, edit_block, remove_block, reorder_blocks, convert_to,
convert_from, run, and snapshot; ensure the module-level comment that
accompanies the exported type DeepnoteMcpServer and the exports createServer and
startServer reflects those exact tool names and removes references to
non-existent items like scaffold, enhance, inspect, lint, bulk_edit, run_block,
and detect_format.
In `@packages/mcp/src/prompts.test.ts`:
- Around line 36-95: Tests miss coverage for the "debug_execution" prompt and
special-character handling; add a Vitest case in prompts.test.ts that calls
getPrompt('debug_execution', { execution_id: 'exec-äöü!@#$', error_message:
'NullPointer¶' }) and assert the returned prompt's description and
result.messages[0].content.text include the provided special-character values
and any expected tokens (e.g., 'deepnote_debug' or prompt-specific keywords),
plus verify message shape (role and type) as in other tests to ensure edge-case
encoding and formatting are handled by getPrompt.
In `@packages/mcp/src/prompts.ts`:
- Around line 3-83: The getPrompt API accepts a plain string which allows
invalid prompt names; derive a type-safe union from the prompts array and use it
in getPrompt to catch invalid names at compile time. Create a type like
PromptName = typeof prompts[number]['name'] and change the getPrompt signature
to getPrompt(name: PromptName, args: Record<string,string>|undefined):
GetPromptResult; update any callers to use the new PromptName type and,
optionally, make the switch exhaustive by replacing the default throw with a
never-check to ensure compile-time coverage of all prompt names (referencing
prompts and getPrompt).
In `@packages/mcp/src/resources.test.ts`:
- Around line 18-69: Add a test to listResources that creates a .deepnote file
with spaces and Unicode in its filename (e.g., "test ñ space.deepnote") inside
tempDir, then call listResources(tempDir) and assert the resource is found
(using resources.find on uri), the resource.name equals the filename without
extension, the resource.mimeType is 'application/x-deepnote', and the
resource.uri contains the URL-encoded filename form (i.e., encodeURIComponent of
the filename) to verify URI handling; locate changes around listResources, the
tests that use resources.find(...), and assertions for mimeType/name to mirror
existing tests.
In `@packages/mcp/src/resources.ts`:
- Around line 196-236: The deepnote://file handling reads parsed.path directly
allowing path traversal; before fs.readFile, resolve and normalize the target
path (using path.resolve/path.normalize) and verify it is inside the provided
workspaceRoot (if workspaceRoot is set) by checking the resolved path startsWith
the resolved workspaceRoot; if the check fails or workspaceRoot is missing and
parsed.path is not already confined, return the existing JSON error response
instead of reading the file. Update the block around parsed.path /
deserializeDeepnoteFile to perform this validation and handle rejection
consistently.
In `@packages/mcp/src/test-tool.ts`:
- Around line 35-46: The help output string labeled "Available tools" is out of
sync with the actual registered tool registry (it lists deepnote_run_block and
deepnote_bulk_edit which are not registered); update the code that builds this
help text (the function/constant that emits the "Available tools" block in
packages/mcp/src/test-tool.ts) to derive the list dynamically from the actual
tool registry (e.g., the registeredTools/TOOL_REGISTRY or toolRegistries
variable) instead of a hardcoded list, or remove the unregistered entries
(deepnote_run_block, deepnote_bulk_edit) so the printed help only contains tools
present in the registry.
- Around line 83-105: The spawn'd child process lacks an 'error' event handler
which can cause unhandled exceptions when the process fails to start; add a
child.on('error', err => { reject(err) }) handler (or reject with a wrapped
Error and optionally log it) next to the existing child.stdout/on and
child.stderr/on listeners so that failures to spawn (in the code surrounding
spawn('node', [serverPath], ...) and the child variable) properly reject the
promise instead of crashing the process.
In `@packages/mcp/src/tools/conversion.ts`:
- Around line 84-171: handleConvertTo and handleConvertFrom assume
args.inputPath exists and is a file, perform auto-detect by reading file content
even when inputPath is a directory, and derive output paths only for specific
extensions; validate and normalize inputs first: in handleConvertTo and
handleConvertFrom, check that args.inputPath is provided and is a string,
resolve it to an absolute path, use fs.stat to verify whether it's a file or
directory, and return a clear error if missing/invalid; for auto-detect
(detectFormat) only read file content when the path is a file (if directory,
treat as batch conversion or error), and update finalOutputPath/finalOutputDir
logic to handle directories and inputs without .ipynb|.qmd|.py extensions (e.g.,
append or replace extension only for files, and if outputPath is a directory
join basename + .deepnote), and cast other args (format, outputPath, outputDir,
projectName) defensively to avoid runtime throws before calling
convertIpynbFilesToDeepnoteFile / convertQuartoFilesToDeepnoteFile /
convertPercentFilesToDeepnoteFile / convertMarimoFilesToDeepnoteFile.
In `@packages/mcp/src/tools/execution.ts`:
- Around line 280-284: The early return when blockIdFilter is set bypasses the
dry-run behavior; modify the block so that before calling handleRunBlock you
check the dryRun flag (the same dryRun passed into the surrounding function) and
if dryRun is true call the dry-run path instead of performing real execution
(either call an existing dry-run helper or add a dryRun parameter to
handleRunBlock and pass it through), ensuring you update handleRunBlock (and its
callers) to accept and respect the dryRun flag so blockId-filtered runs do not
execute when dryRun is true.
- Around line 447-523: handleRunBlock currently calls
engine.runFile(originalPath) which can fail for converted Deepnote formats;
update handleRunBlock to pass the converted DeepnoteFile to the execution engine
(or create a .deepnote snapshot path like snapshot handling) instead of
originalPath. Locate handleRunBlock and replace the engine.runFile(originalPath,
{...}) call with either engine.runProject(file, {...}) or a call that accepts
the DeepnoteFile object (or construct and pass the .deepnote path derived from
originalPath as used in snapshot logic), ensuring the same options
(notebookName, blockId, inputs) are forwarded and that
engine.start()/engine.stop() usage remains unchanged.
In `@packages/mcp/src/tools/reading.ts`:
- Around line 149-162: The current blocks construction in
packages/mcp/src/tools/reading.ts truncates block IDs (block.id.slice(0, 8)) for
both the returned id and dependency entries which can cause collisions; change
the returned object in the mapping over blockDeps to use the full block.id for
id and to push full definingBlock (not definingBlock.slice(0,8)) into dependsOn,
and if you need a short display ID add a separate shortId field computed from
block.id; update references to id/dependsOn consumers if they expect short IDs.
- Around line 299-307: In handleRead, validate that args.path is a string and
args.include is a non-empty array before using them: if args.path is missing or
not a string (filePath) return/throw a clear error; for includeRaw, only accept
it when Array.isArray(includeRaw) and includeRaw.length>0, otherwise fall back
to the default ['structure']; then build include as new Set(validInclude) and
compute includeAll from include.has('all'). Update references to filePath,
includeRaw, include, and includeAll accordingly to avoid downstream type errors.
In `@packages/mcp/src/tools/snapshots.test.ts`:
- Around line 55-87: Add a test case under the existing deepnote_snapshot_split
suite that verifies snapshot tools handle paths with special characters (spaces
and unicode); call handleSnapshotTool with path derived from testNotebookPath
but placed into a tempDir subfolder that includes spaces/unicode (e.g.,
`${tempDir}/snap dir-ü`), then assert extractResult().success is true and that
sourcePath/snapshotPath/latestPath exist and point to files by using fs.stat
like the other tests; reference the existing tests and helpers
(handleSnapshotTool, extractResult, testNotebookPath, tempDir) so the new case
mirrors the "splits source from outputs" and "creates latest snapshot by
default" assertions but uses the special-character path.
In `@packages/mcp/src/tools/snapshots.ts`:
- Around line 413-417: The variable skipMismatched is currently set with
(args.skipMismatched as boolean) || false which treats any truthy non-boolean
(e.g. non-empty string) as true; update the assignment so it normalizes to a
real boolean by explicitly checking the type or parsing string values—e.g. if
typeof args.skipMismatched === 'boolean' use it, otherwise convert
String(args.skipMismatched).toLowerCase() === 'true'—so the symbol
skipMismatched always ends up a boolean and does not coerce arbitrary truthy
values.
- Around line 78-111: The tool definition for 'deepnote_snapshot_split'
incorrectly marks the operation as idempotent; update the annotations object on
the deepnote_snapshot_split tool to set idempotentHint: false and
destructiveHint: true so the runtime knows this rewrites the source and creates
new snapshots each run; locate the annotations block in the
deepnote_snapshot_split tool definition and change those two boolean flags
accordingly.
- Around line 323-326: The keepLatest flag is currently computed as const
keepLatest = args.keepLatest !== false which treats the string "false" as
truthy; change it so undefined still defaults to true but other values are
normalized to a real boolean: if args.keepLatest is undefined use true, else if
it's a string compare lowercased value to "true", otherwise coerce with
Boolean(args.keepLatest). Update the expression that sets keepLatest (the const
keepLatest assignment) accordingly so it reliably interprets "false"/"true"
strings and non-boolean types.
In `@packages/mcp/src/tools/writing.ts`:
- Around line 680-706: The reorder logic currently allows duplicate entries in
blockIds to produce duplicated blocks in reorderedBlocks; while iterating and
resolving each id (using resolveBlockId) build resolvedIds but check for
duplicates (use a Set like resolvedIdSet) and if a resolved fullId is already
present return an error (or reject the request) instead of pushing it again;
update the resolution loop that populates resolvedIds (and the later
resolvedIdSet used for appending remaining notebook.blocks) to enforce
uniqueness so reorderedBlocks cannot contain the same block twice.
- Around line 433-443: The insertion logic currently in the block that computes
insertIndex silently falls back to end when position.after or position.before
targets are not found and allows negative or out-of-range position.index values;
update the code around findBlockIndex/notebook.blocks/position to validate
inputs and reject invalid targets by throwing a clear error: if position.after
or position.before is provided and findBlockIndex(...) returns -1, throw a
descriptive exception (e.g., "Invalid position.after: block not found"); if
position.index is provided, ensure it is an integer within
0..notebook.blocks.length (reject negative or >length values) instead of
allowing negative indexing or clamping silently. Ensure these checks occur
before computing insertIndex so callers receive immediate, explicit failures for
invalid positions.
- Around line 7-27: The current prefix-matching helpers (findBlock,
findBlockIndex, resolveBlockId) silently return the first match which allows
ambiguous prefixes to target the wrong block; update these functions so they
detect multiple matches and treat ambiguous prefixes as errors: change findBlock
to gather all blocks whose id === idOrPrefix or id.startsWith(idOrPrefix) and if
more than one match exists throw a descriptive Error about ambiguous block ID
prefix (only return the single match when exactly one found), change
findBlockIndex to similarly require a unique match (return the index when
unique, throw on ambiguity, and return -1 only when no matches), and update
resolveBlockId to throw on ambiguous matches instead of returning the first id
(or return undefined only when no matches) so callers get a clear failure for
ambiguous prefixes.
- Around line 337-345: Replace the unchecked casts in handleCreate with explicit
runtime validation: verify args.outputPath and args.projectName are non-empty
strings, args.notebooks is an array and each notebook has a string name and
blocks array with objects containing a string type and optional string
content/metadata, and args.dryRun is boolean or undefined; if validation fails,
throw or return a clear error. Implement or call a small reusable validator
(e.g., validateHandlerArgs or a schema via a lightweight validator) and use it
at the top of handleCreate to sanitize/normalize outputPath, projectName,
notebooks, and dryRun before using them.
In `@packages/mcp/src/utils.ts`:
- Around line 26-38: Change the formatOutput signature to accept a stricter type
(e.g., use Record<string, unknown> instead of plain object) and update the
internal handling to use safe type narrowing when iterating entries: update the
function declaration for formatOutput(data: Record<string, unknown>, compact:
boolean): string, ensure Object.entries(data) is treated as [string, unknown]
pairs, add explicit type checks before treating values as arrays or plain
objects (Array.isArray(v) and typeof v === 'object' && v !== null), and update
any call sites that pass non-conforming types to satisfy the new signature.
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In `@examples/mcp-demos/ab_test_evaluator.deepnote`:
- Around line 88-105: The notebook renders an empty figure because plt.figure(),
plt.tight_layout(), and plt.show() run while all example plotting lines (e.g.,
sns.histplot and sns.heatmap) are commented out; update the code in this cell to
either remove those scaffold calls or gate them behind a data check before
creating/ showing a figure (for example, check that df exists and not df.empty
before calling plt.figure()/sns.histplot()/sns.heatmap() and then
plt.tight_layout()/plt.show()), or uncomment one of the example plotting lines
(sns.histplot or sns.heatmap) so the figure has content.
- Around line 72-78: Wrap the df.describe() call in the same guard used in the
earlier exploration block to avoid NameError when df doesn't exist;
specifically, check for the presence and non-emptiness of df (e.g., if 'df' in
globals() and df is not None and not df.empty) before calling df.describe(),
ensuring you reference the df and df.describe() symbols so the cell only
executes when the dataframe is available.
In `@examples/mcp-demos/api_analytics_client.deepnote`:
- Around line 89-111: In get(), the final "return None" is unreachable because
the for-loop either returns a JSON, raises, or continues; remove the unreachable
return and instead ensure the function's control flow is explicit—either delete
the trailing "return None" or add an explicit raise (e.g., raise
RuntimeError("Request failed after retries")) so callers don't receive an
implicit None; update the function named get in the API client accordingly.
In `@examples/mcp-demos/churn_prediction.deepnote`:
- Around line 153-163: The big-number widget references deepnote_variable_name
kpi_value but no prior cell assigns it, so add a code cell executed before the
big-number block that computes/assigns kpi_value (e.g., derive it from your
dataset or model output and set kpi_value = <numeric>). Make sure the new cell
runs before the big-number block (so executionCount/order is correct) and that
kpi_value is a numeric scalar compatible with the big-number template ${value}.
- Around line 75-78: Remove the unused import StandardScaler from the top of the
notebook code block (the import line that reads "from sklearn.preprocessing
import StandardScaler") or use it where appropriate; either delete that token
from the import statement or apply StandardScaler to scale features (e.g.,
instantiate and fit/transform in the data preprocessing steps such as before
train_test_split or inside the preprocessing pipeline) so the symbol is actually
referenced.
In `@packages/mcp/src/server.ts`:
- Around line 96-104: The routing uses fragile prefix matching on the variable
name and should instead match exact tool names; replace the startsWith checks
with an explicit whitelist of valid writing tool names (e.g., declare a const
array like writingTools containing the exact strings for add_block,
add_notebook, edit_block, remove_block, plus any others) and change the
condition to check writingTools.includes(name) (or use an exact-match regex
list) before calling handleWritingTool(name, args) so future tools like
deepnote_add_user won’t be misrouted.
In `@packages/mcp/src/test-tool.ts`:
- Around line 10-14: The file-level docstring examples reference nonexistent
tools (deepnote_inspect, deepnote_scaffold, deepnote_template); update the
header comment examples to match the actual implemented tool names produced by
the runtime help block around the help-printing code (the help generation /
runtime help logic in test-tool.ts), removing or replacing the deepnote_*
entries so the top-of-file docstring is consistent with the runtime help output.
In `@packages/mcp/src/tools/conversion.ts`:
- Around line 114-139: The function resolveConvertToOutputPath currently treats
a non-existent outputPath (resolvedOutputPath) without a .deepnote extension as
a directory; instead, change the logic so that only when resolvedOutputPath
exists and fs.stat(...).isDirectory() is true do we treat it as a directory
(return path.join(resolvedOutputPath, `<basename>.deepnote`) ), otherwise treat
the resolvedOutputPath as the intended file: if resolvedOutputPath already
endsWith '.deepnote' return it, else return resolvedOutputPath + '.deepnote'.
Update the try/catch and the subsequent extension check in
resolveConvertToOutputPath accordingly (keeping deriveDefaultDeepnoteOutputPath
fallback when outputPathRaw is undefined).
In `@packages/mcp/src/tools/execution.ts`:
- Around line 280-283: The call to handleRunBlock currently passes a boolean
positional arg (dryRun === true), which is unclear; change handleRunBlock to
accept a single options object (e.g., handleRunBlock(file, originalPath,
blockIdFilter, notebookFilter, pythonPath, inputs, { dryRun: boolean }) ),
update its implementation to read opts.dryRun, and change all call sites
(including the one in this snippet and the other occurrence around the 447-455
region) to pass an options object instead of a raw boolean so the call is
self-describing.
- Around line 267-283: handleRun currently asserts args.path and calls
resolveAndConvertToDeepnote outside any error handling so missing/invalid paths
or conversion/parsing errors throw raw exceptions; add a type guard to validate
filePath (from args.path) before use and wrap the call to
resolveAndConvertToDeepnote in a try/catch that returns a structured error
object (e.g., { isError: true, error: <Error>, code?: <string> }) instead of
throwing, then short-circuit handleRun to return that structured error;
reference symbols: handleRun, args.path, filePath, resolveAndConvertToDeepnote,
and ensure callers (e.g., handleRunBlock branch) still run only when a valid
file is returned.
In `@packages/mcp/src/tools/reading.ts`:
- Around line 301-544: Handlers handleRead, handleCat, handleValidate and
handleDiff currently call loadDeepnoteFile and path.resolve/fs.readFile without
guarding against thrown errors or invalid arg types; wrap each file-loading call
in a try/catch and validate arg types up front (e.g., ensure args.path /
args.path1 / args.path2 are strings) so failures return a structured error
response with isError: true (or content with JSON indicating valid:false for
validate) instead of throwing. Specifically, in handleRead and handleCat
validate filePath as string before calling loadDeepnoteFile and wrap
loadDeepnoteFile(...) in try/catch to return an object with content containing
an error message and isError: true; in handleDiff validate path1/path2 and wrap
both loadDeepnoteFile calls in try/catch and return a structured error object on
failure; in handleValidate guard path string and wrap path.resolve and
fs.readFile in try/catch (keeping existing JSON error shape), ensuring all
caught errors convert to clear error messages rather than letting exceptions
escape.
In `@packages/mcp/src/tools/snapshots.ts`:
- Around line 322-332: The keepLatest computation in handleSnapshotSplit uses a
nested ternary to produce a boolean (variable keepLatest), which is hard to
read; replace it with explicit branching: inspect keepLatestRaw and set
keepLatest via an if/else (or simple Boolean coercion plus a string check) so
the logic is clear—e.g., check if keepLatestRaw is undefined then set true, else
if it's a string compare lowercased value to 'true', otherwise set
Boolean(keepLatestRaw); update only the keepLatest assignment logic in function
handleSnapshotSplit referencing args.path, args.snapshotDir, and
keepLatestRaw/keepLatest.
In `@packages/mcp/src/tools/writing.ts`:
- Around line 772-775: The code assumes file.project.notebooks has at least one
entry by assigning let notebook = file.project.notebooks[0], which will throw on
empty projects; update the logic in the reorder flow (referencing notebook,
notebookFilter, and file.project.notebooks) to first check if
file.project.notebooks is empty and return/throw a clear, descriptive error
(e.g., "no notebooks in project") before accessing index 0, otherwise proceed to
find by notebookFilter and set notebook accordingly.
- Around line 516-520: The code defaults notebook = file.project.notebooks[0]
without checking for an empty notebooks array, which causes a crash when
accessing notebook.blocks; add a guard before that defaulting: if
file.project.notebooks.length === 0 return or throw a clear error (e.g., "No
notebooks found for file <file.id or file.name>") so callers get a helpful
message; keep the existing notebookFilter logic (the find on
file.project.notebooks and the !found branch) but ensure the empty-array check
runs first to avoid indexing into [] and later uses of notebook.blocks.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@packages/mcp/src/server.ts`:
- Around line 101-111: The routing still uses prefix matching for conversion and
snapshot tools; replace those startsWith checks with explicit sets of allowed
tool names to match the pattern used for the execution tools: create IDENTIFIERS
like CONVERSION_TOOL_NAMES and SNAPSHOT_TOOL_NAMES (or similar) and check
membership (e.g., set.has(name)) before calling handleConversionTool(name, args)
and handleSnapshotTool(name, args); update the logic around the current name
variable so only exact, whitelisted names invoke handleConversionTool and
handleSnapshotTool to avoid misrouting.
In `@packages/mcp/src/tools/snapshots.ts`:
- Around line 424-426: The current coercion for skipMismatched is subtle when
args.skipMismatched is undefined; update the logic in the block referencing
skipMismatchedRaw and skipMismatched so it's explicit: keep the existing boolean
branch (typeof skipMismatchedRaw === 'boolean'), then treat null/undefined as
false (skipMismatched = false), and otherwise coerce by lowercasing
String(skipMismatchedRaw) === 'true'. This makes intent clear and avoids relying
on String(undefined) behavior.
In `@packages/mcp/src/tools/writing.ts`:
- Around line 697-700: Validate the inputs to handleRemoveBlock: ensure
args.path and args.blockId exist and are non-empty strings (and args.dryRun, if
present, is boolean), mirroring the validation pattern used elsewhere; if
validation fails, throw or return a clear error (or early reject) with a
descriptive message referencing filePath and blockId so downstream code in
handleRemoveBlock cannot run with invalid values. Use the function name
handleRemoveBlock and the local variables filePath, blockId, and dryRun to
locate where to add these checks.
- Around line 880-886: handleAddNotebook currently casts args.path and args.name
without validation which will crash if missing or wrong type; add explicit
argument validation at the top of handleAddNotebook: verify args.path and
args.name are present and are strings (throw a clear Error or return early with
a message if not), validate that args.blocks (if provided) is an array of
objects with expected keys and that args.dryRun is boolean (or set a default
false), and normalize/assign safe defaults (e.g., blocks = [] when undefined)
before proceeding; reference the function handleAddNotebook and the variables
filePath, name, blocks, and dryRun when adding these checks and error messages.
- Around line 614-619: The handleEditBlock function currently casts args.path
and args.blockId (and other fields) without validation which can cause runtime
errors when missing or wrong-typed; update handleEditBlock to validate that
args.path and args.blockId exist and are strings (and that args.content is
string|undefined, args.metadata is object|undefined, args.dryRun is
boolean|undefined) before calling loadDeepnoteFile or other logic, and
return/throw a clear error when validation fails; reference the variables
filePath, blockId, newContent, newMetadata, dryRun and the call to
loadDeepnoteFile in your checks so invalid inputs are rejected early with a
helpful message.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/mcp/src/tools/snapshots.ts`:
- Around line 154-162: The handlers (e.g., handleSnapshotList) currently cast
args.path/args.sourcePath with "as string" before checking for presence, which
lets non-string values slip through; change each snapshot handler to first
validate the argument type (e.g., if (typeof args.path !== 'string' ||
args.path.trim() === '') return the error payload) rather than casting, and
mirror the pattern used in handleEditBlock for validating string inputs; apply
the same type-first validation to snapshotDir/sourcePath parameters in the other
snapshot handlers in this module and keep the existing error response shape.
In `@packages/mcp/src/tools/writing.ts`:
- Around line 836-840: In handleReorderBlocks validate incoming args before
casting: ensure args.path is a non-empty string, args.blockIds exists and is an
array of strings (use Array.isArray(blockIds) and blockIds.every(id => typeof id
=== 'string')), coerce or default args.dryRun to a boolean (e.g., false) and
ensure args.notebook (notebookFilter) is either undefined or a string; if
validations fail, throw a clear error or return a rejected Promise so the caller
gets a deterministic failure instead of runtime exceptions. Use the function
name handleReorderBlocks and the variables filePath, blockIds, dryRun,
notebookFilter when updating the checks and error messages so changes are easy
to locate.
- Around line 503-512: handleAddBlock currently casts args.path, args.block,
args.position, and args.dryRun without validation; add explicit runtime checks
at the start of handleAddBlock to validate that path is a non-empty string,
block is an object with a string type (and optional content as string and
optional metadata as an object), position—if present—matches { after?: string;
before?: string; index?: number }, and dryRun—if present—is a boolean; throw or
return a clear error when validation fails so callers don’t hit cryptic runtime
errors and downstream functions that use filePath, blockSpec, position, or
dryRun can assume correct types.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/mcp/src/tools/snapshots.ts`:
- Around line 195-196: Replace uses of the logical-OR fallback pattern with
explicit ternaries: for example change the declaration using
`resolvedSnapshotDir = snapshotDir || path.join(projectDir, 'snapshots')` to an
explicit `resolvedSnapshotDir = snapshotDir ? snapshotDir :
path.join(projectDir, 'snapshots')`; apply the same change to the other
occurrences called out in this file (the other fallback assignments referenced
in the review) so each fallback uses `condition ? value : fallback` instead of
`||`, keeping the same expressions and behavior.
- Around line 15-18: The snapshotError function returns a successful-tool-like
payload, so MCP clients treat errors as successes; update snapshotError (the
function named snapshotError in this file) to include isError: true in the
returned object alongside the existing content so callers receive a proper error
signal (preserve the existing content shape but add the top-level isError: true
flag).
In `@packages/mcp/src/tools/writing.ts`:
- Around line 428-435: The object initializer using `spec.content || ''` and
`spec.metadata || {}` should use explicit ternary fallbacks to avoid relying on
falsy semantics; update the `base` construction (where `id`, `blockGroup`,
`sortingKey`, `type`, `content`, `metadata` are set — the
`randomUUID()`/`generateSortingKey()` block) to use `spec.content !== undefined
? spec.content : ''` and `spec.metadata !== undefined ? spec.metadata : {}` (and
similarly replace other `||` occurrences referenced in the comment with explicit
ternaries) so missing values default correctly without treating valid falsy
values as absent.
- Around line 454-456: validateCreateArgs can throw and currently its call
inside async function handleCreate will bubble unhandled errors; wrap the call
to validateCreateArgs(...) in a try/catch inside handleCreate and on catch
return the standardized writingError with the caught error (e.g., return
writingError(err)) so expected validation failures produce the structured
writingError response instead of an unstructured exception. Ensure you only
catch validation errors from validateCreateArgs and preserve existing flow when
validation succeeds.
tkislan
left a comment
There was a problem hiding this comment.
There is quite a lot of force casting happening when using the input
I understand that there is tool definitions with schema, but I would still use zod schemas to validation unknown values into proper typescript types
| if ( | ||
| name === 'deepnote_read' || | ||
| name === 'deepnote_cat' || | ||
| name === 'deepnote_validate' || | ||
| name === 'deepnote_diff' |
There was a problem hiding this comment.
readingToolNames? similarly as with other groups
| * Python builtins set for detecting undefined variables in notebook code. | ||
| * Only includes actual Python builtins, not common library aliases. | ||
| */ | ||
| export const PYTHON_BUILTINS = new Set([ |
There was a problem hiding this comment.
This is duplicated from cli .. would make sense to extract into a common code? .. I'm not 100% sure if the package build would handle that without issues
There was a problem hiding this comment.
Good callout, I think we can centralize this in the packages/reactivity and import from there.
| Execution outputs are saved to snapshot files (.snapshot.deepnote). Use \`deepnote_snapshot_load\` to inspect them -- do NOT read snapshot files directly. | ||
|
|
||
| **Execution scopes (smallest to largest):** | ||
| 1. \`deepnote_run blockId=X\` - One block + dependencies (use during development) |
There was a problem hiding this comment.
Just an idea, if it makes sense to use typescript strings/enums in these instructions and prompts, to make sure they match as they should, and we would also get some degree of type check
There was a problem hiding this comment.
It does, it would be way better, will fix! 👍
| 'Run notebook locally. Supports .deepnote, .ipynb, .py, .qmd. Use blockId to run a single block. Returns outputs inline by default (includeOutputSummary=true).', | ||
| annotations: { | ||
| readOnlyHint: false, | ||
| destructiveHint: false, |
There was a problem hiding this comment.
shouldn't this be true ? .. as notebook execution can in fact be destructive
There was a problem hiding this comment.
Not sure I understand this one, in what way can notebook execution be destructive in relation to the .deepnote file we are editing?
There was a problem hiding this comment.
not to the .deepnote file itself, but to the environment around it 😊
There was a problem hiding this comment.
My understanding of destructiveHint is that it references the file itself, but I might be wrong.
There was a problem hiding this comment.
then feel free to disregard this comment
| const base = { | ||
| id: randomUUID(), | ||
| blockGroup, | ||
| sortingKey: generateSortingKey(index), | ||
| type: spec.type, | ||
| content: spec.content !== undefined ? spec.content : '', | ||
| metadata: spec.metadata !== undefined ? spec.metadata : {}, | ||
| } | ||
|
|
||
| // Add execution fields for executable blocks | ||
| if ( | ||
| ['code', 'sql', 'notebook-function', 'visualization'].includes(spec.type) || | ||
| spec.type.startsWith('input-') || | ||
| spec.type === 'button' || | ||
| spec.type === 'big-number' | ||
| ) { | ||
| return { | ||
| ...base, | ||
| executionCount: null, | ||
| outputs: [], | ||
| } as DeepnoteBlock | ||
| } | ||
|
|
||
| return base as DeepnoteBlock |
There was a problem hiding this comment.
this should use zod schema for type instead of force casting
There was a problem hiding this comment.
Good idea, I'll apply zod schemas throughout the mcp package, will make things much simpler and explicit.
| const notebookFilter = args.notebook as string | undefined | ||
| const compact = args.compact as boolean | undefined |
There was a problem hiding this comment.
zod schema instead of force casting
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |

Copy of #255 plus additional fixes.
For instructions on how to use: #255 (comment)
Summary by CodeRabbit
New Features
New Examples
Documentation
Dictionary
Tests