feat(@deepnote/mcp): v1 #255
Draft
+10,723
−25
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jamesbhobbs
commented
Jan 28, 2026
| "mcpServers": { | ||
| "deepnote": { | ||
| "command": "npx", | ||
| "args": ["@deepnote/mcp"] |
Contributor
Author
There was a problem hiding this comment.
Before publishing use
"deepnote": {
"command": "node",
"args": ["/Users/james/dev/deepnote/deepnote/packages/mcp/dist/bin.js"]
},
Contributor
📝 WalkthroughWalkthroughAdds a new packages/mcp package implementing a stdio-backed Model Context Protocol (MCP) server for Deepnote. Exports createServer, startServer, a CLI entry and test CLI, plus serverInstructions, prompts, resource discovery/readers, and grouped toolsets (magic, reading, writing, conversion, execution, snapshots) with dispatch handlers. Implements conversion, execution, snapshot, reading, writing, and magic tooling, resource enumeration/reading, example notebooks/snapshots, package manifest, build/test tooling, and comprehensive unit tests. No changes to other packages' public APIs. Sequence Diagram(s)sequenceDiagram
actor Client
participant MCP as "MCP Server (stdio)"
participant Dispatcher as "Request Dispatcher"
participant Tools as "Tool Handlers"
participant FS as "File System"
participant Examples as "Built-in Examples"
participant Cloud as "Deepnote Cloud (optional)"
Client->>MCP: send JSON-RPC request
MCP->>Dispatcher: forward request
alt ListTools / ListPrompts
Dispatcher-->>MCP: return catalogs
else ListResources
Dispatcher->>Examples: enumerate built-in examples
Dispatcher->>FS: scan workspace for .deepnote files
Examples-->>Dispatcher: examples list
FS-->>Dispatcher: workspace file list
Dispatcher-->>MCP: resources response
else ReadResource
Dispatcher->>FS: read file or examples content
FS-->>Dispatcher: parsed summary or error
Dispatcher-->>MCP: resource contents
else CallTool
Dispatcher->>Tools: route to handler (magic/reading/writing/conversion/execution/snapshots)
Tools->>FS: read/write/convert/serialize as needed
alt execution open/upload
Tools->>Cloud: upload/import file
Cloud-->>Tools: importId / launch URL
end
Tools-->>Dispatcher: structured result or error
Dispatcher-->>MCP: tool response
else GetPrompt
Dispatcher-->>MCP: constructed prompt payload
end
MCP-->>Client: JSON-RPC response
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## cli-comb2 #255 +/- ##
==============================================
- Coverage 92.11% 72.81% -19.30%
==============================================
Files 84 96 +12
Lines 5071 6909 +1838
Branches 1367 1977 +610
==============================================
+ Hits 4671 5031 +360
- Misses 400 1877 +1477
- Partials 0 1 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* docs(examples): mcp generated notebooks * fix: inputs handling by mcp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 27
🤖 Fix all issues with AI agents
In `@packages/mcp/package.json`:
- Around line 45-51: Add the token "modelcontextprotocol" to the cspell
dictionary by updating the cspell.json "words" array so the dependency
"@modelcontextprotocol/sdk" in package.json won't be flagged; locate cspell.json
and append "modelcontextprotocol" to the existing words list to allow that
package name.
In `@packages/mcp/README.md`:
- Line 167: Add the term "pytest" to the project's cspell dictionary so the
spellchecker no longer flags it: update the cspell configuration (e.g.,
cspell.json or .cspellrc) to include "pytest" in the "words" array or custom
dictionary, then commit the change so CI recognizes the term and the pipeline
stops failing on this common testing term.
- Line 9: The CSpell failure is caused by the word "toolsets" in the README line
"**Bounded toolsets**": either add "toolsets" to the cspell dictionary (e.g.,
add "toolsets" to your cspell.json word list) or edit the README to rephrase the
phrase to "Bounded tool sets" (two words) so the spell checker no longer errors;
update the README.md line accordingly or update the project's cspell config to
include the new word.
- Around line 124-128: Multiple fenced code blocks in the README (the examples
starting with "Use deepnote_scaffold with:" and the similar blocks later) lack a
language identifier, triggering MD040; update each fenced block by adding a
language tag such as text or plaintext (e.g., change ``` to ```text) for the
examples at the occurrences containing the phrases "Use deepnote_scaffold with:"
and the other similar example blocks so Markdownlint no longer flags them.
In `@packages/mcp/src/resources.ts`:
- Around line 13-19: The type BlobResourceContents is defined but never used;
either remove the BlobResourceContents declaration and update the union type
alias ResourceContents to only reference TextResourceContents, or if it's
intended for future use, add a clear TODO comment above BlobResourceContents
indicating planned usage. Also scan for and update any references to
ResourceContents or related types (e.g., TextResourceContents) to ensure the
union remains correct after removing or preserving BlobResourceContents.
- Around line 44-46: The empty catch block in resources.ts silently swallows
filesystem errors; change the catch to capture the exception (e.g., catch (err))
and either log the error with context (include the directory path/operation and
err) using the existing logger or surface a partial-result + error object to the
caller; update the block that currently reads "catch { // Directory not
accessible, skip }" to log or return error details so failures are visible
during debugging.
- Line 4: The CI spellcheck flags the import specifier '@modelcontextprotocol'
in packages/mcp/src/resources.ts; update your repository CSpell dictionary
(e.g., .cspell.json or cspell.words referenced by your config) to include the
token "modelcontextprotocol" (or the full module name) so the import string is
accepted; locate the CSpell config file used by the pipeline and add
"modelcontextprotocol" to the "words" array (matching the change you made for
server.ts) and commit that change.
- Around line 92-102: The current code pushes resources using the absolute
filesystem path (variable file) into the URI, leaking system structure; change
the URI construction in the loop that iterates workspaceFiles to use the
computed relativePath (e.g., uri: `deepnote://file/${relativePath}`) instead of
`file`, and update the corresponding readResource function to resolve that
relative path against workspaceRoot when opening/reading files so resource
lookup still points to the correct absolute file. Ensure references to
DEEPNOTE_EXTENSION, name and mimeType remain unchanged and that readResource
correctly normalizes/joins the relative path to workspaceRoot before filesystem
access.
- Around line 61-65: The current URI parsing function that returns {type:
'file', path: ...} for any non-matching deepnote:// value will misclassify
malformed URIs; change that parser so it returns an explicit error/unknown
result (e.g., { type: 'unknown', uri }) or throws an Error when the URI doesn't
match known patterns instead of defaulting to file, and then update readResource
to detect this 'unknown' result and handle it by throwing a clear error or
returning a rejected Promise with a descriptive message (use the parser function
name and readResource to locate the changes).
- Line 116: The current examplesDir assignment uses __dirname which is undefined
in ESM; update the code that sets examplesDir (the constant named examplesDir
where path.resolve is used) to derive the directory from import.meta.url via
fileURLToPath (import fileURLToPath from 'url') and path.dirname, or
alternatively make the examples path configurable and read it from env/config;
ensure you replace all references to __dirname and keep the resolved path logic
using path.resolve(path.dirname(fileURLToPath(import.meta.url)),
'../../../examples') or a configurable fallback.
- Around line 191-233: The code reads parsed.path directly via fs.readFile
(inside the block handling parsed.type === 'file') which allows path traversal;
fix by resolving and normalizing the input and ensuring the resolved absolute
path is inside the intended workspace root before reading: compute an
absolutePath from parsed.path (using path.resolve with the workspaceRoot),
verify absolutePath.startsWith(workspaceRoot) (or use path.relative and ensure
it does not start with '..'), and if the check fails return the same JSON error
response instead of calling fs.readFile and deserializeDeepnoteFile; keep
existing error handling and use the same uri/mimeType response shape when
rejecting.
In `@packages/mcp/src/server.ts`:
- Around line 1-10: The spellchecker flags the '@modelcontextprotocol'
identifier as unknown; add "modelcontextprotocol" (and optionally
"@modelcontextprotocol") to the project's cspell dictionary (e.g., cspell.json
"words" or cspell section in package.json) so imports like Server and
StdioServerTransport from '@modelcontextprotocol/sdk/...' are accepted; update
the config, commit the change, and re-run the pipeline.
- Around line 146-150: startServer currently creates the server and connects the
StdioServerTransport but doesn't handle process shutdown; add signal handlers
for SIGINT and SIGTERM that perform graceful cleanup by calling
server.disconnect() (or server.close() if that's the API) and disposing/closing
the StdioServerTransport instance, and ensure the handlers remove themselves
after running to avoid duplicate execution; locate startServer, createServer,
StdioServerTransport and the server.connect call to add this shutdown logic and
ensure startServer awaits the disconnect/cleanup before exiting.
- Around line 84-128: The prefix-based startsWith routing in server.ts is
fragile and repetitive; replace the long startsWith chains with an explicit
name-to-handler registry: build a Map of exact tool names to handler categories
(e.g., populate from your magicTools/readingTools/writingTools lists or from an
allTools array that includes a category), then lookup the incoming name and
dispatch to the appropriate handler function (handleMagicTool,
handleReadingTool, handleWritingTool, handleConversionTool, handleExecutionTool)
using that map; this ensures exact matching, avoids prefix collisions (e.g.,
deepnote_fix vs deepnote_fix_all), and centralizes routing logic for easier
maintenance.
- Around line 55-63: The request handler for GetPromptRequestSchema catches
errors and rethrows a new Error which discards the original stack; update the
handler (the async callback passed to server.setRequestHandler for
GetPromptRequestSche
10BC0
ma that calls getPrompt) to either rethrow the caught error
directly (throw error) or rethrow preserving cause (throw new Error(message, {
cause: error })) so the original stack is retained; keep the existing message
extraction (message) if you choose cause, and ensure the thrown value is the
original error or wraps it via the cause option.
- Around line 21-26: The module-level constant workspaceRoot is computed at
import time (const workspaceRoot = process.env.DEEPNOTE_WORKSPACE ||
process.cwd()), which prevents updates for tests or runtime changes; remove that
top-level evaluation and instead compute workspaceRoot inside createServer() or
add a createServer(workspaceRoot?: string) parameter with a default of
process.env.DEEPNOTE_WORKSPACE || process.cwd(); update all usages in this file
to reference the local workspaceRoot variable (or the passed parameter) rather
than the removed module-level symbol so tests can inject isolated values and
runtime env changes take effect.
In `@packages/mcp/src/tools/conversion.ts`:
- Around line 118-123: The current computation of finalOutputPath and
finalProjectName uses a case-sensitive regex on absoluteInput which can fail for
uppercase extensions or extensionless files and may overwrite the source; update
the logic in conversion.ts to use path.parse on absoluteInput and always replace
or append the .deepnote extension when outputPath is not provided, and derive
finalProjectName from the parsed name when projectName is not provided; ensure
you use the existing variables finalOutputPath, finalProjectName, absoluteInput,
outputPath, and projectName when implementing this change.
In `@packages/mcp/src/tools/execution.ts`:
- Around line 210-217: The current loop uses b.id.startsWith(blockId) which can
match multiple blocks; update the lookup so you first try an exact match (b.id
=== blockId) across file.project.notebooks and use that if found; if no exact
match and you must support prefix lookup, collect all blocks where
b.id.startsWith(blockId) and only assign targetBlock/targetNotebook when that
collection has exactly one element, otherwise raise/return an explicit error or
validation message indicating ambiguity so callers can disambiguate; ensure you
update any logic that assumed a single match to handle the error case.
In `@packages/mcp/src/tools/magic.ts`:
- Around line 2112-2254: handleRefactor currently copies
extractedFunctions/extractedClasses into a new module notebook but leaves the
original cells intact and doesn't add an import; update handleRefactor to (1)
remove the extracted function/class source lines from their original Deepnote
blocks (use extractedFunctions[*].blockId and extractedClasses[*].blockId to
locate blocks and modify block.content) so the original definitions are deleted
or replaced with a short placeholder, (2) insert a new import code block into
the original notebook(s) (use createBlock to create a code cell that does either
"from <moduleName> import ..." or "import <moduleName> as <alias>" listing the
extracted names), and (3) ensure you persist these changes by calling
saveDeepnoteFile(filePath, file) after mutating the original notebooks; keep
existing module creation (moduleId, moduleBlocks) but perform the mutation+save
before final return.
- Around line 892-1053: The handleEnhance function currently only mutates
notebook.blocks for documentation while inputs/visualizations/structure only
append to the changes array but are never applied; either implement their
mutations or mark them as suggestions and not count them as applied—do the
latter now: create a separate suggestions array (or add an applied: false flag)
for non-documentation findings (inputs, visualizations, structure), stop
mutating notebook.blocks for those, and change the final returned payload and
changesApplied computation so only actually applied changes (those that caused
notebook.blocks modifications via blocksToInsert and the sortingKey update) are
counted; adjust the places that push into changes (refer to the patterns loop,
the visualization check, and the structure header detection) to push into
suggestions or set applied:false, and ensure saveDeepnoteFile and the returned
JSON use changesApplied based on applied changes only.
- Around line 1148-1151: The current circular-dependency branch is a no-op;
replace it with real handling: when (fixAll || fixTypes.includes('circular'))
call a detection routine (e.g., detectCircularDependencies(projectGraph)) and if
cycles are found either attempt to resolve them via a reordering/breaking helper
(e.g., breakCircularDependency or topologicalSortFallback) or immediately fail
with a clear error (throw new Error('circular fix requested but not implemented;
found cycles: ' + JSON.stringify(cycles))). Ensure you reference the existing
variables fixAll and fixTypes and add/implement detectCircularDependencies and
breakCircularDependency helpers or, at minimum, throw the error so the request
is not silently ignored.
- Around line 2511-2528: The code unconditionally calls JSON.parse on resultText
(twice), causing non-JSON outputs (e.g., markdown from deepnote_explain) to
throw and mark steps failed; change the logic to attempt JSON.parse once inside
the existing try/catch (assign to a local parsed variable), use that parsed
value for lastOutputPath and for the results.push entry, and if parsing fails
set the result field to the raw resultText (not undefined) so non-JSON outputs
are preserved; update references to resultText, parsed, lastOutputPath, and the
results.push creation accordingly so parsing errors do not bubble up.
- Around line 1395-1404: The documentation suggestion logic in the function
using suggestAll/categories and the block check (block.type === 'code' &&
(!prevBlock || !prevBlock.type.includes('text-cell'))) treats only 'text-cell'
as valid preceding documentation, causing false positives for markdown; update
the condition that checks prevBlock.type (referenced where block.type,
prevBlock, and suggestions.push are used) to accept markdown/documentation block
types (e.g., 'markdown', 'text-cell', or any other project-specific doc types)
by testing for those values instead of only includes('text-cell'), so a
preceding markdown block prevents pushing the "Code block has no preceding
documentation" suggestion.
In `@packages/mcp/src/tools/reading.ts`:
- Line 6: The CSpell error arises because the token "modelcontextprotocol" used
in the import path (e.g., '@modelcontextprotocol/sdk/types.js') isn't in the
spell-check dictionary; fix it by adding "modelcontextprotocol" to your CSpell
configuration's custom words list (or equivalent dictionary entry) so the
spell-checker accepts imports like '@modelcontextprotocol/sdk/types.js' without
failing; update the cspell config (cspell.json / cspell.yaml or package.json
cspell section) to include "modelcontextprotocol" in the "words" array.
- Around line 183-187: Extract the duplicated helper loadDeepnoteFile (which
resolves a path, reads file content, and calls deserializeDeepnoteFile) into a
shared utility module (e.g., export an async function loadDeepnoteFile in a new
utils/file.ts or utils/deepnote.ts). Replace the duplicate implementations in
reading.ts, execution.ts, and writing.ts by importing loadDeepnoteFile from the
new module and remove the local functions; ensure you also export or reuse
deserializeDeepnoteFile from the same shared module or import it where needed so
callers still call deserializeDeepnoteFile via the shared helper.
- Around line 604-675: The isPythonBuiltin function recreates the Set on every
call; hoist the Set to module scope by defining a constant (e.g.,
PYTHON_BUILTINS or PYTHON_BUILTINS_SET) at top-level and initialize it once with
the listed names, then update isPythonBuiltin to simply return
PYTHON_BUILTINS.has(name); ensure the constant name is exported only if needed
and preserved exactly where the current array contents are defined.
In `@packages/mcp/src/tools/writing.ts`:
- Around line 928-937: The code constructs a RegExp from user-controlled
transform.replaceContent.find and passes it to new RegExp, which risks ReDoS; to
fix, treat find as a literal unless explicitly trusted: add an escapeRegExp
utility and use new RegExp(escapeRegExp(transform.replaceContent.find), 'g') (or
otherwise validate/sanitize the pattern when regex semantics are required), and
wrap the RegExp construction/replace call in a try/catch to safely handle
invalid or problematic patterns; update the block.content.replace usage to use
the escaped/validated pattern (and reference transform.replaceContent.find, new
RegExp, and block.content.replace to locate the change).
|
|
||
| - **Server instructions**: Comprehensive usage guidance injected into AI's system prompt | ||
| - **MCP Prompts**: Pre-built workflow templates for common tasks | ||
| - **Bounded toolsets**: Focused tools with specific contracts |
Contributor
There was a problem hiding this comment.
Fix CSpell error: add "toolsets" to dictionary.
Pipeline fails on unknown word.
Suggested fix
Add toolsets to cspell configuration or rephrase to "Focused tool sets" (two words).
🧰 Tools
🪛 GitHub Actions: CI
[error] 9-9: CSpell: Unknown word 'toolsets'.
🤖 Prompt for AI Agents
In `@packages/mcp/README.md` at line 9, The CSpell failure is caused by the word
"toolsets" in the README line "**Bounded toolsets**": either add "toolsets" to
the cspell dictionary (e.g., add "toolsets" to your cspell.json word list) or
edit the README to rephrase the phrase to "Bounded tool sets" (two words) so the
spell checker no longer errors; update the README.md line accordingly or update
the project's cspell config to include the new word.
Comment on lines
+124
to
+128
| ``` | ||
| Use deepnote_scaffold with: | ||
| - description: "Data analysis notebook that loads CSV, explores with visualizations, trains a model" | ||
| - outputPath: "analysis.deepnote" | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language identifiers to example code blocks.
Markdownlint (MD040) flags missing language specifiers. Use text or plaintext for these examples.
Example fix
-```
+```text
Use deepnote_scaffold with:Same applies to lines 132, 139, 147, 155, and 164.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
124-124: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@packages/mcp/README.md` around lines 124 - 128, Multiple fenced code blocks
in the README (the examples starting with "Use deepnote_scaffold with:" and the
similar blocks later) lack a language identifier, triggering MD040; update each
fenced block by adding a language tag such as text or plaintext (e.g., change
``` to ```text) for the examples at the occurrences containing the phrases "Use
deepnote_scaffold with:" and the other similar example blocks so Markdownlint no
longer flags them.
| ``` | ||
| Use deepnote_test with: | ||
| - path: "notebook.deepnote" | ||
| - testFramework: "pytest" (or unittest, assert) |
Contributor
There was a problem hiding this comment.
Fix CSpell error: add "pytest" to dictionary.
Pipeline fails. Common term; add to cspell config.
🧰 Tools
🪛 GitHub Actions: CI
[error] 167-167: CSpell: Unknown word 'pytest'.
🤖 Prompt for AI Agents
In `@packages/mcp/README.md` at line 167, Add the term "pytest" to the project's
cspell dictionary so the spellchecker no longer flags it: update the cspell
configuration (e.g., cspell.json or .cspellrc) to include "pytest" in the
"words" array or custom dictionary, then commit the change so CI recognizes the
term and the pipeline stops failing on this common testing term.
| import * as fs from 'node:fs/promises' | ||
| import * as path from 'node:path' | ||
| import { deserializeDeepnoteFile } from '@deepnote/blocks' | ||
| import type { Resource } from '@modelcontextprotocol/sdk/types.js' |
Contributor
There was a problem hiding this comment.
Add 'modelcontextprotocol' to CSpell dictionary.
Same pipeline failure as server.ts.
🧰 Tools
🪛 GitHub Actions: CI
[error] 4-4: CSpell: Unknown word 'modelcontextprotocol'.
🤖 Prompt for AI Agents
In `@packages/mcp/src/resources.ts` at line 4, The CI spellcheck flags the import
specifier '@modelcontextprotocol' in packages/mcp/src/resources.ts; update your
repository CSpell dictionary (e.g., .cspell.json or cspell.words referenced by
your config) to include the token "modelcontextprotocol" (or the full module
name) so the import string is accepted; locate the CSpell config file used by
the pipeline and add "modelcontextprotocol" to the "words" array (matching the
change you made for server.ts) and commit that change.
Comment on lines
+13
to
+19
| interface BlobResourceContents { | ||
| uri: string | ||
| mimeType?: string | ||
| blob: string // base64 encoded | ||
| } | ||
|
|
||
| type ResourceContents = TextResourceContents | BlobResourceContents |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
BlobResourceContents is unused.
Defined but never referenced. Remove or add a TODO if planned for future use.
🤖 Prompt for AI Agents
In `@packages/mcp/src/resources.ts` around lines 13 - 19, The type
BlobResourceContents is defined but never used; either remove the
BlobResourceContents declaration and update the union type alias
ResourceContents to only reference TextResourceContents, or if it's intended for
future use, add a clear TODO comment above BlobResourceContents indicating
planned usage. Also scan for and update any references to ResourceContents or
related types (e.g., TextResourceContents) to ensure the union remains correct
after removing or preserving BlobResourceContents.
| import type { DeepnoteFile } from '@deepnote/blocks' | ||
| import { deserializeDeepnoteFile } from '@deepnote/blocks' | ||
| import { getBlockDependencies } from '@deepnote/reactivity' | ||
| import type { Tool } from '@modelcontextprotocol/sdk/types.js' |
Contributor
There was a problem hiding this comment.
Fix CSpell error: add "modelcontextprotocol" to dictionary.
Pipeline fails. Add to cspell config.
🧰 Tools
🪛 GitHub Actions: CI
[error] 6-6: CSpell: Unknown word 'modelcontextprotocol'.
🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/reading.ts` at line 6, The CSpell error arises because
the token "modelcontextprotocol" used in the import path (e.g.,
'@modelcontextprotocol/sdk/types.js') isn't in the spell-check dictionary; fix
it by adding "modelcontextprotocol" to your CSpell configuration's custom words
list (or equivalent dictionary entry) so the spell-checker accepts imports like
'@modelcontextprotocol/sdk/types.js' without failing; update the cspell config
(cspell.json / cspell.yaml or package.json cspell section) to include
"modelcontextprotocol" in the "words" array.
Comment on lines
+183
to
+187
| async function loadDeepnoteFile(filePath: string): Promise<DeepnoteFile> { | ||
| const absolutePath = path.resolve(filePath) | ||
| const content = await fs.readFile(absol 4B92 utePath, 'utf-8') | ||
| return deserializeDeepnoteFile(content) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting loadDeepnoteFile to a shared utility.
This helper is duplicated in reading.ts, execution.ts, and writing.ts. Extract to a common module.
🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/reading.ts` around lines 183 - 187, Extract the
duplicated helper loadDeepnoteFile (which resolves a path, reads file content,
and calls deserializeDeepnoteFile) into a shared utility module (e.g., export an
async function loadDeepnoteFile in a new utils/file.ts or utils/deepnote.ts).
Replace the duplicate implementations in reading.ts, execution.ts, and
writing.ts by importing loadDeepnoteFile from the new module and remove the
local functions; ensure you also export or reuse deserializeDeepnoteFile from
the same shared module or import it where needed so callers still call
deserializeDeepnoteFile via the shared helper.
Comment on lines
+604
to
+675
| // Common Python builtins to exclude from undefined variable checks | ||
| function isPythonBuiltin(name: string): boolean { | ||
| const builtins = new Set([ | ||
| 'print', | ||
| 'len', | ||
| 'range', | ||
| 'str', | ||
| 'int', | ||
| 'float', | ||
| 'list', | ||
| 'dict', | ||
| 'set', | ||
| 'tuple', | ||
| 'bool', | ||
| 'None', | ||
| 'True', | ||
| 'False', | ||
| 'type', | ||
| 'isinstance', | ||
| 'hasattr', | ||
| 'getattr', | ||
| 'setattr', | ||
| 'open', | ||
| 'input', | ||
| 'sum', | ||
| 'min', | ||
| 'max', | ||
| 'abs', | ||
| 'round', | ||
| 'sorted', | ||
| 'reversed', | ||
| 'enumerate', | ||
| 'zip', | ||
| 'map', | ||
| 'filter', | ||
| 'any', | ||
| 'all', | ||
| 'ord', | ||
| 'chr', | ||
| 'hex', | ||
| 'bin', | ||
| 'oct', | ||
| 'format', | ||
| 'repr', | ||
| 'id', | ||
| 'dir', | ||
| 'vars', | ||
| 'globals', | ||
| 'locals', | ||
| 'exec', | ||
| 'eval', | ||
| 'compile', | ||
| '__name__', | ||
| '__file__', | ||
| '__doc__', | ||
| 'Exception', | ||
| 'ValueError', | ||
| 'TypeError', | ||
| 'KeyError', | ||
| 'IndexError', | ||
| 'AttributeError', | ||
| 'ImportError', | ||
| 'RuntimeError', | ||
| 'StopIteration', | ||
| 'object', | ||
| 'super', | ||
| 'classmethod', | ||
| 'staticmethod', | ||
| 'property', | ||
| ]) | ||
| return builtins.has(name) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hoist builtins Set to module scope.
Set is recreated on every isPythonBuiltin call. Define once at module level.
Suggested fix
+const PYTHON_BUILTINS = new Set([
+ 'print', 'len', 'range', /* ... rest of builtins */
+])
+
function isPythonBuiltin(name: string): boolean {
- const builtins = new Set([
- 'print',
- // ...
- ])
- return builtins.has(name)
+ return PYTHON_BUILTINS.has(name)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Common Python builtins to exclude from undefined variable checks | |
| function isPythonBuiltin(name: string): boolean { | |
| const builtins = new Set([ | |
| 'print', | |
| 'len', | |
| 'range', | |
| 'str', | |
| 'int', | |
| 'float', | |
| 'list', | |
| 'dict', | |
| 'set', | |
| 'tuple', | |
| 'bool', | |
| 'None', | |
| 'True', | |
| 'False', | |
| 'type', | |
| 'isinstance', | |
| 'hasattr', | |
| 'getattr', | |
| 'setattr', | |
| 'open', | |
| 'input', | |
| 'sum', | |
| 'min', | |
| 'max', | |
| 'abs', | |
| 'round', | |
| 'sorted', | |
| 'reversed', | |
| 'enumerate', | |
| 'zip', | |
| 'map', | |
| 'filter', | |
| 'any', | |
| 'all', | |
| 'ord', | |
| 'chr', | |
| 'hex', | |
| 'bin', | |
| 'oct', | |
| 'format', | |
| 'repr', | |
| 'id', | |
| 'dir', | |
| 'vars', | |
| 'globals', | |
| 'locals', | |
| 'exec', | |
| 'eval', | |
| 'compile', | |
| '__name__', | |
| '__file__', | |
| '__doc__', | |
| 'Exception', | |
| 'ValueError', | |
| 'TypeError', | |
| 'KeyError', | |
| 'IndexError', | |
| 'AttributeError', | |
| 'ImportError', | |
| 'RuntimeError', | |
| 'StopIteration', | |
| 'object', | |
| 'super', | |
| 'classmethod', | |
| 'staticmethod', | |
| 'property', | |
| ]) | |
| return builtins.has(name) | |
| } | |
| const PYTHON_BUILTINS = new Set([ | |
| 'print', | |
| 'len', | |
| 'range', | |
| 'str', | |
| 'int', | |
| 'float', | |
| 'list', | |
| 'dict', | |
| 'set', | |
| 'tuple', | |
| 'bool', | |
| 'None', | |
| 'True', | |
| 'False', | |
| 'type', | |
| 'isinstance', | |
| 'hasattr', | |
| 'getattr', | |
| 'setattr', | |
| 'open', | |
| 'input', | |
| 'sum', | |
| 'min', | |
| 'max', | |
| 'abs', | |
| 'round', | |
| 'sorted', | |
| 'reversed', | |
| 'enumerate', | |
| 'zip', | |
| 'map', | |
| 'filter', | |
| 'any', | |
| 'all', | |
| 'ord', | |
| 'chr', | |
| 'hex', | |
| 'bin', | |
| 'oct', | |
| 'format', | |
| 'repr', | |
| 'id', | |
| 'dir', | |
| 'vars', | |
| 'globals', | |
| 'locals', | |
| 'exec', | |
| 'eval', | |
| 'compile', | |
| '__name__', | |
| '__file__', | |
| '__doc__', | |
| 'Exception', | |
| 'ValueError', | |
| 'TypeError', | |
| 'KeyError', | |
| 'IndexError', | |
| 'AttributeError', | |
| 'ImportError', | |
| 'RuntimeError', | |
| 'StopIteration', | |
| 'object', | |
| 'super', | |
| 'classmethod', | |
| 'staticmethod', | |
| 'property', | |
| ]) | |
| function isPythonBuiltin(name: string): boolean { | |
| return PYTHON_BUILTINS.has(name) | |
| } |
🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/reading.ts` around lines 604 - 675, The
isPythonBuiltin function recreates the Set on every call; hoist the Set to
module scope by defining a constant (e.g., PYTHON_BUILTINS or
PYTHON_BUILTINS_SET) at top-level and initialize it once with the listed names,
then update isPythonBuiltin to simply return PYTHON_BUILTINS.has(name); ensure
the constant name is exported only if needed and preserved exactly where the
current array contents are defined.
Comment on lines
+928
to
+937
| if (transform.replaceContent && block.content) { | ||
| const newContent = block.content.replace( | ||
| new RegExp(transform.replaceContent.find, 'g'), | ||
| transform.replaceContent.replace | ||
| ) | ||
| if (newContent !== block.content) { | ||
| block.content = newContent | ||
| changes.push(`replaced "${transform.replaceContent.find}"`) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
ReDoS vulnerability: user-controlled regex pattern.
transform.replaceContent.find is passed directly to new RegExp(). Malicious patterns can cause exponential backtracking.
Mitigation options
+import { escapeRegExp } from './utils' // or inline escape
+
if (transform.replaceContent && block.content) {
- const newContent = block.content.replace(
- new RegExp(transform.replaceContent.find, 'g'),
- transform.replaceContent.replace
- )
+ // Option 1: Use string.replaceAll (no regex)
+ const newContent = block.content.replaceAll(
+ transform.replaceContent.find,
+ transform.replaceContent.replace
+ )
+ // Option 2: If regex needed, validate/escape input🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 929-929: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(transform.replaceContent.find, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/writing.ts` around lines 928 - 937, The code
constructs a RegExp from user-controlled transform.replaceContent.find and
passes it to new RegExp, which risks ReDoS; to fix, treat find as a literal
unless explicitly trusted: add an escapeRegExp utility and use new
RegExp(escapeRegExp(transform.replaceContent.find), 'g') (or otherwise
validate/sanitize the pattern when regex semantics are required), and wrap the
RegExp construction/replace call in a try/catch to safely handle invalid or
problematic patterns; update the block.content.replace usage to use the
escaped/validated pattern (and reference transform.replaceContent.find, new
RegExp, and block.content.replace to locate the change).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@examples/mcp-demos/ab_test_evaluator.deepnote`:
- Around line 21-23: The notebook markdown cell content string describing the
tool is grammatically awkward; rewrite the sentence in that cell (the value of
the content field in the markdown cell) to be clear and idiomatic—e.g., "This
notebook is an A/B test statistical analysis tool that accepts experiment data
with control and treatment groups, calculates required sample sizes for desired
power, performs t-tests and chi-square tests, computes confidence intervals,
visualizes conversion rates with error bars, and gives recommendations based on
statistical significance." Update the content field accordingly to replace the
original sentence.
In `@examples/mcp-demos/api_analytics_client.deepnote`:
- Around line 89-101: The get method currently can silently return None if
retries is 0; add an input validation at the start of Client.get (the get(self,
endpoint, params=None, retries=3) method) to ensure retries is a positive
integer (e.g., raise ValueError if retries < 1) so callers don’t receive None
unexpectedly, or alternatively normalize retries with something like retries =
max(1, int(retries)) before entering the retry loop; keep the existing
retry/backoff logic and ensure the function never reaches the final `return
None` path.
In `@examples/mcp-demos/churn_prediction.deepnote`:
- Around line 76-77: The import "StandardScaler" is unused; either remove it
from the import line or apply it to preprocess features. In the file's imports
(the line importing from sklearn.preprocessing), delete "StandardScaler" if you
don't standardize inputs, or instantiate and use StandardScaler (fit/transform
on your feature matrix, e.g., in the data preparation pipeline before
train_test_split) referencing StandardScaler to avoid the unused-import warning.
- Around line 21-22: Update the markdown cell whose content starts "This
notebook Customer churn prediction model..." to a complete sentence by adding
the missing verb; for example change it to "This notebook implements a customer
churn prediction model that loads customer data, performs feature engineering on
usage metrics, subscription history, and engagement data, trains a
classification model with hyperparameter tuning, evaluates model performance
with confusion matrix and ROC curve, and outputs churn probability scores."
Locate the markdown entry by the keys type: markdown and the content string to
apply the edit.
- Around line 125-142: The notebook cell uses plt.figure(), plt.xlabel(),
plt.ylabel(), plt.title(), and plt.show() but never imports matplotlib.pyplot;
add the missing import statement (import matplotlib.pyplot as plt) at the top of
this cell (or the top of the notebook) so calls to plt in the plotting block
will work; locate the plotting block that computes cm = confusion_matrix(...)
and calls plt.figure() / plt.show() to insert the import.
In `@examples/mcp-demos/README.md`:
- Around line 20-56: The README shows deepnote_template, deepnote_scaffold
C02E
,
deepnote_enhance, and deepnote_workflow as shell commands but they are MCP tool
names implemented in packages/mcp/src/tools/magic.ts and must be invoked through
the MCP server (deepnote-mcp) or an MCP client, not run directly. Update the
examples to (1) state these are MCP tool identifiers, (2) show the correct
invocation pattern via the MCP server/client (e.g., calling the tool name
through deepnote-mcp or an MCP client API with the tool name and args payload),
and (3) replace the bash command blocks with illustrative MCP client calls or a
JSON RPC payload example referencing the tool names deepnote_template,
deepnote_scaffold, deepnote_enhance, and deepnote_workflow so users know how to
call them correctly.
In `@packages/mcp/src/tools/magic.ts`:
- Around line 1836-1838: The code uses the deprecated pandas API
fillna(method='ffill').fillna(method='bfill') on df_transformed; replace that
call with the modern chained methods df_transformed.ffill().bfill() (preserving
the existing assignment to df_transformed and the missing_before
calculation/print) to avoid the deprecation warning—update the expression that
assigns df_transformed in the block containing missing_before and the print.
- Around line 478-492: Both loadDeepnoteFile and saveDeepnoteFile lack error
handling for file I/O and YAML serialization/deserialization; wrap the body of
each function (the path.resolve, fs.readFile/fs.writeFile and
deserializeDeepnoteFile/yamlStringify calls) in try/catch blocks, and on error
throw a new Error (or a custom Error subclass) that includes the operation, the
filePath, and the original error message/stack to preserve context (e.g.,
"loadDeepnoteFile: failed to read/parse <filePath>: <originalError>"). Ensure
you rethrow or include the original error as a cause where supported so callers
get both the high-level context and the underlying error details.
- Around line 496-501: The function handleScaffold uses unsafe casts
(description and outputPath) without runtime checks; add explicit runtime
validation at the start of handleScaffold to ensure description and outputPath
are non-empty strings (and optionally validate projectName and style types), and
throw a clear Error if validation fails so callers that bypass schema validation
fail fast; update references to description/outputPath/projectName/style within
handleScaffold to rely on the validated local variables.
- Around line 478-481: The loadDeepnoteFile function currently resolves
arbitrary filePath with path.resolve(filePath) which allows path traversal;
update loadDeepnoteFile (and any saveDeepnoteFile siblings) to validate the
resolved absolutePath is contained within the intended workspace root before
reading/writing: compute const absolutePath = path.resolve(filePath), compute
the workspaceRootResolved = path.resolve(workspaceRoot) (or use path.relative
like resources.ts) and if the absolutePath is outside workspaceRootResolved
(e.g. !absolutePath.startsWith(workspaceRootResolved) or
path.relative(workspaceRootResolved, absolutePath).startsWith('..')), throw a
clear error like 'Path traversal not allowed' and only proceed to
fs.readFile()/fs.writeFile() when the check passes.
Comment on lines
+21
to
+23
| type: markdown | ||
| content: This notebook A/B test statistical analysis tool that takes experiment data with control and treatment groups, calculates sample sizes for statistical power, performs t-tests and chi-square tests, computes confidence intervals, visualizes conversion rates with error bars, and provides recommendations based on statistical significance. | ||
| metadata: {} |
Contributor
There was a problem hiding this comment.
Fix grammar in the notebook description.
The sentence reads awkwardly; a small rewrite improves clarity.
✏️ Proposed fix
- content: This notebook A/B test statistical analysis tool that takes experiment data with control and treatment groups, calculates sample sizes for statistical power, performs t-tests and chi-square tests, computes confidence intervals, visualizes conversion rates with error bars, and provides recommendations based on statistical significance.
+ content: This notebook is an A/B test statistical analysis tool that takes experiment data with control and treatment groups, calculates sample sizes for statistical power, performs t-tests and chi-square tests, computes confidence intervals, visualizes conversion rates with error bars, and provides recommendations based on statistical significance.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type: markdown | |
| content: This notebook A/B test statistical analysis tool that takes experiment data with control and treatment groups, calculates sample sizes for statistical power, performs t-tests and chi-square tests, computes confidence intervals, visualizes conversion rates with error bars, and provides recommendations based on statistical significance. | |
| metadata: {} | |
| type: markdown | |
| content: This notebook is an A/B test statistical analysis tool that takes experiment data with control and treatment groups, calculates sample sizes for statistical power, performs t-tests and chi-square tests, computes confidence intervals, visualizes conversion rates with error bars, and provides recommendations based on statistical significance. | |
| metadata: {} |
🤖 Prompt for AI Agents
In `@examples/mcp-demos/ab_test_evaluator.deepnote` around lines 21 - 23, The
notebook markdown cell content string describing the tool is grammatically
awkward; rewrite the sentence in that cell (the value of the content field in
the markdown cell) to be clear and idiomatic—e.g., "This notebook is an A/B test
statistical analysis tool that accepts experiment data with control and
treatment groups, calculates required sample sizes for desired power, performs
t-tests and chi-square tests, computes confidence intervals, visualizes
conversion rates with error bars, and gives recommendations based on statistical
significance." Update the content field accordingly to replace the original
sentence.
Comment on lines
+89
to
+101
| def get(self, endpoint, params=None, retries=3): | ||
| """GET request with retry logic.""" | ||
| url = f"{self.base_url}/{endpoint.lstrip('/')}" | ||
| for attempt in range(retries): | ||
| try: | ||
| response = self.session.get(url, params=params, timeout=30) | ||
| response.raise_for_status() | ||
| return response.json() | ||
| except requests.RequestException as e: | ||
| if attempt == retries - 1: | ||
| raise | ||
| time.sleep(2 ** attempt) # Exponential backoff | ||
| return None |
Contributor
There was a problem hiding this comment.
Guard against retries=0 returning None silently.
Add a quick validation so callers don’t get an unexpected None.
🐛 Proposed fix
def get(self, endpoint, params=None, retries=3):
"""GET request with retry logic."""
+ if retries < 1:
+ raise ValueError("retries must be >= 1")
url = f"{self.base_url}/{endpoint.lstrip('/')}"
for attempt in range(retries):
try:
response = self.session.get(url, params=params, timeout=30)
response.raise_for_status()
return response.json()
except requests.RequestException as e:
if attempt == retries - 1:
raise
time.sleep(2 ** attempt) # Exponential backoff
- return None🤖 Prompt for AI Agents
In `@examples/mcp-demos/api_analytics_client.deepnote` around lines 89 - 101, The
get method currently can silently return None if retries is 0; add an input
validation at the start of Client.get (the get(self, endpoint, params=None,
retries=3) method) to ensure retries is a positive integer (e.g., raise
ValueError if retries < 1) so callers don’t receive None unexpectedly, or
alternatively normalize retries with something like retries = max(1,
int(retries)) before entering the retry loop; keep the existing retry/backoff
logic and ensure the function never reaches the final `return None` path.
Comment on lines
+21
to
+22
| type: markdown | ||
| content: This notebook Customer churn prediction model that loads customer data, performs feature engineering on usage metrics, subscription history, and engagement data, trains a classification model with hyperparameter tuning, evaluates model performance with confusion matrix and ROC curve, and outputs churn probability scores. |
Contributor
There was a problem hiding this comment.
Incomplete sentence in description.
Missing verb: "This notebook Customer churn prediction model..." → "This notebook implements a customer churn prediction model..."
🤖 Prompt for AI Agents
In `@examples/mcp-demos/churn_prediction.deepnote` around lines 21 - 22, Update
the markdown cell whose content starts "This notebook Customer churn prediction
model..." to a complete sentence by adding the missing verb; for example change
it to "This notebook implements a customer churn prediction model that loads
customer data, performs feature engineering on usage metrics, subscription
history, and engagement data, trains a classification model with hyperparameter
tuning, evaluates model performance with confusion matrix and ROC curve, and
outputs churn probability scores." Locate the markdown entry by the keys type:
markdown and the content string to apply the edit.
Comment on lines
+76
to
+77
| < 3486 /td> | from sklearn.model_selection import train_test_split | |
| from sklearn.preprocessing import StandardScaler |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused import.
StandardScaler imported but never used.
🤖 Prompt for AI Agents
In `@examples/mcp-demos/churn_prediction.deepnote` around lines 76 - 77, The
import "StandardScaler" is unused; either remove it from the import line or
apply it to preprocess features. In the file's imports (the line importing from
sklearn.preprocessing), delete "StandardScaler" if you don't standardize inputs,
or instantiate and use StandardScaler (fit/transform on your feature matrix,
e.g., in the data preparation pipeline before train_test_split) referencing
StandardScaler to avoid the unused-import warning.
Comment on lines
+125
to
+142
| content: |- | ||
| from sklearn.metrics import classification_report, confusion_matrix | ||
| import seaborn as sns | ||
|
|
||
| # Predictions | ||
| y_pred = model.predict(X_test) | ||
|
|
||
| # Classification report | ||
| print(classification_report(y_test, y_pred)) | ||
|
|
||
| # Confusion matrix | ||
| plt.figure(figsize=(8, 6)) | ||
| cm = confusion_matrix(y_test, y_pred) | ||
| sns.heatmap(cm, annot=True, fmt='d', cmap='Blues') | ||
| plt.xlabel('Predicted') | ||
| plt.ylabel('Actual') | ||
| plt.title('Confusion Matrix') | ||
| plt.show() |
Contributor
There was a problem hiding this comment.
Missing matplotlib.pyplot import.
Code uses plt.figure(), plt.xlabel(), etc., but matplotlib.pyplot isn't imported. Add import matplotlib.pyplot as plt.
Proposed fix
from sklearn.metrics import classification_report, confusion_matrix
import seaborn as sns
+import matplotlib.pyplot as plt📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| content: |- | |
| from sklearn.metrics import classification_report, confusion_matrix | |
| import seaborn as sns | |
| # Predictions | |
| y_pred = model.predict(X_test) | |
| # Classification report | |
| print(classification_report(y_test, y_pred)) | |
| # Confusion matrix | |
| plt.figure(figsize=(8, 6)) | |
| cm = confusion_matrix(y_test, y_pred) | |
| sns.heatmap(cm, annot=True, fmt='d', cmap='Blues') | |
| plt.xlabel('Predicted') | |
| plt.ylabel('Actual') | |
| plt.title('Confusion Matrix') | |
| plt.show() | |
| content: |- | |
| from sklearn.metrics import classification_report, confusion_matrix | |
| import seaborn as sns | |
| import matplotlib.pyplot as plt | |
| # Predictions | |
| y_pred = model.predict(X_test) | |
| # Classification report | |
| print(classification_report(y_test, y_pred)) | |
| # Confusion matrix | |
| plt.figure(figsize=(8, 6)) | |
| cm = confusion_matrix(y_test, y_pred) | |
| sns.heatmap(cm, annot=True, fmt='d', cmap='Blues') | |
| plt.xlabel('Predicted') | |
| plt.ylabel('Actual') | |
| plt.title('Confusion Matrix') | |
| plt.show() |
🤖 Prompt for AI Agents
In `@examples/mcp-demos/churn_prediction.deepnote` around lines 125 - 142, The
notebook cell uses plt.figure(), plt.xlabel(), plt.ylabel(), plt.title(), and
plt.show() but never imports matplotlib.pyplot; add the missing import statement
(import matplotlib.pyplot as plt) at the top of this cell (or the top of the
notebook) so calls to plt in the plotting block will work; locate the plotting
block that computes cm = confusion_matrix(...) and calls plt.figure() /
plt.show() to insert the import.
Comment on lines
+20
to
+56
| ### Using Templates | ||
|
|
||
| ```bash | ||
| # Create a dashboard from template | ||
| deepnote_template --template=dashboard --outputPath=my_dashboard.deepnote | ||
|
|
||
| # Available templates: dashboard, ml_pipeline, etl, report, api_client | ||
| ``` | ||
|
|
||
| ### Using Natural Language Scaffolding | ||
|
|
||
| ```bash | ||
| # Describe what you want and get a complete notebook | ||
| deepnote_scaffold \ | ||
| --description="Customer churn prediction with feature engineering and model evaluation" \ | ||
| --outputPath=churn_model.deepnote \ | ||
| --style=tutorial | ||
| ``` | ||
|
|
||
| ### Enhancing Notebooks | ||
|
|
||
| ```bash | ||
| # Add interactivity, documentation, and visualizations | ||
| deepnote_enhance --path=my_notebook.deepnote --enhancements=all | ||
| ``` | ||
|
|
||
| ### Using Workflows | ||
|
|
||
| Chain multiple operations together: | ||
|
|
||
| ```bash | ||
| deepnote_workflow --steps='[ | ||
| {"tool": "template", "args": {"template": "ml_pipeline"}}, | ||
| {"tool": "enhance", "args": {"enhancements": ["inputs", "documentation"]}}, | ||
| {"tool": "suggest", "args": {}} | ||
| ]' | ||
| ``` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for deepnote command references in codebase
rg -n "deepnote_(template|scaffold|enhance|workflow)" --type markdown --type typescript --type json
# Check for bin entries in package.json files
find . -name "package.json" -path "*/mcp/*" | head -5 | xargs -I {} bash -c "echo '=== {} ===' && cat {}"
# Look for CLI entry point definitions
rg -n "bin|command" --type json -A 5 -B 5 | head -100Repository: deepnote/deepnote
Length of output: 5464
🏁 Script executed:
#!/bin/bash
# More targeted search for CLI definitions
find . -name "bin.ts" -o -name "cli.ts" -o -name "index.ts" | grep -E "mcp|deepnote" | head -10
# Check actual packages directory structure
ls -la packages/ 2>/dev/null || echo "No packages dir"
ls -la . | grep -E "^d" | head -20Repository: deepnote/deepnote
Length of output: 1409
🏁 Script executed:
#!/bin/bash
# Check CLI bin.ts to see what commands are actually available
cat packages/cli/src/bin.ts | head -100
# Also check for MCP tools definitions
cat packages/mcp/src/bin.ts | head -100Repository: deepnote/deepnote
Length of output: 260
🏁 Script executed:
#!/bin/bash
# Search for where template, scaffold, enhance, workflow are actually defined
rg -n "template|scaffold|enhance|workflow" packages/cli/src/ packages/mcp/src/ -A 2 -B 2 | head -150Repository: deepnote/deepnote
Length of output: 9668
The command names are MCP tools, not CLI entry points—examples are misleading.
These are Model Context Protocol tool names defined in packages/mcp/src/tools/magic.ts, not CLI commands. The README shows them as shell invocations (e.g., deepnote_template --template=dashboard), which won't work if users try to run them directly. They're called through the MCP server (deepnote-mcp), not as standalone commands. Rewrite the examples to clarify they're MCP tools and show proper invocation through an MCP client, not shell commands.
🤖 Prompt for AI Agents
In `@examples/mcp-demos/README.md` around lines 20 - 56, The README shows
deepnote_template, deepnote_scaffold, deepnote_enhance, and deepnote_workflow as
shell commands but they are MCP tool names implemented in
packages/mcp/src/tools/magic.ts and must be invoked through the MCP server
(deepnote-mcp) or an MCP client, not run directly. Update the examples to (1)
state these are MCP tool identifiers, (2) show the correct invocation pattern
via the MCP server/client (e.g., calling the tool name through deepnote-mcp or
an MCP client API with the tool name and args payload), and (3) replace the bash
command blocks with illustrative MCP client calls or a JSON RPC payload example
referencing the tool names deepnote_template, deepnote_scaffold,
deepnote_enhance, and deepnote_workflow so users know how to call them
correctly.
Comment on lines
+478
to
+492
| async function loadDeepnoteFile(filePath: string): Promise<DeepnoteFile> { | ||
| const absolutePath = path.resolve(filePath) | ||
| const content = await fs.readFile(absolutePath, 'utf-8') | ||
| return deserializeDeepnoteFile(content) | ||
| } | ||
|
|
||
| async function saveDeepnoteFile(filePath: string, file: DeepnoteFile): Promise<void> { | ||
| const absolutePath = path.resolve(filePath) | ||
| const content = yamlStringify(file, { | ||
| lineWidth: 0, | ||
| defaultStringType: 'PLAIN', | ||
| defaultKeyType: 'PLAIN', | ||
| }) | ||
| await fs.writeFile(absolutePath, content, 'utf-8') | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing error handling for file I/O.
loadDeepnoteFile and saveDeepnoteFile don't handle common errors (file not found, invalid YAML, permission denied). Callers receive raw exceptions without context.
♻️ Proposed fix
async function loadDeepnoteFile(filePath: string): Promise<DeepnoteFile> {
const absolutePath = path.resolve(filePath)
- const content = await fs.readFile(absolutePath, 'utf-8')
- return deserializeDeepnoteFile(content)
+ try {
+ const content = await fs.readFile(absolutePath, 'utf-8')
+ return deserializeDeepnoteFile(content)
+ } catch (error) {
+ const message = error instanceof Error ? error.message : String(error)
+ throw new Error(`Failed to load Deepnote file "${filePath}": ${message}`)
+ }
}
async function saveDeepnoteFile(filePath: string, file: DeepnoteFile): Promise<void> {
const absolutePath = path.resolve(filePath)
- const content = yamlStringify(file, {
- lineWidth: 0,
- defaultStringType: 'PLAIN',
- defaultKeyType: 'PLAIN',
- })
- await fs.writeFile(absolutePath, content, 'utf-8')
+ try {
+ const content = yamlStringify(file, {
+ lineWidth: 0,
+ defaultStringType: 'PLAIN',
+ defaultKeyType: 'PLAIN',
+ })
+ await fs.writeFile(absolutePath, content, 'utf-8')
+ } catch (error) {
+ const message = error instanceof Error ? error.message : String(error)
+ throw new Error(`Failed to save Deepnote file "${filePath}": ${message}`)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function loadDeepnoteFile(filePath: string): Promise<DeepnoteFile> { | |
| const absolutePath = path.resolve(filePath) | |
| const content = await fs.readFile(absolutePath, 'utf-8') | |
| return deserializeDeepnoteFile(content) | |
| } | |
| async function saveDeepnoteFile(filePath: string, file: DeepnoteFile): Promise<void> { | |
| const absolutePath = path.resolve(filePath) | |
| const content = yamlStringify(file, { | |
| lineWidth: 0, | |
| defaultStringType: 'PLAIN', | |
| defaultKeyType: 'PLAIN', | |
| }) | |
| await fs.writeFile(absolutePath, content, 'utf-8') | |
| } | |
| async function loadDeepnoteFile(filePath: string): Promise<DeepnoteFile> { | |
| const absolutePath = path.resolve(filePath) | |
| try { | |
| const content = await fs.readFile(absolutePath, 'utf-8') | |
| return deserializeDeepnoteFile(content) | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : String(error) | |
| throw new Error(`Failed to load Deepnote file "${filePath}": ${message}`) | |
| } | |
| } | |
| async function saveDeepnoteFile(filePath: string, file: DeepnoteFile): Promise<void> { | |
| const absolutePath = path.resolve(filePath) | |
| try { | |
| const content = yamlStringify(file, { | |
| lineWidth: 0, | |
| defaultStringType: 'PLAIN', | |
| defaultKeyType: 'PLAIN', | |
| }) | |
| await fs.writeFile(absolutePath, content, 'utf-8') | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : String(error) | |
| throw new Error(`Failed to save Deepnote file "${filePath}": ${message}`) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/magic.ts` around lines 478 - 492, Both
loadDeepnoteFile and saveDeepnoteFile lack error handling for file I/O and YAML
serialization/deserialization; wrap the body of each function (the path.resolve,
fs.readFile/fs.writeFile and deserializeDeepnoteFile/yamlStringify calls) in
try/catch blocks, and on error throw a new Error (or a custom Error subclass)
that includes the operation, the filePath, and the original error message/stack
to preserve context (e.g., "loadDeepnoteFile: failed to read/parse <filePath>:
<originalError>"). Ensure you rethrow or include the original error as a cause
where supported so callers get both the high-level context and the underlying
error details.
Comment on lines
+478
to
+481
| async function loadDeepnoteFile(filePath: string): Promise<DeepnoteFile> { | ||
| const absolutePath = path.resolve(filePath) | ||
| const content = await fs.readFile(absolutePath, 'utf-8') | ||
| return deserializeDeepnoteFile(content) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find loadDeepnoteFile function and context
rg -n "loadDeepnoteFile" --type=ts -B5 -A10Repository: deepnote/deepnote
Length of output: 32729
🏁 Script executed:
#!/bin/bash
# Check for path validation patterns in codebase
rg -n "normalize|resolve|join" --type=ts | head -30Repository: deepnote/deepnote
Length of output: 3152
🏁 Script executed:
#!/bin/bash
# Search for workspace root or base directory concepts
rg -n "workspace|basePath|rootPath" --type=ts | head -20Repository: deepnote/deepnote
Length of output: 1851
🏁 Script executed:
#!/bin/bash
# Look at tool definitions in magic.ts to see if filePath is user-facing
rg -n "tools:\s*\[|description:|required:" packages/mcp/src/tools/magic.ts | head -40Repository: deepnote/deepnote
Length of output: 2811
🏁 Script executed:
#!/bin/bash
# Check how filePath is validated or constrained in tool handlers
rg -n "args\.path|filePath" packages/mcp/src/tools/magic.ts -B2 -A2 | head -60Repository: deepnote/deepnote
Length of output: 1876
🏁 Script executed:
#!/bin/bash
# Check if there's any path validation wrapper in the server/dispatch layer
rg -n "dispatch|handleCall|process.*path" packages/mcp/src/server.ts -B2 -A2 | head -50Repository: deepnote/deepnote
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for where tools are dispatched/invoked in server.ts
rg -n "handleCall|tools\[|invoke" packages/mcp/src/server.ts -B3 -A3Repository: deepnote/deepnote
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if there's any path normalization or restriction in resources.ts
rg -n "normalize|realpath|sanitize" packages/mcp/src/ -t tsRepository: deepnote/deepnote
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look at server.ts to understand tool dispatch
cat packages/mcp/src/server.ts | head -100Repository: deepnote/deepnote
Length of output: 3357
🏁 Script executed:
#!/bin/bash
# Check how filePath restrictions could be implemented
rg -n "path.relative|startsWith|includes" packages/mcp/src/tools/ --type=ts | grep -E "(validation|check|allow)" | head -10Repository: deepnote/deepnote
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check handleMagicTool signature to see if it receives workspaceRoot
rg -n "handleMagicTool|export.*function handle" packages/mcp/src/tools/magic.ts | head -20Repository: deepnote/deepnote
Length of output: 162
🏁 Script executed:
#!/bin/bash
# Verify the exact function signature and parameters
sed -n '1,50p' packages/mcp/src/tools/magic.tsRepository: deepnote/deepnote
Length of output: 2067
Path traversal vulnerability in file access.
The path.resolve(filePath) on line 479 accepts arbitrary paths from user input without restriction. Callers can use paths like ../../etc/passwd to access files outside the intended workspace.
Validate that resolved paths stay within the workspace root before file access:
const absolutePath = path.resolve(filePath)
if (!absolutePath.startsWith(path.resolve(workspaceRoot))) {
throw new Error('Path traversal not allowed')
}Similar validation exists in resources.ts using path.relative(). Apply the same pattern t
D73A
o all loadDeepnoteFile and saveDeepnoteFile calls.
🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/magic.ts` around lines 478 - 481, The loadDeepnoteFile
function currently resolves arbitrary filePath with path.resolve(filePath) which
allows path traversal; update loadDeepnoteFile (and any saveDeepnoteFile
siblings) to validate the resolved absolutePath is contained within the intended
workspace root before reading/writing: compute const absolutePath =
path.resolve(filePath), compute the workspaceRootResolved =
path.resolve(workspaceRoot) (or use path.relative like resources.ts) and if the
absolutePath is outside workspaceRootResolved (e.g.
!absolutePath.startsWith(workspaceRootResolved) or
path.relative(workspaceRootResolved, absolutePath).startsWith('..')), throw a
clear error like 'Path traversal not allowed' and only proceed to
fs.readFile()/fs.writeFile() when the check passes.
Comment on lines
+496
to
+501
| async function handleScaffold(args: Record<string, unknown>) { | ||
| const description = args.description as string | ||
| const outputPath = args.outputPath as string | ||
| const projectName = args.projectName as string | undefined | ||
| const style = (args.style as string) || 'documented' | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unsafe type assertions on required args.
description and outputPath are required per schema but cast with as string without validation. If caller bypasses schema validation, this fails silently or produces undefined behavior.
♻️ Proposed fix
async function handleScaffold(args: Record<string, unknown>) {
- const description = args.description as string
- const outputPath = args.outputPath as string
+ const description = args.description
+ const outputPath = args.outputPath
+ if (typeof description !== 'string' || typeof outputPath !== 'string') {
+ return {
+ content: [{ type: 'text', text: 'Missing required arguments: description, outputPath' }],
+ isError: true,
+ }
+ }
const projectName = args.projectName as string | undefined
const style = (args.style as string) || 'documented'🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/magic.ts` around lines 496 - 501, The function
handleScaffold uses unsafe casts (description and outputPath) without runtime
checks; add explicit runtime validation at the start of handleScaffold to ensure
description and outputPath are non-empty strings (and optionally validate
projectName and style types), and throw a clear Error if validation fails so
callers that bypass schema validation fail fast; update references to
description/outputPath/projectName/style within handleScaffold to rely on the
validated local variables.
Comment on lines
+1836
to
+1838
| missing_before = df_transformed.isnull().sum().sum() | ||
| df_transformed = df_transformed.fillna(method='ffill').fillna(method='bfill') | ||
| print(f" - Handled {missing_before} missing values") |
Contributor
There was a problem hiding this comment.
Deprecated pandas API.
fillna(method='ffill') deprecated since pandas 2.1. Use ffill() and bfill() instead.
🐛 Proposed fix
- df_transformed = df_transformed.fillna(method='ffill').fillna(method='bfill')
+ df_transformed = df_transformed.ffill().bfill()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| missing_before = df_transformed.isnull().sum().sum() | |
| df_transformed = df_transformed.fillna(method='ffill').fillna(method='bfill') | |
| print(f" - Handled {missing_before} missing values") | |
| missing_before = df_transformed.isnull().sum().sum() | |
| df_transformed = df_transformed.ffill().bfill() | |
| print(f" - Handled {missing_before} missing values") |
🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/magic.ts` around lines 1836 - 1838, The code uses the
deprecated pandas API fillna(method='ffill').fillna(method='bfill') on
df_transformed; replace that call with the modern chained methods
df_transformed.ffill().bfill() (preserving the existing assignment to
df_transformed and the missing_before calculation/print) to avoid the
deprecation warning—update the expression that assigns df_transformed in the
block containing missing_before and the print.
Adds a simple CLI utility to test MCP tools directly without needing a full MCP client connection. Usage: pnpm --filter @deepnote/mcp test:tool <tool-name> '<json-args>'
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@docs-dictionary.txt`:
- Around line 296-297: Two truncated dictionary entries "explor" and "analys"
should be replaced with their full intended words; update the entries in
docs-dictionary.txt by replacing "explor" with the appropriate full form (e.g.,
"explore" or "exploration") and "analys" with the appropriate full form (e.g.,
"analyse" or "analysis") so the dictionary contains complete tokens and
consistent spelling variants.
In `@packages/mcp/package.json`:
- Line 52: The dependency entry for "zod" in package.json is pinned to an exact
version ("3.25.76") while other deps use caret ranges; update the "zod"
dependency to use a caret range (change "zod": "3.25.76" to "zod": "^3.25.76")
unless the exact pin was intentional—if you intend to move to v4, instead set it
to a compatible careted v4 version (e.g., "^4.x") and update any usages
accordingly.
In `@packages/mcp/src/test-tool.ts`:
- Around line 79-81: The code computes scriptDir with
path.dirname(process.argv[1] || ''), which yields '.' when process.argv[1] is
undefined and can produce an incorrect path for resolving bin.js; change the
fallback to a reliable file path (e.g., use process.argv[1] ?? __filename or
process.argv[1] || __filename) so that scriptDir = path.dirname(process.argv[1]
?? __filename) (or similar) inside the Promise block where scriptDir is
declared, ensuring you reference process.argv[1] and __filename (or
process.cwd() if you prefer) to avoid an empty-string dirname.
- Around line 53-56: Remove the redundant null/undefined check for
toolArgsString: delete the if block that checks if (!toolArgsString) and its
console.error/process.exit calls in test-tool.ts because args[1] is guaranteed
when the earlier args.length >= 2 check passes; rely on the existing args length
validation (variable args) and the toolArgsString usage instead of keeping this
extra guard.
- Around line 123-126: The code calls process.exit(1) inside the promise when
response.error is truthy, which leaves the promise unresolved; instead reject
the promise with the error so callers can handle it. Replace the process.exit(1)
path in the block that checks response.error (the branch referencing
response.error.message || response.error) to call the surrounding Promise's
reject(...) with response.error (or new Error(String(response.error))) and
return afterward (do not call resolve), ensuring the promise is settled instead
of forcing exit.
Comment on lines
+296
to
+297
| explor | ||
| analys |
Contributor
There was a problem hiding this comment.
Truncated words detected.
explor and analys appear incomplete. Should these be explore/exploration and analyse/analysis?
🤖 Prompt for AI Agents
In `@docs-dictionary.txt` around lines 296 - 297, Two truncated dictionary entries
"explor" and "analys" should be replaced with their full intended words; update
the entries in docs-dictionary.txt by replacing "explor" with the appropriate
full form (e.g., "explore" or "exploration") and "analys" with the appropriate
full form (e.g., "analyse" or "analysis") so the dictionary contains complete
tokens and consistent spelling variants.
| "@deepnote/runtime-core": "workspace:*", | ||
| "@modelcontextprotocol/sdk": "^1.12.1", | ||
| "yaml": "^2.8.0", | ||
| "zod": "3.25.76" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest zod npm package version 2025
💡 Result:
The latest stable zod version published in 2025 was 4.3.4, released on December 31, 2025. [1]
(For context, Zod 4.0.0 was released on npm on July 10, 2025.) [2]
Sources: [1] [2]
🏁 Script executed:
cat -n packages/mcp/package.json | head -65Repository: deepnote/deepnote
Length of output: 1966
zod pinned without caret—inconsistent with other dependencies.
All other dependencies use ^ (lines 50–51, 55), but zod uses exact "3.25.76". Verify if intentional; if not, align with "^3.25.76". Note: latest zod is 4.3.4 (v3.25.76 is outdated by 2+ major versions).
🤖 Prompt for AI Agents
In `@packages/mcp/package.json` at line 52, The dependency entry for "zod" in
package.json is pinned to an exact version ("3.25.76") while other deps use
caret ranges; update the "zod" dependency to use a caret range (change "zod":
"3.25.76" to "zod": "^3.25.76") unless the exact pin was intentional—if you
intend to move to v4, instead set it to a compatible careted v4 version (e.g.,
"^4.x") and update any usages accordingly.
Comment on lines
+53
to
+56
| if (!toolArgsString) { | ||
| console.error('Error: Missing JSON arguments') | ||
| process.exit(1) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant null check.
args[1] is guaranteed to exist when args.length >= 2 passes on line 22.
♻️ Suggested removal
const toolName = args[0]
const toolArgsString = args[1]
-
- if (!toolArgsString) {
- console.error('Error: Missing JSON arguments')
- process.exit(1)
- }🤖 Prompt for AI Agents
In `@packages/mcp/src/test-tool.ts` around lines 53 - 56, Remove the redundant
null/undefined check for toolArgsString: delete the if block that checks if
(!toolArgsString) and its console.error/process.exit calls in test-tool.ts
because args[1] is guaranteed when the earlier args.length >= 2 check passes;
rely on the existing args length validation (variable args) and the
toolArgsString usage instead of keeping this extra guard.
Comment on lines
+79
to
+81
| return new Promise((resolve, reject) => { | ||
| // Resolve bin.js relative to this script's location (process.argv[1]) | ||
| const scriptDir = path.dirname(process.argv[1] || '') |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Empty string fallback may produce invalid path.
If process.argv[1] is undefined, path.dirname('') returns '.', which might resolve bin.js incorrectly depending on cwd.
♻️ Explicit check
- const scriptDir = path.dirname(process.argv[1] || '')
+ const scriptPath = process.argv[1]
+ if (!scriptPath) {
+ throw new Error('Unable to determine script path')
+ }
+ const scriptDir = path.dirname(scriptPath)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return new Promise((resolve, reject) => { | |
| // Resolve bin.js relative to this script's location (process.argv[1]) | |
| const scriptDir = path.dirname(process.argv[1] || '') | |
| return new Promise((resolve, reject) => { | |
| // Resolve bin.js relative to this script's location (process.argv[1]) | |
| const scriptPath = process.argv[1] | |
| if (!scriptPath) { | |
| throw new Error('Unable to determine script path') | |
| } | |
| const scriptDir = path.dirname(scriptPath) |
🤖 Prompt for AI Agents
In `@packages/mcp/src/test-tool.ts` around lines 79 - 81, The code computes
scriptDir with path.dirname(process.argv[1] || ''), which yields '.' when
process.argv[1] is undefined and can produce an incorrect path for resolving
bin.js; change the fallback to a reliable file path (e.g., use process.argv[1]
?? __filename or process.argv[1] || __filename) so that scriptDir =
path.dirname(process.argv[1] ?? __filename) (or similar) inside the Promise
block where scriptDir is declared, ensuring you reference process.argv[1] and
__filename (or process.cwd() if you prefer) to avoid an empty-string dirname.
Comment on lines
+123
to
+126
| if (response.error) { | ||
| console.error('Error:', response.error.message || response.error) | ||
| process.exit(1) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
process.exit(1) inside promise leaves promise unresolved.
After process.exit(1), resolve() is never called. Node will exit anyway, but for cleanliness consider reject() instead.
♻️ Use reject for consistency
if (response.error) {
- console.error('Error:', response.error.message || response.error)
- process.exit(1)
+ reject(new Error(response.error.message || JSON.stringify(response.error)))
+ return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (response.error) { | |
| console.error('Error:', response.error.message || response.error) | |
| process.exit(1) | |
| } | |
| if (response.error) { | |
| reject(new Error(response.error.message || JSON.stringify(response.error))) | |
| return | |
| } |
🤖 Prompt for AI Agents
In `@packages/mcp/src/test-tool.ts` around lines 123 - 126, The code calls
process.exit(1) inside the promise when response.error is truthy, which leaves
the promise unresolved; instead reject the promise with the error so callers can
handle it. Replace the process.exit(1) path in the block that checks
response.error (the branch referencing response.error.message || response.error)
to call the surrounding Promise's reject(...) with response.error (or new
Error(String(response.error))) and return afterward (do not call resolve),
ensuring the promise is settled instead of forcing exit.
- prompts.test.ts: 10 tests for MCP prompts and getPrompt function - resources.test.ts: 10 tests for resource discovery and reading - tools/tools.test.ts: 22 tests for tool definitions and schemas - tools/magic.test.ts: 16 tests for magic tool handlers
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/mcp/src/prompts.test.ts`:
- Around line 56-72: Add tests in prompts.test.ts that call
getPrompt('convert_and_enhance') and getPrompt('fix_and_document') without
passing the required param objects to cover default-argument behavior; ensure
each call does not throw, that result.messages[0].content exists and its text
contains the same tokens you already assert for the parameterized cases (e.g.,
for convert_and_enhance assert presence of 'deepnote_convert_to',
'deepnote_enhance' and a placeholder or default filename if applicable; for
fix_and_document assert 'deepnote_lint' and 'deepnote_fix'), so the contract is
locked when args are omitted.
In `@packages/mcp/src/resources.test.ts`:
- Around line 116-146: Add a test case that verifies readResource handles file
paths containing spaces and unicode by creating a temp file whose name includes
spaces and a non-ASCII character (e.g., "indiv ü test.deepnote"), writing the
same notebook content to that path (use the existing test's testNotebook and
filePath creation logic but with special chars), call readResource with an
appropriately encoded URI (use encodeURI or encodeURIComponent on the path when
building `deepnote://file/${filePath}`), and assert the same expectations as the
existing "reads individual notebook file" test (contents length, uri equals the
encoded URI, mimeType, and parsed data contains projectName or error). Reference
the existing test block and the readResource function to locate where to
add/modify the test.
- Around line 10-16: The afterEach cleanup assumes tempDir was created and calls
fs.rm unconditionally; if fs.mkdtemp in beforeEach throws this will attempt to
delete an undefined path and mask the original error. Modify the test setup so
tempDir is declared/initialized (e.g., let tempDir: string | undefined) and
change the afterEach to only call await fs.rm(tempDir, { recursive: true, force:
true }) when tempDir is defined/truthy; this ensures beforeEach's mkdtemp errors
are not shadowed and cleanup is skipped safely.
In `@packages/mcp/src/tools/magic.test.ts`:
- Around line 16-22: The afterEach cleanup currently calls fs.rm(tempDir...)
unconditionally which can throw or mask errors if mkdtemp failed; update
afterEach to guard removal by checking that tempDir is set and the directory
exists (e.g., typeof tempDir !== 'undefined' or a truthy check and optionally
fs.stat/fs.access) before calling fs.rm, or wrap the removal in a try/catch to
swallow only ENOENT-type errors so the original mkdtemp error is not masked;
reference the beforeEach (mkdtemp) and afterEach (fs.rm) and the tempDir
variable when making this change.
Comment on lines
+56
to
+72
| it('returns convert_and_enhance prompt', () => { | ||
| const result = getPrompt('convert_and_enhance', { source_path: 'my_notebook.ipynb' }) | ||
| expect(result.messages[0].content).toMatchObject({ type: 'text' }) | ||
| const content = result.messages[0].content as { type: string; text: string } | ||
| expect(content.text).toContain('my_notebook.ipynb') | ||
| expect(content.text).toContain('deepnote_convert_to') | ||
| expect(c 3486 ontent.text).toContain('deepnote_enhance') | ||
| }) | ||
|
|
||
| it('returns fix_and_document prompt', () => { | ||
| const result = getPrompt('fix_and_document', { notebook_path: 'analysis.deepnote' }) | ||
| const content = result.messages[0].content as { type: string; text: string } | ||
| expect(content.text).toContain('analysis.deepnote') | ||
| expect(content.text).toContain('deepnote_lint') | ||
| expect(content.text).toContain('deepnote_fix') | ||
| }) | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add default-argument coverage for required params.
convert_and_enhance and fix_and_document have required args; add tests for default behavior when args are omitted to lock the contract.
🧪 Suggested tests
+ it('returns convert_and_enhance prompt with default source_path', () => {
+ const result = getPrompt('convert_and_enhance', undefined)
+ const content = result.messages[0].content as { type: string; text: string }
+ expect(content.text).toContain('notebook.ipynb')
+ })
+
+ it('returns fix_and_document prompt with default notebook_path', () => {
+ const result = getPrompt('fix_and_document', undefined)
+ const content = result.messages[0].content as { type: string; text: string }
+ expect(content.text).toContain('notebook.deepnote')
+ })As per coding guidelines, "Test edge cases, error handling, and special characters".
🤖 Prompt for AI Agents
In `@packages/mcp/src/prompts.test.ts` around lines 56 - 72, Add tests in
prompts.test.ts that call getPrompt('convert_and_enhance') and
getPrompt('fix_and_document') without passing the required param objects to
cover default-argument behavior; ensure each call does not throw, that
result.messages[0].content exists and its text contains the same tokens you
already assert for the parameterized cases (e.g., for convert_and_enhance assert
presence of 'deepnote_convert_to', 'deepnote_enhance' and a placeholder or
default filename if applicable; for fix_and_document assert 'deepnote_lint' and
'deepnote_fix'), so the contract is locked when args are omitted.
Comment on lines
+10
to
+16
| beforeEach(async () => { | ||
| tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcp-resources-test-')) | ||
| }) | ||
|
|
||
| afterEach(async () => { | ||
| await fs.rm(tempDir, { recursive: true, force: true }) | ||
| }) |
Contributor
There was a problem hiding this comment.
Guard cleanup when temp dir setup fails.
If mkdtemp throws, afterEach will attempt to delete an undefined path and mask the original error.
🔧 Proposed fix
- let tempDir: string
+ let tempDir: string | undefined
@@
- afterEach(async () => {
- await fs.rm(tempDir, { recursive: true, force: true })
- })
+ afterEach(async () => {
+ if (tempDir) {
+ await fs.rm(tempDir, { recursive: true, force: true })
+ }
+ })🤖 Prompt for AI Agents
In `@packages/mcp/src/resources.test.ts` around lines 10 - 16, The afterEach
cleanup assumes tempDir was created and calls fs.rm unconditionally; if
fs.mkdtemp in beforeEach throws this will attempt to delete an undefined path
and mask the original error. Modify the test setup so tempDir is
declared/initialized (e.g., let tempDir: string | undefined) and change the
afterEach to only call await fs.rm(tempDir, { recursive: true, force: true })
when tempDir is defined/truthy; this ensures beforeEach's mkdtemp errors are not
shadowed and cleanup is skipped safely.
Comment on lines
+116
to
+146
| it('reads individual notebook file', async () => { | ||
| const testNotebook = `version: "1.0" | ||
| project: | ||
| name: "Individual Test" | ||
| id: "ind-123" | ||
| notebooks: | ||
| - name: "Analysis" | ||
| id: "ana-123" | ||
| blocks: | ||
| - id: "b1" | ||
| type: "markdown" | ||
| content: "# Hello" | ||
| - id: "b2" | ||
| type: "code" | ||
| content: "x = 42" | ||
| metadata: | ||
| createdAt: "2026-01-01T00:00:00Z" | ||
| modifiedAt: "2026-01-01T00:00:00Z" | ||
| ` | ||
| const filePath = path.join(tempDir, 'individual.deepnote 571A 9;) | ||
| await fs.writeFile(filePath, testNotebook) | ||
|
|
||
| const contents = await readResource(`deepnote://file/${filePath}`) | ||
| expect(contents).toHaveLength(1) | ||
| expect(contents[0].uri).toBe(`deepnote://file/${filePath}`) | ||
| expect(contents[0].mimeType).toBe('application/json') | ||
|
|
||
| const data = JSON.parse((contents[0] as { text: string }).text) | ||
| // Response should have either notebook data or an error | ||
| expect(data.projectName || data.error).toBeDefined() | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a special-characters path case for URI handling.
Paths with spaces/unicode are common and can break URI parsing if not handled consistently.
🧪 Suggested test
+ it('reads notebook with special characters in path', async () => {
+ const testNotebook = `version: "1.0"
+project:
+ name: "Special Chars"
+ id: "spec-123"
+ notebooks:
+ - name: "Main"
+ id: "main-123"
+ blocks: []
+metadata:
+ createdAt: "2026-01-01T00:00:00Z"
+ modifiedAt: "2026-01-01T00:00:00Z"
+`
+ const filePath = path.join(tempDir, 'my notebook №1.deepnote')
+ await fs.writeFile(filePath, testNotebook)
+
+ const contents = await readResource(`deepnote://file/${filePath}`)
+ const data = JSON.parse((contents[0] as { text: string }).text)
+ expect(data.projectName || data.error).toBeDefined()
+ })As per coding guidelines, "Test edge cases, error handling, and special characters".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('reads individual notebook file', async () => { | |
| const testNotebook = `version: "1.0" | |
| project: | |
| name: "Individual Test" | |
| id: "ind-123" | |
| notebooks: | |
| - name: "Analysis" | |
| id: "ana-123" | |
| blocks: | |
| - id: "b1" | |
| type: "markdown" | |
| content: "# Hello" | |
| - id: "b2" | |
| type: "code" | |
| content: "x = 42" | |
| metadata: | |
| createdAt: "2026-01-01T00:00:00Z" | |
| modifiedAt: "2026-01-01T00:00:00Z" | |
| ` | |
| const filePath = path< B4A6 span class="pl-kos">.join(tempDir, 'individual.deepnote') | |
| await fs.writeFile(filePath, testNotebook) | |
| const contents = await readResource(`deepnote://file/${filePath}`) | |
| expect(contents).toHaveLength(1) | |
| expect(contents[0].uri).toBe(`deepnote://file/${filePath}`) | |
| expect(contents[0].mimeType).toBe('application/json') | |
| const data = JSON.parse((contents[0] as { text: string }).text) | |
| // Response should have either notebook data or an error | |
| expect(data.projectName || data.error).toBeDefined() | |
| }) | |
| it('reads individual notebook file', async () => { | |
| const testNotebook = `version: "1.0" | |
| project: | |
| name: "Individual Test" | |
| id: "ind-123" | |
| notebooks: | |
| - name: "Analysis" | |
| id: "ana-123" | |
| blocks: | |
| - id: "b1" | |
| type: "markdown" | |
| content: "# Hello" | |
| - id: "b2" | |
| type: "code" | |
| content: "x = 42" | |
| metadata: | |
| createdAt: "2026-01-01T00:00:00Z" | |
| modifiedAt: "2026-01-01T00:00:00Z" | |
| ` | |
| const filePath = path.join(tempDir, 'individual.deepnote') | |
| await fs.writeFile(filePath, testNotebook) | |
| const contents = await readResource(`deepnote://file/${filePath}`) | |
| expect(contents).toHaveLength(1) | |
| expect(contents[0].uri).toBe(`deepnote://file/${filePath}`) | |
| expect(contents[0].mimeType).toBe('application/json') | |
| const data = JSON.parse((contents[0] as { text: string }).text) | |
| // Response should have either notebook data or an error | |
| expect(data.projectName || data.error).toBeDefined() | |
| }) | |
| it('reads notebook with special characters in path', async () => { | |
| const testNotebook = `version: "1.0" | |
| project: | |
| name: "Special Chars" | |
| id: "spec-123" | |
| notebooks: | |
| - name: "Main" | |
| id: "main-123" | |
| blocks: [] | |
| metadata: | |
| createdAt: "2026-01-01T00:00:00Z" | |
| modifiedAt: "2026-01-01T00:00:00Z" | |
| ` | |
| const filePath = path.join(tempDir, 'my notebook №1.deepnote') | |
| await fs.writeFile(filePath, testNotebook) | |
| const contents = await readResource(`deepnote://file/${filePath}`) | |
| const data = JSON.parse((contents[0] as { text: string }).text) | |
| expect(data.projectName || data.error).toBeDefined() | |
| }) |
🤖 Prompt for AI Agents
In `@packages/mcp/src/resources.test.ts` around lines 116 - 146, Add a test case
that verifies readResource handles file paths containing spaces and unicode by
creating a temp file whose name includes spaces and a non-ASCII character (e.g.,
"indiv ü test.deepnote"), writing the same notebook content to that path (use
the existing test's testNotebook and filePath creation logic but with special
chars), call readResource with an appropriately encoded URI (use encodeURI or
encodeURIComponent on the path when building `deepnote://file/${filePath}`), and
assert the same expectations as the existing "reads individual notebook file"
test (contents length, uri equals the encoded URI, mimeType, and parsed data
contains projectName or error). Reference the existing test block and the
readResource function to locate where to add/modify the test.
Comment on lines
+16
to
+22
| beforeEach(async () => { | ||
| tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcp-magic-test-')) | ||
| }) | ||
|
|
||
| afterEach(async () => { | ||
| await fs.rm(tempDir, { recursive: true, force: true }) | ||
| }) |
Contributor
There was a problem hiding this comment.
Guard cleanup when temp dir setup fails.
If mkdtemp throws, cleanup will throw and mask the root cause.
🔧 Proposed fix
- let tempDir: string
+ let tempDir: string | undefined
@@
- afterEach(async () => {
- await fs.rm(tempDir, { recursive: true, force: true })
- })
+ afterEach(async () => {
+ if (tempDir) {
+ await fs.rm(tempDir, { recursive: true, force: true })
+ }
+ })🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/magic.test.ts` around lines 16 - 22, The afterEach
cleanup currently calls fs.rm(tempDir...) unconditionally which can throw or
mask errors if mkdtemp failed; update afterEach to guard removal by checking
that tempDir is set and the directory exists (e.g., typeof tempDir !==
'undefined' or a truthy check and optionally fs.stat/fs.access) before calling
fs.rm, or wrap the removal in a try/catch to swallow only ENOENT-type errors so
the original mkdtemp error is not masked; reference the beforeEach (mkdtemp) and
afterEach (fs.rm) and the tempDir variable when making this change.
- Add 4 snapshot tools: list, load, split, merge - Fix deepnote_run to run ALL notebooks by default (project level) - Document 3 execution levels: project, notebook, block - Document that snapshots are runnable for debugging
Contributor
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 158-162: The error responses in handleSnapshotList (the
early-return when filePath is falsy and the similar block around lines 195-198)
are missing the isError: true flag; update those returned objects to include
isError: true so they match the pattern used in handleSnapshotTool (see its
default/error branch in handleSnapshotTool) and ensure all error return shapes
consistently include isError: true alongside the content payload.
- Around line 219-227: The code repeats the same logic to count notebook blocks
with outputs (iterating snapshot.project.notebooks -> notebook.blocks and
checking (block as { outputs?: unknown[] }).outputs) in four places; extract
that into a single helper named countBlocksWithOutputs(snapshot:
DeepnoteSnapshot): number that iterates snapshot.project.notebooks and
increments when (block as { outputs?: unknown[] }).outputs?.length is truthy,
return the count, and replace each duplicated loop (the totalOutputs variable
code at the shown diff and the similar blocks at the other locations) with a
call to countBlocksWithOutputs(snapshot) to compute the value. Ensure the helper
is exported or scoped where all call sites can access it and keep the same
typing for DeepnoteSnapshot and the execBlock cast.
- Around line 205-209: Several early-return error responses in snapshots.ts
return a content payload with an error message but omit the standardized isError
flag; update every error return (e.g., the filePath guard and the other error
blocks mentioned around lines 305-309, 317-321, 394-399, 408-412, 480-485) so
the response includes isError: true alongside the existing content structure
(for example JSON.stringify({ error: '...', isError: true })). Ensure every
branch that returns an error object uses the same shape: { isError: true,
content: [...] } or embeds isError: true inside the JSON error payload where
currently missing.
Comment on lines
+158
to
+162
| if (!filePath) { | ||
| return { | ||
| content: [{ type: 'text', text: JSON.stringify({ error: 'path is required' }) }], | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Missing isError: true on error responses.
handleSnapshotList returns errors without isError flag, unlike handleSnapshotTool default case (line 503). Be consistent.
Proposed fix
return {
content: [{ type: 'text', text: JSON.stringify({ error: 'path is required' }) }],
+ isError: true,
}Also applies to: 195-198
🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/snapshots.ts` around lines 158 - 162, The error
responses in handleSnapshotList (the early-return when filePath is falsy and the
similar block around lines 195-198) are missing the isError: true flag; update
those returned objects to include isError: true so they match the pattern used
in handleSnapshotTool (see its default/error branch in handleSnapshotTool) and
ensure all error return shapes consistently include isError: true alongside the
content payload.
Comment on lines
+205
to
+209
| if (!filePath) { | ||
| return { | ||
| content: [{ type: 'text', text: JSON.stringify({ error: 'path is required' }) }], | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent isError flag across error responses.
Some error returns include isError: true, others don't. Standardize all error responses.
Also applies to: 305-309, 317-321, 394-399, 408-412, 480-485
🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/snapshots.ts` around lines 205 - 209, Several
early-return error responses in snapshots.ts return a content payload with an
error message but omit the standardized isError flag; update every error return
(e.g., the filePath guard and the other error blocks mentioned around lines
305-309, 317-321, 394-399, 408-412, 480-485) so the response includes isError:
true alongside the existing content structure (for example JSON.stringify({
error: '...', isError: true })). Ensure every branch that returns an error
object uses the same shape: { isError: true, content: [...] } or embeds isError:
true inside the JSON error payload where currently missing.
Comment on lines
+219
to
+227
| let totalOutputs = 0 | ||
| for (const notebook of snapshot.project.notebooks) { | ||
| for (const block of notebook.blocks) { | ||
| const execBlock = block as { outputs?: unknown[] } | ||
| if (execBlock.outputs && execB 571A lock.outputs.length > 0) { | ||
| totalOutputs++ | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract duplicated output counting logic.
Same pattern appears 4 times. Consider a helper function.
function countBlocksWithOutputs(snapshot: DeepnoteSnapshot): number {
let count = 0
for (const notebook of snapshot.project.notebooks) {
for (const block of notebook.blocks) {
const execBlock = block as { outputs?: unknown[] }
if (execBlock.outputs?.length) count++
}
}
return count
}Also applies to: 273-281, 349-357, 448-456
🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/snapshots.ts` around lines 219 - 227, The code repeats
the same logic to count notebook blocks with outputs (iterating
snapshot.project.notebooks -> notebook.blocks and checking (block as { outputs?:
unknown[] }).outputs) in four places; extract that into a single helper named
countBlocksWithOutputs(snapshot: DeepnoteSnapshot): number that iterates
snapshot.project.notebooks and increments when (block as { outputs?: unknown[]
}).outputs?.length is truthy, return the count, and replace each duplicated loop
(the totalOutputs variable code at the shown diff and the similar blocks at the
other locations) with a call to countBlocksWithOutputs(snapshot) to compute the
value. Ensure the helper is exported or scoped where all call sites can access
it and keep the same typing for DeepnoteSnapshot and the execBlock cast.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/mcp/src/tools/tools.test.ts`:
- Around line 85-100: The test currently checks that several tools require a
'path' parameter but misses asserting that deepnote_validate requires 'path' and
that deepnote_diff requires 'path1' and 'path2'; update the test loop to include
'deepnote_validate' in the pathRequiredTools list (so readingTools.find(...) and
schema checks validate presence of schema.properties.path and schema.required
contains 'path' for deepnote_validate), and add a separate assertion block for
the 'deepnote_diff' tool (use readingTools.find(t => t.name === 'deepnote_diff')
to get its InputSchema and assert schema.properties.path1 and
schema.properties.path2 are defined and schema.required contains both 'path1'
and 'path2').
Comment on lines
+85
to
+100
| it('all reading tools require path parameter', () => { | ||
| const pathRequiredTools = [ | ||
| 'deepnote_inspect', | ||
| 'deepnote_cat', | ||
| 'deepnote_lint', | ||
| 'deepnote_stats', | ||
| 'deepnote_analyze', | ||
| 'deepnote_dag', | ||
| ] | ||
| for (const name of pathRequiredTools) { | ||
| const tool = readingTools.find(t => t.name === name) | ||
| const schema = tool?.inputSchema as InputSchema | ||
| expect(schema?.properties?.path).toBeDefined() | ||
| expect(schema?.required).toContain('path') | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extend path-required assertions for validate/diff.
deepnote_validate requires path, and deepnote_diff should enforce path1/path2 explicitly.
♻️ Suggested test additions
const pathRequiredTools = [
'deepnote_inspect',
'deepnote_cat',
'deepnote_lint',
'deepnote_stats',
'deepnote_analyze',
'deepnote_dag',
+ 'deepnote_validate',
]
for (const name of pathRequiredTools) {
const tool = readingTools.find(t => t.name === name)
const schema = tool?.inputSchema as InputSchema
expect(schema?.properties?.path).toBeDefined()
expect(schema?.required).toContain('path')
}
+
+ const diffTool = readingTools.find(t => t.name === 'deepnote_diff')
+ const diffSchema = diffTool?.inputSchema as InputSchema
+ const diffRequired = diffSchema?.required || []
+ expect(diffRequired).toEqual(expect.arrayContaining(['path1', 'path2']))🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/tools.test.ts` around lines 85 - 100, The test
currently checks that several tools require a 'path' parameter but misses
asserting that deepnote_validate requires 'path' and that deepnote_diff requires
'path1' and 'path2'; update the test loop to include 'deepnote_validate' in the
pathRequiredTools list (so readingTools.find(...) and schema checks validate
presence of schema.properties.path and schema.required contains 'path' for
deepnote_validate), and add a separate assertion block for the 'deepnote_diff'
tool (use readingTools.find(t => t.name === 'deepnote_diff') to get its
InputSchema and assert schema.properties.path1 and schema.properties.path2 are
defined and schema.required contains both 'path1' and 'path2').
…ave and multi-format support - Update server instructions to clearly explain that all execution outputs (stdout, stderr, charts, errors, timing) are saved to snapshot files - Add automatic snapshot saving after deepnote_run execution - Add multi-format support: deepnote_run now accepts .ipynb, .py, .qmd files - Add debug_execution prompt showing how to use snapshots for debugging - Update tool descriptions to reference snapshotPath in responses - Restore example snapshots for documentation purposes - Fix tests to work with updated examples structure
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@packages/blocks/src/deserialize-file/validate-all-deepnote-files.test.ts`:
- Around line 87-103: findSnapshots duplicates the recursive traversal logic in
findDeepnoteFiles; extract a single helper (e.g., findFilesRecursive or
walkAndFilter) that accepts (dir: string, predicate: (entry: Dirent, fullPath:
string) => boolean) and reuses readdir with withFileTypes, recursion, and error
handling, then replace both findSnapshots and findDeepnoteFiles to call this
helper with appropriate predicates (one checking
entry.name.endsWith('.snapshot.deepnote') for findSnapshots). Ensure the new
helper returns the same shape (Array<{ fileName: string; fullPath: string }>)
and update any tests that reference either function accordingly.
- Around line 108-114: The test currently returns early when
snapshotFiles.length === 0 which makes it silently pass; change it to
conditionally skip so the test output shows the skip and/or log a warning.
Replace the early-return pattern in the 'discovers .snapshot.deepnote files in
test-fixtures' test by selecting the test runner based on snapshotFiles (e.g.,
const maybeIt = snapshotFiles.length === 0 ? it.skip : it) and use maybeIt(...)
for the test, and add a console.warn or similar message when
snapshotFiles.length === 0 so the CI/test output clearly indicates why the test
was skipped; reference the snapshotFiles variable and the test name when making
the change.
- Around line 99-101: The empty catch in the test silently swallows all
filesystem errors; change it to catch the error object (catch (err)) and only
suppress the expected "directory not found" case (err.code === 'ENOENT' or
similar), but rethrow or log and fail for other errors (e.g., permission errors
like EACCES) so unexpected FS issues surface; update the catch block in
validate-all-deepnote-files.test.ts accordingly to either rethrow non-ENOENT
errors or call console.error / fail the test.
In `@packages/cli/src/commands/diff.test.ts`:
- Around line 359-361: Replace the manual fixture path construction in the diff
test (the file1 and file2 variables that call resolve(process.cwd(),
'test-fixtures/diff/...')) with the standard helper getDiffFixturePath() used by
other tests; specifically call
getDiffFixturePath('base-modified-blocks.deepnote') for the base fixture and
getDiffFixturePath('modified-blocks.deepnote') for the modified fixture so the
test uses the same fixture-loading pattern as the rest of the suite.
In `@packages/mcp/src/instructions.ts`:
- Line 122: The instruction text "Use deepnote_lint to catch issues before
running" is missing inline code formatting; update that literal so it matches
the style used elsewhere (e.g., change the phrase to use backticks around
deepnote_lint) — look for the exact string "Use deepnote_lint to catch issues
before running" in packages/mcp/src/instructions.ts and wrap deepnote_lint in
backticks to be consistent with the `deepnote_lint` usage on line 87.
In `@packages/mcp/src/prompts.ts`:
- Around line 67-84: Ensure required prompt arguments are actually enforced:
update getPrompt to validate that required args are present before delegating to
helpers (getDebugExecutionPrompt, getCreateNotebookPrompt,
getConvertAndEnhancePrompt, getFixAndDocumentPrompt); for each case, check the
specific required keys declared in that prompt's schema (or maintain a small map
of required keys per prompt name) and throw a clear Error when a required arg is
missing, or alternatively remove the `required: true` flags from the prompt
schemas if you prefer keeping defaults—make the change so the declared schema
accurately reflects runtime behavior.
In `@packages/mcp/src/tools/execution.ts`:
- Around line 559-561: In deepnote_open, guard the fs.readFile(absolutePath)
call so a file removed between stat and read doesn't throw: wrap the read in a
try/catch (or a Promise .catch) around the fs.readFile call that produces
fileBuffer and, on error, return the proper error to the caller/handler (e.g.,
call the open callback with an ENOENT/EIO appropriate errno) and log the failure
instead of letting the exception escape; update any code paths that use
fileBuffer to only proceed after successful read.
- Around line 449-456: The call to loadDeepnoteFile inside handleRunBlock can
throw on missing/invalid files and currently lets the exception escape the tool;
wrap the await loadDeepnoteFile(filePath) in a try/catch inside handleRunBlock,
catch errors from loadDeepnoteFile and return or throw a handled tool response
(e.g., a structured error/result for the deepnote_run_block tool) that includes
a clear message and the original error message, so the tool handler doesn't
crash; update any callers or the deepnote_run_block contract to expect this
handled error shape if needed.
- Around line 457-460: The variables targetBlock and targetNotebook are declared
as "let ... = null" which yields implicit any; change their declarations to
explicit nullable types (e.g., the concrete Block and Notebook types used in
this module) — replace the implicit any with the proper type unions (Block |
null and Notebook | null or the exact names used elsewhere in this file) and add
any needed imports for those types so strictTypeChecking passes; update the
declarations for targetBlock and targetNotebook in execution.ts accordingly.
- Around line 290-299: The call to resolveAndConvertToDeepnote inside handleRun
can throw before the existing try/catch; wrap that call in its own try/catch so
conversion/parse errors are caught and converted into the tool's structured
failure response (e.g., return a consistent object with a success/error/message
shape) instead of bubbling out. Specifically, update handleRun to catch
exceptions from resolveAndConvertToDeepnote, extr
B4A6
act the error message/stack,
and return the same structured error format used by other failures in this
handler; reference the handleRun function and the resolveAndConvertToDeepnote
call when making the change.
Comment on lines
+87
to
+103
| const findSnapshots = async (dir: string): Promise<Array<{ fileName: string; fullPath: string }>> => { | ||
| const results: Array<{ fileName: string; fullPath: string }> = [] | ||
| try { | ||
| const entries = await readdir(dir, { withFileTypes: true }) | ||
| for (const entry of entries) { | ||
| const fullPath = join(dir, entry.name) | ||
| if (entry.isDirectory()) { | ||
| results.push(...(await findSnapshots(fullPath))) | ||
| } else if (entry.isFile() && entry.name.endsWith('.snapshot.deepnote')) { | ||
| results.push({ fileName: entry.name, fullPath }) | ||
| } | ||
| } | ||
| } catch { | ||
| // Directory doesn't exist or not readable | ||
| } | ||
| return results | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicate recursive file finder.
findSnapshots largely mirrors findDeepnoteFiles (lines 11-29). Consider extracting a shared helper with a filter predicate.
🤖 Prompt for AI Agents
In `@packages/blocks/src/deserialize-file/validate-all-deepnote-files.test.ts`
around lines 87 - 103, findSnapshots duplicates the recursive traversal logic in
findDeepnoteFiles; extract a single helper (e.g., findFilesRecursive or
walkAndFilter) that accepts (dir: string, predicate: (entry: Dirent, fullPath:
string) => boolean) and reuses readdir with withFileTypes, recursion, and error
handling, then replace both findSnapshots and findDeepnoteFiles to call this
helper with appropriate predicates (one checking
entry.name.endsWith('.snapshot.deepnote') for findSnapshots). Ensure the new
helper returns the same shape (Array<{ fileName: string; fullPath: string }>)
and update any tests that reference either function accordingly.
Comment on lines
+99
to
+101
| } catch { | ||
| // Directory doesn't exist or not readable | ||
| } |
Contributor
There was a problem hiding this comment.
Silent error swallowing hides failures.
Empty catch block could mask permission errors or unexpected filesystem issues. Consider logging or narrowing the catch.
Suggested fix
- } catch {
- // Directory doesn't exist or not readable
+ } catch (err) {
+ // Only ignore ENOENT (directory doesn't exist)
+ if ((err as NodeJS.ErrnoException).code !== 'ENOENT') {
+ throw err
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch { | |
| // Directory doesn't exist or not readable | |
| } | |
| } catch (err) { | |
| // Only ignore ENOENT (directory doesn't exist) | |
| if ((err as NodeJS.ErrnoException).code !== 'ENOENT') { | |
| throw err | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/blocks/src/deserialize-file/validate-all-deepnote-files.test.ts`
around lines 99 - 101, The empty catch in the test silently swallows all
filesystem errors; change it to catch the error object (catch (err)) and only
suppress the expected "directory not found" case (err.code === 'ENOENT' or
similar), but rethrow or log and fail for other errors (e.g., permission errors
like EACCES) so unexpected FS issues surface; update the catch block in
validate-all-deepnote-files.test.ts accordingly to either rethrow non-ENOENT
errors or call console.error / fail the test.
Comment on lines
+108
to
114
| it('discovers .snapshot.deepnote files in test-fixtures', () => { | ||
| // This test is skipped if no snapshot files exist (they're gitignored) | ||
| if (snapshotFiles.length === 0) { | ||
| return | ||
| } | ||
| expect(snapshotFiles.length).toBeGreaterThan(0) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Silent skip may mask issues.
Test passes vacuously when no files exist. Consider using it.skipIf() or logging to make skips visible in test output.
Alternative approach
- it('discovers .snapshot.deepnote files in test-fixtures', () => {
- // This test is skipped if no snapshot files exist (they're gitignored)
- if (snapshotFiles.length === 0) {
- return
- }
+ it('discovers .snapshot.deepnote files in test-fixtures', ({ skip }) => {
+ if (snapshotFiles.length === 0) {
+ skip()
+ return
+ }
expect(snapshotFiles.length).toBeGreaterThan(0)
})🤖 Prompt for AI Agents
In `@packages/blocks/src/deserialize-file/validate-all-deepnote-files.test.ts`
around lines 108 - 114, The test currently returns early when
snapshotFiles.length === 0 which makes it silently pass; change it to
conditionally skip so the test output shows the skip and/or log a warning.
Replace the early-return pattern in the 'discovers .snapshot.deepnote files in
test-fixtures' test by selecting the test runner based on snapshotFiles (e.g.,
const maybeIt = snapshotFiles.length === 0 ? it.skip : it) and use maybeIt(...)
for the test, and add a console.warn or similar message when
snapshotFiles.length === 0 so the CI/test output clearly indicates why the test
was skipped; reference the snapshotFiles variable and the test name when making
the change.
Comment on lines
+359
to
+361
| // Compare two files with modified blocks | < B4A6 /tr>||
| const file1 = resolve(process.cwd(), 'test-fixtures/diff/base-modified-blocks.deepnote') | ||
| const file2 = resolve(process.cwd(), 'test-fixtures/diff/modified-blocks.deepnote') |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent fixture loading pattern.
Other tests use getDiffFixturePath() helper for diff fixtures. Consider using it here for consistency.
Suggested fix
- // Compare two files with modified blocks
- const file1 = resolve(process.cwd(), 'test-fixtures/diff/base-modified-blocks.deepnote')
- const file2 = resolve(process.cwd(), 'test-fixtures/diff/modified-blocks.deepnote')
+ // Compare two files with modified blocks
+ const file1 = getDiffFixturePath('base-modified-blocks.deepnote')
+ const file2 = getDiffFixturePath('modified-blocks.deepnote')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Compare two files with modified blocks | |
| const file1 = resolve(process.cwd(), 'test-fixtures/diff/base-modified-blocks.deepnote') | |
| const file2 = resolve(process.cwd(), 'test-fixtures/diff/modified-blocks.deepnote') | |
| // Compare two files with modified blocks | |
| const file1 = getDiffFixturePath('base-modified-blocks.deepnote') | |
| const file2 = getDiffFixturePath('modified-blocks.deepnote') |
🤖 Prompt for AI Agents
In `@packages/cli/src/commands/diff.test.ts` around lines 359 - 361, Replace the
manual fixture path construction in the diff test (the file1 and file2 variables
that call resolve(process.cwd(), 'test-fixtures/diff/...')) with the standard
helper getDiffFixturePath() used by other tests; specifically call
getDiffFixturePath('base-modified-blocks.deepnote') for the base fixture and
getDiffFixturePath('modified-blocks.deepnote') for the modified fixture so the
test uses the same fixture-loading pattern as the rest of the suite.
| - Organize with section headers (text-cell-h2) | ||
| - Convert hardcoded values to input blocks for interactivity | ||
| - Add markdown explanations before complex code | ||
| - Use deepnote_lint to catch issues before running |
Contributor
There was a problem hiding this comment.
Inconsistent formatting: missing backticks around deepnote_lint.
Line 87 uses \deepnote_lint`` but line 122 omits the backticks.
Proposed fix
-- Use deepnote_lint to catch issues before running
+- Use \`deepnote_lint\` to catch issues before running📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - Use deepnote_lint to catch issues before running | |
| - Use `deepnote_lint` to catch issues before running |
🤖 Prompt for AI Agents
In `@packages/mcp/src/instructions.ts` at line 122, The instruction text "Use
deepnote_lint to catch issues before running" is missing inline code formatting;
update that literal so it matches the style used elsewhere (e.g., change the
phrase to use backticks around deepnote_lint) — look for the exact string "Use
deepnote_lint to catch issues before running" in
packages/mcp/src/instructions.ts and wrap deepnote_lint in backticks to be
consistent with the `deepnote_lint` usage on line 87.
Comment on lines
+67
to
+84
| export function getPrompt(name: string, args: Record<string, string> | undefined): GetPromptResult { | ||
| switch (name) { | ||
| case 'debug_execution': | ||
| return getDebugExecutionPrompt(args) | ||
| case 'create_notebook': | ||
| return getCreateNotebookPrompt(args) | ||
| case 'convert_and_enhance': | ||
| return getConvertAndEnhancePrompt(args) | ||
| case 'fix_and_document': | ||
| return getFixAndDocumentPrompt(args) | ||
| case 'block_types_reference': | ||
| return getBlockTypesReferencePrompt() | ||
| case 'best_practices': | ||
| return getBestPracticesPrompt() | ||
| default: | ||
| throw new Error(`Unknown prompt: ${name}`) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Required arguments aren't validated.
The prompts array declares some arguments as required: true, but getPrompt passes args directly to helpers which fall back to defaults. The schema contract is misleading.
Options:
- Validate required args and throw if missing.
- Remove
required: truefrom schemas since defaults exist.
🔧 Option 1: Add validation
export function getPrompt(name: string, args: Record<string, string> | undefined): GetPromptResult {
+ const promptDef = prompts.find((p) => p.name === name)
+ if (!promptDef) {
+ throw new Error(`Unknown prompt: ${name}`)
+ }
+ for (const arg of promptDef.arguments ?? []) {
+ if (arg.required && !args?.[arg.name]) {
+ throw new Error(`Missing required argument: ${arg.name}`)
+ }
+ }
switch (name) {
case 'debug_execution':
return getDebugExecutionPrompt(args)
// ...
- default:
- throw new Error(`Unknown prompt: ${name}`)
}
+ throw new Error(`Unknown prompt: ${name}`)
}🤖 Prompt for AI Agents
In `@packages/mcp/src/prompts.ts` around lines 67 - 84, Ensure required prompt
arguments are actually enforced: update getPrompt to validate that required args
are present before delegating to helpers (getDebugExecutionPrompt,
getCreateNotebookPrompt, getConvertAndEnhancePrompt, getFixAndDocumentPrompt);
for each case, check the specific required keys declared in that prompt's schema
(or maintain a small map of required keys per prompt name) and throw a clear
Error when a required arg is missing, or alternatively remove the `required:
true` flags from the prompt schemas if you prefer keeping defaults—make the
change so the declared schema accurately reflects runtime behavior.
Comment on lines
+290
to
+299
| async function handleRun(args: Record<string, unknown>) { | ||
| const filePath = args.path as string | ||
| const notebookFilter = args.notebook as string | undefined | ||
| const pythonPath = args.pythonPath as string | undefined | ||
| const inputs = args.inputs as Record<string, unknown> | undefined | ||
| const dryRun = args.dryRun as boolean | undefined | ||
|
|
||
| // Load file, auto-converting from other formats if needed | ||
| const { file, originalPath, format, wasConverted } = await resolveAndConvertToDeepnote(filePath) | ||
|
|
Contributor
There was a problem hiding this comment.
Catch conversion/parse errors before execution.
resolveAndConvertToDeepnote can throw before the try/catch, which will bubble out of the tool handler. Return a structured error instead.
🩹 Suggested fix
async function handleRun(args: Record<string, unknown>) {
const filePath = args.path as string
const notebookFilter = args.notebook as string | undefined
const pythonPath = args.pythonPath as string | undefined
const inputs = args.inputs as Record<string, unknown> | undefined
const dryRun = args.dryRun as boolean | undefined
// Load file, auto-converting from other formats if needed
- const { file, originalPath, format, wasConverted } = await resolveAndConvertToDeepnote(filePath)
+ let converted: ConvertedFile
+ try {
+ converted = await resolveAndConvertToDeepnote(filePath)
+ } catch (error) {
+ const message = error instanceof Error ? error.message : String(error)
+ return {
+ content: [{ type: 'text', text: `Cannot load file: ${message}` }],
+ isError: true,
+ }
+ }
+ const { file, originalPath, format, wasConverted } = converted🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/execution.ts` around lines 290 - 299, The call to
resolveAndConvertToDeepnote inside handleRun can throw before the existing
try/catch; wrap that call in its own try/catch so conversion/parse errors are
caught and converted into the tool's structured failure response (e.g., return a
consistent object with a success/error/message shape) instead of bubbling out.
Specifically, update handleRun to catch exceptions from
resolveAndConvertToDeepnote, extract the error message/stack, and return the
same structured error format used by other failures in this handler; reference
the handleRun function and the resolveAndConvertToDeepnote call when making the
change.
Comment on lines
+449
to
+456
| async function handleRunBlock(args: Record<string, unknown>) { | ||
| const filePath = args.path as string | ||
| const blockId = args.blockId as string | ||
| const pythonPath = args.pythonPath as string | undefined | ||
| const inputs = args.inputs as Record<string, unknown> | undefined | ||
|
|
||
| const file = await loadDeepnoteFile(filePath) | ||
|
|
Contributor
There was a problem hiding this comment.
Handle file read/deserialize failures in deepnote_run_block.
loadDeepnoteFile can throw (missing file, invalid YAML). That currently escapes the tool handler.
🩹 Suggested fix
async function handleRunBlock(args: Record<string, unknown>) {
const filePath = args.path as string
const blockId = args.blockId as string
const pythonPath = args.pythonPath as string | undefined
const inputs = args.inputs as Record<string, unknown> | undefined
- const file = await loadDeepnoteFile(filePath)
+ let file: DeepnoteFile
+ try {
+ file = await loadDeepnoteFile(filePath)
+ } catch (error) {
+ const message = error instanceof Error ? error.message : String(error)
+ return {
+ content: [{ type: 'text', text: `Cannot load file: ${message}` }],
+ isError: true,
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function handleRunBlock(args: Record<string, unknown>) { | |
| const filePath = args.path as string | |
| const blockId = args.blockId as string | |
| const pythonPath = args.pythonPath as string | undefined | |
| const inputs = args.inputs as Record<string, unknown> | undefined | |
| const file = await loadDeepnoteFile(filePath) | |
| async function handleRunBlock(args: Record<string, unknown>) { | |
| const filePath = args.path as string | |
| const blockId = args.blockId as string | |
| const pythonPath = args.pythonPath as string | undefined | |
| const inputs = args.inputs as Record<string, unknown> | undefined | |
| let file: DeepnoteFile | |
| try { | |
| file = await loadDeepnoteFile(filePath) | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : String(error) | |
| return { | |
| content: [{ type: 'text', text: `Cannot load file: ${message}` }], | |
| isError: true, | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/execution.ts` around lines 449 - 456, The call to
loadDeepnoteFile inside handleRunBlock can throw on missing/invalid files and
currently lets the exception escape the tool; wrap the await
loadDeepnoteFile(filePath) in a try/catch inside handleRunBlock, catch errors
from loadDeepnoteFile and return or throw a handled tool response (e.g., a
structured error/result for the deepnote_run_block tool) that includes a clear
message and the original error message, so the tool handler doesn't crash;
update any callers or the deepnote_run_block contract to expect this handled
error shape if needed.
Comment on lines
+457
to
+460
| // Find the block | ||
| let targetBlock = null | ||
| let targetNotebook = null | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Type targetBlock/targetNotebook explicitly (avoid implicit any).
let x = null infers any under TS, which undermines strict typing.
✅ Suggested fix
// Find the block
- let targetBlock = null
- let targetNotebook = null
+ let targetNotebook: DeepnoteFile['project']['notebooks'][number] | null = null
+ let targetBlock: DeepnoteFile['project']['notebooks'][number]['blocks'][number] | null = nullAs per coding guidelines: Use strict type checking in TypeScript files; Avoid any types - use proper type definitions.
🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/execution.ts` around lines 457 - 460, The variables
targetBlock and targetNotebook are declared as "let ... = null" which yields
implicit any; change their declarations to explicit nullable types (e.g., the
concrete Block and Notebook types used in this module) — replace the implicit
any with the proper type unions (Block | null and Notebook | null or the exact
names used elsewhere in this file) and add any needed imports for those types so
strictTypeChecking passes; update the declarations for targetBlock and
targetNotebook in execution.ts accordingly.
Comment on lines
+559
to
+561
| // Read file contents | ||
| const fileBuffer = await fs.readFile(absolutePath) | ||
|
|
Contributor
There was a problem hiding this comment.
Handle read failures after stat in deepnote_open.
The file can disappear between stat and readFile, which currently throws out of the handler.
🩹 Suggested fix
// Read file contents
- const fileBuffer = await fs.readFile(absolutePath)
+ let fileBuffer: Buffer
+ try {
+ fileBuffer = await fs.readFile(absolutePath)
+ } catch (error) {
+ const message = error instanceof Error ? error.message : String(error)
+ return {
+ content: [{ type: 'text', text: JSON.stringify({ error: `Cannot read file: ${message}` }) }],
+ isError: true,
+ }
+ }🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/execution.ts` around lines 559 - 561, In
deepnote_open, guard the fs.readFile(absolutePath) call so a file removed
between stat and read doesn't throw: wrap the read in a try/catch (or a Promise
.catch) around the fs.readFile call that produces fileBuffer and, on error,
return the proper error to the caller/handler (e.g., call the open callback with
an ENOENT/EIO appropriate errno) and log the failure instead of letting the
exception escape; update any code paths that use fileBuffer to only proceed
after successful read.
- Replace __dirname with import.meta.url for ESM compatibility in resources.ts - Fix examples resource listing which was crashing with "__dirname is not defined" - Add try-catch for JSON.parse in workflow results to handle non-JSON outputs - Fix workflow tool crashing when using deepnote_explain (which returns markdown)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/mcp/src/tools/magic.ts`:
- Around line 2489-2523: The switch on step.tool in this routing block only maps
scaffold/enhance/fix/explain/suggest/template/refactor/profile/test to handler
functions (handleScaffold, handleEnhance, handleFix, etc.), but the
schema/description advertises extra tools like convert_to and lint; update the
routing to match the schema by adding cases for the missing tools (e.g., case
'convert_to': result = await handleConvertTo(toolArgs); case 'lint': result =
await handleLint(toolArgs);), ensuring corresponding handler functions
(handleConvertTo, handleLint) exist and follow the same result shape, or
alternatively remove/adjust convert_to and lint from the schema/description so
they are not advertised; keep the default Unknown tool branch as a fallback.
- Around line 1454-1459: The visualization-detection predicate incorrectly
scopes only plt. to code blocks and treats sns. in any block as a visualization;
update the predicate used in the notebook.blocks.some(...) check so both checks
are constrained to code blocks (i.e., change the arrow function for
notebook.blocks.some to require b.type === 'code' and then test
b.content?.includes('plt.') || b.content?.includes('sns.')). This touches the
block that checks block.content for '.describe()' or '.value_counts()' and the
notebook.blocks.some(...) predicate in packages/mcp/src/tools/magic.ts.
Comment on lines
+2489
to
+2523
| // Route to the appropriate handler | ||
| switch (step.tool) { | ||
| case 'scaffold': | ||
| result = await handleScaffold(toolArgs) | ||
| break | ||
| case 'enhance': | ||
| result = await handleEnhance(toolArgs) | ||
| break | ||
| case 'fix': | ||
| result = await handleFix(toolArgs) | ||
| break | ||
| case 'explain': | ||
| result = await handleExplain(toolArgs) | ||
| break | ||
| case 'suggest': | ||
| result = await handleSuggest(toolArgs) | ||
| break | ||
| case 'template': | ||
| result = await handleTemplate(toolArgs) | ||
| break | ||
| case 'refactor': | ||
| result = await handleRefactor(toolArgs) | ||
| break | ||
| case 'profile': | ||
| result = await handleProfile(toolArgs) | ||
| break | ||
| case 'test': | ||
| result = await handleTest(toolArgs) | ||
| break | ||
| default: | ||
| result = { | ||
| content: [{ type: 'text', text: `Unknown tool: ${step.tool}` }], | ||
| isError: true, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Workflow advertises tools it doesn’t route.
The schema text lists tools like convert_to and lint, but the switch only handles scaffold/enhance/fix/explain/suggest/template/refactor/profile/test. Following the docs will yield “Unknown tool.” Align the schema/description with supported tools or route to the other toolsets.
🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/magic.ts` around lines 2489 - 2523, The switch on
step.tool in this routing block only maps
scaffold/enhance/fix/explain/suggest/template/refactor/profile/test to handler
functions (handleScaffold, handleEnhance, handleFix, etc.), but the
schema/description advertises extra tools like convert_to and lint; update the
routing to match the schema by adding cases for the missing tools (e.g., case
'convert_to': result = await handleConvertTo(toolArgs); case 'lint': result =
await handleLint(toolArgs);), ensuring corresponding handler functions
(handleConvertTo, handleLint) exist and follow the same result shape, or
alternatively remove/adjust convert_to and lint from the schema/description so
they are not advertised; keep the default Unknown tool branch as a fallback.
- Log directory scan errors to stderr for debugging - Log snapshot save failures to stderr for debugging
…ar deps, and refactor enhance tool: - Now actually applies input blocks (input-slider) for hardcoded values like test_size, n_estimators, threshold, learning_rate, max_depth - Modifies code to reference the input variable instead of hardcoded value - Adds section headers (h1, h2) when structure enhancement is applied fix tool: - Implements circular dependency detection using DAG analysis - Attempts to break cycles by reordering blocks (moving high-output blocks earlier) - Reports detected cycles with the variables involved refactor tool: - Now removes extracted functions/classes from source blocks - Adds import statements to source notebooks (from <module> import <names>) - Reports sourceModified and importAdded in response Updated tool descriptions to accurately reflect new capabilities.
…ar deps, and refactor (#258) enhance tool: - Now actually applies input blocks (input-slider) for hardcoded values like test_size, n_estimators, threshold, learning_rate, max_depth - Modifies code to reference the input variable instead of hardcoded value - Adds section headers (h1, h2) when structure enhancement is applied fix tool: - Implements circular dependency detection using DAG analysis - Attempts to break cycles by reordering blocks (moving high-output blocks earlier) - Reports detected cycles with the variables involved refactor tool: - Now removes extracted functions/classes from source blocks - Adds import statements to source notebooks (from <module> import <names>) - Reports sourceModified and importAdded in response Updated tool descriptions to accurately reflect new capabilities.
- Add Structure section explaining block → notebook → project hierarchy - Add execution scope guidance: deepnote_run_block → run notebook → run project - Add incremental development workflow for complex/custom logic - Update Quick Start to recommend incremental approach - Add execution scoping tips to Best Practices
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/mcp/src/tools/execution.ts`:
- Around line 300-373: The notebook filter accepts name or ID but you pass the
raw notebookFilter into runProject and the dry-run metadata, causing failures
when an ID is used; resolve the notebook by finding it (the existing const
found) and pass found.name into ExecutionEngine.runProject's notebookName option
and use found.name for the dry-run "level: 'notebook'" metadata and notebooks
list instead of notebookFilter; update references around notebookFilter, found,
runProject, and the dry-run JSON creation so all notebook names are the resolved
names.
Comment on lines
+300
to
+373
| // Filter notebooks if specified, otherwise run all | ||
| let notebooks = file.project.notebooks | ||
| if (notebookFilter) { | ||
| const found = file.project.notebooks.find(n => n.name === notebookFilter || n.id === notebookFilter) | ||
| if (!found) { | ||
| return { | ||
| content: [{ type: 'text', text: `Notebook not found: ${notebookFilter}` }], | ||
| isError: true, | ||
| } | ||
| } | ||
| notebooks = [found] | ||
| } | ||
|
|
||
| // Collect all executable blocks from target notebooks | ||
| const executableBlocks: Array<{ notebook: string; block: { id: string; type: string; content?: string } }> = [] | ||
| for (const notebook of notebooks) { | ||
| for (const block of notebook.blocks) { | ||
| if (executableBlockTypeSet.has(block.type)) { | ||
| executableBlocks.push({ | ||
| notebook: notebook.name, | ||
| block: { id: block.id, type: block.type, content: block.content }, | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (dryRun) { | ||
| // Show execution plan | ||
| return { | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| text: JSON.stringify( | ||
| { | ||
| dryRun: true, | ||
| level: notebookFilter ? 'notebook' : 'project', | ||
| notebooks: notebooks.map(n => n.name), | ||
| blocksToExecute: executableBlocks.length, | ||
| executionOrder: executableBlocks.map(b => ({ | ||
| notebook: b.notebook, | ||
| id: b.block.id.slice(0, 8), | ||
| type: b.block.type, | ||
| contentPreview: b.block.content?.slice(0, 50) || '', | ||
| })), | ||
| inputs: inputs || {}, | ||
| }, | ||
| null, | ||
| 2 | ||
| ), | ||
| }, | ||
| ], | ||
| } | ||
| } | ||
|
|
||
| // Actually run the notebooks | ||
| const workingDir = path.dirname(originalPath) | ||
| const engine = new ExecutionEngine({ | ||
| pythonEnv: pythonPath || 'python', | ||
| workingDirectory: workingDir, | ||
| }) | ||
|
|
||
| const results: Array<{ notebook: string; blockId: string; type: string; success: boolean; error?: string }> = [] | ||
| const blockOutputs: Array<{ id: string; outputs: unknown[]; executionCount?: number | null }> = [] | ||
|
|
||
| // Track execution timing | ||
| const executionStartedAt = new Date().toISOString() | ||
|
|
||
| try { | ||
| await engine.start() | ||
|
|
||
| const summary = await engine.runProject(file, { | ||
| notebookName: notebookFilter, | ||
| inputs, | ||
| onBlockDone: result => { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find ExecutionEngine.runProject signature and its usage
rg -n "runProject\(" -g '*.ts' packages -A 3 -B 2Repository: deepnote/deepnote
Length of output: 27812
🏁 Script executed:
#!/bin/bash
# Find ExecutionEngine class definition and runProject method
rg -n "class ExecutionEngine" -g '*.ts' packages -A 50 | head -100Repository: deepnote/deepnote
Length of output: 3845
🏁 Script executed:
#!/bin/bash
# Search for runProject method definition
rg -n "runProject\s*\(" -g '*.ts' packages -B 2 -A 10Repository: deepnote/deepnote
Length of output: 49414
🏁 Script executed:
#!/bin/bash
# Get more context on runProject implementation
sed -n '126,200p' packages/runtime-core/src/execution-engine.tsRepository: deepnote/deepnote
Length of output: 2716
🏁 Script executed:
#!/bin/bash
# Find ExecutionOptions type definition
rg -n "type ExecutionOptions\|interface ExecutionOptions" -g '*.ts' packages -A 10Repository: deepnote/deepnote
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for ExecutionOptions definition
rg -n "ExecutionOptions" -g '*.ts' packages/runtime-core/src -B 2 -A 8 | head -80Repository: deepnote/deepnote
Length of output: 3916
Pass the resolved notebook name to runProject, not the filter value.
The filter accepts notebook name or ID (line 303), but runProject only accepts names. When an ID is passed, execution fails. Use found.name for both the dry-run metadata and the notebookName parameter.
Proposed fix
- const notebookFilter = args.notebook as string | undefined
+ const notebookFilter = args.notebook as string | undefined
+ let notebookName: string | undefined
// Filter notebooks if specified, otherwise run all
let notebooks = file.project.notebooks
if (notebookFilter) {
const found = file.project.notebooks.find(n => n.name === notebookFilter || n.id === notebookFilter)
if (!found) {
return {
content: [{ type: 'text', text: `Notebook not found: ${notebookFilter}` }],
isError: true,
}
}
notebooks = [found]
+ notebookName = found.name
}
@@
- level: notebookFilter ? 'notebook' : 'project',
+ level: notebookName ? 'notebook' : 'project',
@@
- const summary = await engine.runProject(file, {
- notebookName: notebookFilter,
+ const summary = await engine.runProject(file, {
+ notebookName,
inputs,
onBlockDone: result => {🤖 Prompt for AI Agents
In `@packages/mcp/src/tools/execution.ts` around lines 300 - 373, The notebook
filter accepts name or ID but you pass the raw notebookFilter into runProject
and the dry-run metadata, causing failures when an ID is used; resolve the
notebook by finding it (the existing const found) and pass found.name into
ExecutionEngine.runProject's notebookName option and use found.name for the
dry-run "level: 'notebook'" metadata and notebooks list instead of
notebookFilter; update references around notebookFilter, found, runProject, and
the dry-run JSON creation so all notebook names are the resolved names.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.