Docs/readme screenshots - #8
Open
UsamaK98 wants to merge 3 commits into
Open
Conversation
## New Feature Set ### Architecture - Restructure server.py into modular notebook_mcp/ package - notebook_mcp/config.py: global workspace state - notebook_mcp/security.py: path jail (CWE-22 protection) - notebook_mcp/core/: notebook I/O, cell operations, search - notebook_mcp/execution/: kernel lifecycle, cell executor - notebook_mcp/visualization/: detector, describer, image handler, viz server - notebook_mcp/intelligence/: summarizer, pipeline analyser, error/data inspector - Add pyproject.toml with optional dependency groups ### New Tools (34 added, 9 original retained) **Cell Management (14 tools)** - delete_cell, move_cell, duplicate_cell, change_cell_type - split_cell, merge_cells, bulk_add_cells - clear_cell_outputs, clear_all_outputs - notebook_search (keyword search with context snippets) - notebook_outline (headings + functions + classes) - notebook_info, notebook_validate, notebook_export **Workspace (4 tools)** - rename_notebook, delete_notebook (new) - initialize_workspace, list_notebooks (retained) **Execution Engine (8 tools, requires jupyter_client)** - execute_cell, execute_range, execute_all - restart_kernel, interrupt_kernel, get_kernel_status - inspect_variable, list_kernel_variables **Visualization Intelligence (5 tools)** - describe_visualization: structured text description of plots (chart type, axes, title, data variables, library) - scan_visualizations: find all viz cells in a notebook - get_visualization_image: return base64 image for vision LLMs - open_visualization: on-demand browser window (stdlib HTTP server) - open_gallery: gallery of all plots in a notebook **Notebook Intelligence (4 tools)** - summarize_notebook (brief/standard/detailed) - get_notebook_pipeline (static data-flow DAG) - analyze_cell_error (traceback analysis + fix hints) - inspect_dataframe_output (parse HTML table output) ### Tests - 87 tests across 5 test files (86 pass, 1 skip on Windows) - test_cell_ops.py, test_search.py, test_visualization.py - test_intelligence.py, test_security.py
…eature highlights - tool_overview.png: visual map of all 43 tools across 6 categories - architecture.png: module structure diagram (server.py -> notebook_mcp/) - viz_intelligence.png: shows how describe_visualization works - error_analysis.png: shows analyze_cell_error diagnostic flow - pipeline_analysis.png: shows data flow DAG across notebook cells - README updated to embed all 5 images at strategic positions
📝 WalkthroughWalkthroughRestructures a single-file Jupyter notebook MCP server into a ChangesNotebook MCP v2.0 Refactor and 43-Tool Expansion
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant Server as server.py (FastMCP)
participant Kernel as execution/kernel.py
participant Executor as execution/executor.py
participant IO as core/notebook_io.py
rect rgba(70, 130, 180, 0.5)
note over Client,IO: Execution flow
Client->>Server: execute_cell(filepath, cell_index)
Server->>Kernel: get_or_start_kernel(filepath)
Kernel-->>Server: (KernelManager, KernelClient)
Server->>Executor: execute_cell(filepath, cell_index, ...)
Executor->>Kernel: kc.execute(source) → msg_id
Executor->>Kernel: _capture_outputs(kc, msg_id, timeout)
Kernel-->>Executor: raw_outputs + timed_out/error flags
Executor->>IO: write_notebook_atomic(nb, path)
Executor-->>Server: {outputs, execution_count, duration, ...}
Server-->>Client: structured result
end
sequenceDiagram
participant Client as MCP Client
participant Server as server.py (FastMCP)
participant VizDet as visualization/detector.py
participant VizDesc as visualization/describer.py
participant ImgH as visualization/image_handler.py
participant VizSrv as visualization/viz_server.py
rect rgba(60, 179, 113, 0.5)
note over Client,VizSrv: Visualization open flow
Client->>Server: open_visualization(filepath, cell_index)
Server->>VizDesc: describe_cell_visualization(cell, idx)
VizDesc->>VizDet: detect_library / detect_chart_type / has_visual_output
VizDet-->>VizDesc: library, chart_type, rendered flag
VizDesc-->>Server: PlotDescription
Server->>VizDet: get_html_output(cell)
alt HTML output found
VizDet-->>Server: html_content
else fallback to image
Server->>ImgH: get_cell_image(cell)
ImgH-->>Server: {mime, data, ...}
end
Server->>VizSrv: cache_and_open_visualization(filepath, idx, html)
VizSrv->>VizSrv: ensure_server_running() → base_url
VizSrv-->>Server: http://127.0.0.1:PORT/viz/{key}
Server-->>Client: {url, description, ...}
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
notebook_mcp/core/search.py-23-23 (1)
23-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
context_linesto prevent invalid snippet windows.If
context_linesis negative, the Line 47–48 window math can invert/empty the snippet slice and produce inconsistent output. Clamp or reject negatives at input.Proposed fix
def notebook_search( filepath: str, query: str, case_sensitive: bool = False, context_lines: int = 2, ) -> List[Dict[str, Any]]: @@ + if context_lines < 0: + raise ValueError("context_lines must be >= 0") + nb = get_notebook_content(filepath)Also applies to: 47-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notebook_mcp/core/search.py` at line 23, The context_lines parameter accepts negative values which cause invalid window calculations in the snippet slicing logic at lines 47-48, resulting in inverted or empty slices. Add input validation for the context_lines parameter early in the function to either reject negative values (raise a ValueError) or clamp them to a minimum of 0 to ensure the window math always produces consistent and correct snippet windows.notebook_mcp/core/search.py-157-158 (1)
157-158:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLanguage detection misses common notebook metadata.
Line 158 reads
metadata.kernelspec.language, but many notebooks only populatemetadata.language_info.name. This can return"unknown"for valid notebooks.Proposed fix
- language = nb.metadata.get("kernelspec", {}).get("language", "unknown") + language = ( + nb.metadata.get("kernelspec", {}).get("language") + or nb.metadata.get("language_info", {}).get("name") + or "unknown" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notebook_mcp/core/search.py` around lines 157 - 158, The language detection on line 158 only checks metadata.kernelspec.language but many notebooks store the language information in metadata.language_info.name instead. Modify the language assignment to first attempt to get the language from kernelspec.language, then fall back to checking language_info.name if the first option is unavailable, and finally use "unknown" as the default if neither location contains a value.tests/test_security.py-57-59 (1)
57-59:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNarrow expected exception type to enforce the API contract
Line 58 currently allows
TypeErrorandAttributeError, butresolve_path()explicitly validates and raisesValueErrorfor non-string input. Keeping the test broad can mask a regression in input handling.Suggested test tightening
- def test_none_raises(self): - with self.assertRaises((ValueError, TypeError, AttributeError)): - resolve_path(None) + def test_none_raises(self): + with self.assertRaises(ValueError): + resolve_path(None)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_security.py` around lines 57 - 59, The test_none_raises method in the test class is catching too broad a set of exceptions. The resolve_path() function explicitly validates input and raises ValueError for non-string input, so change the assertRaises call to expect only ValueError instead of the tuple of ValueError, TypeError, and AttributeError. This will ensure the test enforces the actual API contract and catches any regressions in the input validation behavior.notebook_mcp/core/cell_ops.py-186-190 (1)
186-190:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace en dash with hyphen-minus in error message.
Line 188 uses an en dash (
–) instead of a standard hyphen-minus (-).Proposed fix
- f"index {index} out of range (0–{len(nb.cells)} inclusive)." + f"index {index} out of range (0-{len(nb.cells)} inclusive)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notebook_mcp/core/cell_ops.py` around lines 186 - 190, In the IndexError exception within the range validation check (where index is compared against len(nb.cells)), the error message contains an en dash character between the numbers in the range description. Replace the en dash with a standard hyphen-minus character to maintain consistency with standard error message formatting.Source: Linters/SAST tools
notebook_mcp/core/cell_ops.py-65-82 (1)
65-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace en dashes with hyphen-minus in error messages.
Lines 72 and 74 use en dashes (
–) instead of standard hyphen-minus (-).Proposed fix
- raise IndexError(f"from_index {from_index} out of range (0–{n-1}).") + raise IndexError(f"from_index {from_index} out of range (0-{n-1}).") - raise IndexError(f"to_index {to_index} out of range (0–{n-1}).") + raise IndexError(f"to_index {to_index} out of range (0-{n-1}).")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notebook_mcp/core/cell_ops.py` around lines 65 - 82, In the move_cell function, the error messages on lines 72 and 74 contain en dashes (–) in the range notation instead of standard hyphen-minus (-) characters. Replace the en dashes with regular hyphens in both IndexError messages where the range is displayed as "0–{n-1}" to ensure consistent formatting and proper character encoding.Source: Linters/SAST tools
notebook_mcp/core/cell_ops.py-25-48 (1)
25-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace en dash with hyphen-minus in error messages.
Line 34 uses an en dash (
–U+2013) instead of a standard hyphen-minus (-U+002D). This can cause display issues in terminals or logs with limited Unicode support.Proposed fix
- f"(valid indices: 0–{upper})." + f"(valid indices: 0-{upper})."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notebook_mcp/core/cell_ops.py` around lines 25 - 48, The error message in the _validate_index function contains a Unicode en dash character (–) in the valid indices range display that should be replaced with a standard hyphen-minus character (-). Locate the IndexError message in _validate_index where it displays the valid cell indices range and replace the en dash in "0–{upper}" with a regular hyphen-minus to ensure proper display in terminals and logs with limited Unicode support.Source: Linters/SAST tools
notebook_mcp/visualization/describer.py-166-167 (1)
166-167:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRename ambiguous loop variable at Line 166 to satisfy lint and readability.
lis flagged by Ruff E741 and is hard to distinguish visually.Suggested fix
- lines = [l for l in source.splitlines() if l.strip() and not l.strip().startswith("#")] + lines = [line for line in source.splitlines() if line.strip() and not line.strip().startswith("#")]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notebook_mcp/visualization/describer.py` around lines 166 - 167, The loop variable `l` in the list comprehension on line 166 is ambiguous and violates Ruff E741 linting rule (ambiguous variable name). Rename the loop variable `l` to a more readable name like `line` throughout the list comprehension where it appears three times - in the iteration `for l in source.splitlines()`, in the condition `if l.strip()`, and in the condition `not l.strip().startswith("#")`. This will improve code readability and satisfy the linter.Source: Linters/SAST tools
notebook_mcp/visualization/image_handler.py-102-107 (1)
102-107:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse decoded byte size for the KB limit check.
At Line 103–Line 104,
len(result_data)measures base64 characters, not bytes. This skewsmax_size_kbenforcement and can trigger unnecessary downscaling.Suggested fix
- max_chars = max_size_kb * 1024 - if len(result_data) > max_chars and mime == "image/png": + max_bytes = max_size_kb * 1024 + result_bytes = len(base64.b64decode(result_data)) + if result_bytes > max_bytes and mime == "image/png": # Aggressively resize result_data = _try_resize_png(raw, max_width // 2, max_height // 2) resized = True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notebook_mcp/visualization/image_handler.py` around lines 102 - 107, The size check using len(result_data) measures base64-encoded characters rather than actual bytes, causing incorrect enforcement of the max_size_kb limit. Modify the condition that checks if len(result_data) > max_chars to instead use the decoded byte size of the result_data. Decode the base64-encoded result_data to get the actual bytes and compare that length against max_chars to properly enforce the KB size limit before deciding to aggressively resize the PNG.server.py-969-972 (1)
969-972:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTool count mismatch in startup log.
The log claims
notebook_crud(8)but there are actually 7 Notebook CRUD tools:create_notebook,read_notebook,read_cell,edit_cell,add_cell,read_notebook_outputs,read_cell_output. Total is 42 tools, not 43.The module docstring (lines 6-12) also has inconsistent counts. Consider auditing and synchronizing both locations.
Suggested fix
log.info( - "Tools available: workspace(4) + notebook_crud(8) + cell_mgmt(14) + " - "execution(8) + visualization(5) + intelligence(4) = 43 total" + "Tools available: workspace(4) + notebook_crud(7) + cell_mgmt(14) + " + "execution(8) + visualization(5) + intelligence(4) = 42 total" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.py` around lines 969 - 972, The startup log message in the info call claims notebook_crud has 8 tools and a total of 43 tools, but the actual count of Notebook CRUD tools is 7 (create_notebook, read_notebook, read_cell, edit_cell, add_cell, read_notebook_outputs, read_cell_output), making the correct total 42. Update the log message to change notebook_crud from 8 to 7 and the total from 43 to 42. Additionally, review the module docstring in lines 6-12 and ensure it reflects the same corrected tool counts to maintain consistency across the codebase.notebook_mcp/intelligence/data_inspector.py-34-37 (1)
34-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
ModuleNotFoundErrorisn’t matched by the import-error pattern.On Line 35,
(?:Import|Module)Errorwon’t matchModuleNotFoundError, so a very common dependency error can miss the tailored hint.💡 Suggested fix
- "pattern": r"(?:Import|Module)Error.*'([^']+)'", + "pattern": r"(?:ImportError|ModuleNotFoundError).*['\"]([^'\"]+)['\"]",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notebook_mcp/intelligence/data_inspector.py` around lines 34 - 37, The regex pattern in the "ImportError" key's pattern field does not match ModuleNotFoundError, which is a common Python error that inherits from ImportError. Update the pattern string to include ModuleNotFoundError in the alternation by modifying the pattern to also match the ModuleNotFoundError exception type alongside ImportError, ensuring that common dependency errors receive the tailored hint message.notebook_mcp/intelligence/summarizer.py-182-186 (1)
182-186:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
libraries_useddrops allfrom ... import ...libraries.On Line 185, filtering out
fromimports makesoperations.libraries_usedincomplete for many notebooks (e.g.,from sklearn.model_selection import ...), so the summary can report no library even when one is clearly used.💡 Suggested fix
- "operations": { - "libraries_used": [ - imp.replace("import ", "").replace("from ", "").split(" ")[0] - for imp in imports[:20] - if not imp.startswith("from") - ], + "operations": { + "libraries_used": list(dict.fromkeys( + imp.split()[1].split(".")[0] + for imp in imports[:20] + if imp.startswith(("import ", "from ")) + )), "functions_defined": functions[:10], "classes_defined": classes[:10], },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notebook_mcp/intelligence/summarizer.py` around lines 182 - 186, The `libraries_used` list comprehension is filtering out all imports that start with "from" using the condition `if not imp.startswith("from")`, which causes it to miss libraries imported via from-import statements like `from sklearn.model_selection import ...`. Instead of filtering out from-imports entirely, modify the logic to properly extract the library name from both import types: for "from" imports, extract the package name that appears after "from" and before "import", and for regular imports, continue extracting the name after "import". This will ensure the libraries_used list includes all imported libraries regardless of import style.
🧹 Nitpick comments (5)
tests/test_cell_ops.py (1)
214-218: ⚡ Quick winAlso assert
execution_countreset in clear-all test
clear_all_outputs()resets bothoutputsandexecution_count; this test currently checks only outputs. Add the second assertion to lock the full contract.Suggested assertion addition
nb2 = get_notebook_content(path) for cell in nb2.cells: assert cell.outputs == [] + assert cell.execution_count is None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cell_ops.py` around lines 214 - 218, The test for clear_all_outputs() function only verifies that cell outputs are cleared but does not check that execution_count is also reset. Add an additional assertion within the for loop iterating through nb2.cells to verify that the execution_count attribute is also reset to None for each cell, ensuring the test fully validates the contract of the clear_all_outputs() function.tests/test_security.py (1)
40-43: ⚡ Quick winAvoid swallowing all teardown errors
Line 42 catches
Exceptionand Line 43 suppresses it. That can hide real cleanup issues and make CI behavior harder to diagnose.Suggested cleanup handling
def tearDown(self): - try: - shutil.rmtree(self._scratch, ignore_errors=True) - except Exception: - pass + shutil.rmtree(self._scratch, ignore_errors=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_security.py` around lines 40 - 43, The teardown code is catching all exceptions and silently suppressing them with a bare pass statement in the except block after shutil.rmtree, which hides real cleanup issues and makes debugging difficult. Modify the exception handling to either remove the overly broad exception handling entirely or, if exception handling is necessary, log the error details so that cleanup failures can be properly diagnosed instead of being silently swallowed.Source: Linters/SAST tools
notebook_mcp/execution/kernel.py (2)
154-154: ⚡ Quick winPrefix unused variable with underscore.
The
kcvariable is unpacked but never used. Use_or_kcto indicate it's intentionally unused.♻️ Proposed fix
- km, kc = _kernels[key] + km, _ = _kernels[key]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notebook_mcp/execution/kernel.py` at line 154, In the unpacking statement where `km, kc = _kernels[key]` is assigned, the variable `kc` is captured but never used in the subsequent code. Replace `kc` with `_kc` (or simply `_` if it's more concise) to explicitly indicate that this variable is intentionally unused and suppress any linting warnings about unused variables. This clarifies your intent to readers and follows Python naming conventions for intentionally ignored values.Source: Linters/SAST tools
26-31: ⚡ Quick winChain the exception with
from Noneto suppress the original traceback.Using
raise ... from Noneexplicitly indicates that the original exception context is intentionally suppressed, producing a cleaner traceback for users.♻️ Proposed fix
except ImportError: - raise ImportError( + raise ImportError( "jupyter_client is not installed. Cell execution requires it.\n" "Install with: pip install jupyter_client ipykernel\n" "Or: uv pip install jupyter_client ipykernel" - ) + ) from None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notebook_mcp/execution/kernel.py` around lines 26 - 31, In the except ImportError block, add `from None` at the end of the raise ImportError statement to suppress the original traceback and provide a cleaner error message to users. This explicit exception chaining will prevent Python from automatically displaying the original exception context, which helps make the error output more user-friendly and focused on the helpful installation instructions.Source: Linters/SAST tools
server.py (1)
850-875: ⚡ Quick winDirect access to private
viz_serverinternals breaks encapsulation.
open_galleryaccesses private membersviz_server._make_key,viz_server._viz_cache, andviz_server._render_gallery_page. This tightly couples the server to the internal implementation ofviz_server, making future refactoring difficult.Consider exposing a public API in
viz_server(e.g.,build_gallery_page(...)andcache_visualization(...)) that encapsulates these operations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.py` around lines 850 - 875, The code in the gallery generation loop directly accesses private members of viz_server (_make_key, _viz_cache, and _render_gallery_page), violating encapsulation principles. Create public methods in the viz_server class to encapsulate these operations, such as a public method to generate and cache gallery pages (e.g., build_gallery_page or similar) and a public method to cache visualizations. Then replace all direct accesses to the private members in the open_gallery function with calls to these new public methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@notebook_mcp/core/search.py`:
- Around line 103-104: The code at line 103 uses ast.walk(tree) to find function
and class definitions, but ast.walk traverses the entire AST including nested
definitions, which violates the documented top-level only contract stated in the
docstring at line 73. Replace ast.walk(tree) with direct iteration over
tree.body to ensure only top-level definitions (those at the root level of the
module) are included in the outline, preventing over-reporting of nested
functions and classes.
In `@notebook_mcp/execution/executor.py`:
- Around line 69-71: The image data truncation logic in the mime type checking
loop is slicing base64-encoded image data at an arbitrary boundary without
considering base64 encoding structure (4-character groups), which corrupts the
image. Replace the direct slicing at MAX_OUTPUT_SIZE with one of the following
approaches: remove truncation for image mime types entirely, calculate a valid
base64 boundary by adjusting the slice index to be divisible by 4, or omit the
image data with a placeholder message when it exceeds MAX_OUTPUT_SIZE. Update
the conditional block that checks for "image/png" and "image/jpeg" and truncates
the data accordingly.
In `@notebook_mcp/intelligence/data_inspector.py`:
- Around line 173-201: The function initializes result with {"cell_index":
cell_index} at the start, which contradicts the docstring promise to return an
empty dict when no DataFrame metadata is found. Since the cell_index is always
added regardless of whether any actual metadata is extracted, the function never
returns a truly empty dict. Change the initialization to start with an empty
result dict, then only add the cell_index key to result after confirming that at
least one piece of DataFrame metadata (columns, visible_rows, or shape) was
successfully extracted from the cell outputs.
In `@notebook_mcp/intelligence/pipeline.py`:
- Around line 109-111: The imports classification logic in the condition around
line 109 is flawed because the empty string "" in the startswith() tuple matches
every non-empty line, causing incorrect classification of non-import cells.
Remove the empty string "" from the tuple passed to startswith() so that the
condition only validates lines that actually begin with "import ", "from ", or
"#" (comments), allowing proper cell role classification.
In `@notebook_mcp/security.py`:
- Around line 54-81: The resolve_path function only performs the workspace
containment check when config.is_initialized() is true, which means
uninitialized workspaces can return paths outside the workspace boundary without
validation. To fix this, move the containment validation logic that uses
os.path.commonpath and normcase comparisons outside of the
config.is_initialized() conditional block so that the path containment check is
always enforced regardless of initialization status, ensuring the function fails
securely.
In `@notebook_mcp/visualization/viz_server.py`:
- Around line 210-212: The ensure_server_running function returns the caller's
requested port parameter instead of the actual bound port when a fallback port
is used. Track the actual bound port in a module-level or instance variable
whenever a server is successfully bound (in the fallback binding logic around
lines 217-221). Then modify the return statements in ensure_server_running at
lines 210-212 and 229-230 to return the actual bound port from this tracked
variable instead of the port parameter passed to the function.
- Line 29: The _viz_cache dictionary has no size limits and will grow
indefinitely with each visualization stored, consuming memory in long-lived
sessions. Replace the unbounded dictionary implementation with a bounded cache
approach such as Python's functools.lru_cache decorator or
collections.OrderedDict with manual size management. Identify all locations
where cache entries are added (around lines 254-255 and 273-274 in addition to
the _viz_cache initialization at line 29) and implement an eviction policy that
removes the oldest entries when the cache exceeds a reasonable size limit (e.g.,
maximum number of cached visualizations or total memory threshold). This will
ensure the cache remains bounded while still providing caching benefits for
recent visualizations.
- Around line 44-50: The _render_image_page function injects raw SVG data
directly into the HTML response at line 47 without any sanitization or escaping,
creating an XSS vulnerability. Apply html.escape() to the data parameter when
creating the SVG container div element, similar to how the title is escaped in
the img tag. Additionally, review and apply the same sanitization to similar
SVG/HTML injection points mentioned around lines 83-107 to prevent malicious
scripts from executing when notebooks with crafted SVG output are opened in the
browser.
In `@pyproject.toml`:
- Around line 60-66: The console entrypoint defined in [project.scripts] as
notebook-mcp = "server:main" references the root-level server.py module, but the
setuptools package discovery configuration only includes packages matching the
pattern notebook_mcp*, which excludes the server module. Add a py-modules
configuration option in the [tool.setuptools] section and set it to ["server"]
to explicitly include the server.py module in the distribution so that the
console script entrypoint can be resolved when the package is installed.
- Around line 48-50: The `full` extra in the pyproject.toml file contains a
self-referential dependency on `python-notebook-mcp[execution,imaging,export]`,
which creates resolver complexity. Replace this self-reference with explicit
concrete dependencies by identifying all the individual packages listed in the
execution, imaging, and export extras sections and including them directly in
the `full` extra instead of referencing the extras by name.
In `@README.md`:
- Line 8: The badge in the README.md displays "Tools-43" but the actual
implementation in server.py only contains 42 MCP tools. Verify the tool count in
server.py by reviewing the implementations in categories: Workspace & Navigation
(4), Notebook CRUD (7), Cell Management (14), Execution Engine (8),
Visualization Intelligence (5), and Notebook Intelligence (4), which totals 42.
Then update the badge to display "Tools-42" to match the actual tool count, or
alternatively implement a 43rd tool to match the badge claim.
---
Minor comments:
In `@notebook_mcp/core/cell_ops.py`:
- Around line 186-190: In the IndexError exception within the range validation
check (where index is compared against len(nb.cells)), the error message
contains an en dash character between the numbers in the range description.
Replace the en dash with a standard hyphen-minus character to maintain
consistency with standard error message formatting.
- Around line 65-82: In the move_cell function, the error messages on lines 72
and 74 contain en dashes (–) in the range notation instead of standard
hyphen-minus (-) characters. Replace the en dashes with regular hyphens in both
IndexError messages where the range is displayed as "0–{n-1}" to ensure
consistent formatting and proper character encoding.
- Around line 25-48: The error message in the _validate_index function contains
a Unicode en dash character (–) in the valid indices range display that should
be replaced with a standard hyphen-minus character (-). Locate the IndexError
message in _validate_index where it displays the valid cell indices range and
replace the en dash in "0–{upper}" with a regular hyphen-minus to ensure proper
display in terminals and logs with limited Unicode support.
In `@notebook_mcp/core/search.py`:
- Line 23: The context_lines parameter accepts negative values which cause
invalid window calculations in the snippet slicing logic at lines 47-48,
resulting in inverted or empty slices. Add input validation for the
context_lines parameter early in the function to either reject negative values
(raise a ValueError) or clamp them to a minimum of 0 to ensure the window math
always produces consistent and correct snippet windows.
- Around line 157-158: The language detection on line 158 only checks
metadata.kernelspec.language but many notebooks store the language information
in metadata.language_info.name instead. Modify the language assignment to first
attempt to get the language from kernelspec.language, then fall back to checking
language_info.name if the first option is unavailable, and finally use "unknown"
as the default if neither location contains a value.
In `@notebook_mcp/intelligence/data_inspector.py`:
- Around line 34-37: The regex pattern in the "ImportError" key's pattern field
does not match ModuleNotFoundError, which is a common Python error that inherits
from ImportError. Update the pattern string to include ModuleNotFoundError in
the alternation by modifying the pattern to also match the ModuleNotFoundError
exception type alongside ImportError, ensuring that common dependency errors
receive the tailored hint message.
In `@notebook_mcp/intelligence/summarizer.py`:
- Around line 182-186: The `libraries_used` list comprehension is filtering out
all imports that start with "from" using the condition `if not
imp.startswith("from")`, which causes it to miss libraries imported via
from-import statements like `from sklearn.model_selection import ...`. Instead
of filtering out from-imports entirely, modify the logic to properly extract the
library name from both import types: for "from" imports, extract the package
name that appears after "from" and before "import", and for regular imports,
continue extracting the name after "import". This will ensure the libraries_used
list includes all imported libraries regardless of import style.
In `@notebook_mcp/visualization/describer.py`:
- Around line 166-167: The loop variable `l` in the list comprehension on line
166 is ambiguous and violates Ruff E741 linting rule (ambiguous variable name).
Rename the loop variable `l` to a more readable name like `line` throughout the
list comprehension where it appears three times - in the iteration `for l in
source.splitlines()`, in the condition `if l.strip()`, and in the condition `not
l.strip().startswith("#")`. This will improve code readability and satisfy the
linter.
In `@notebook_mcp/visualization/image_handler.py`:
- Around line 102-107: The size check using len(result_data) measures
base64-encoded characters rather than actual bytes, causing incorrect
enforcement of the max_size_kb limit. Modify the condition that checks if
len(result_data) > max_chars to instead use the decoded byte size of the
result_data. Decode the base64-encoded result_data to get the actual bytes and
compare that length against max_chars to properly enforce the KB size limit
before deciding to aggressively resize the PNG.
In `@server.py`:
- Around line 969-972: The startup log message in the info call claims
notebook_crud has 8 tools and a total of 43 tools, but the actual count of
Notebook CRUD tools is 7 (create_notebook, read_notebook, read_cell, edit_cell,
add_cell, read_notebook_outputs, read_cell_output), making the correct total 42.
Update the log message to change notebook_crud from 8 to 7 and the total from 43
to 42. Additionally, review the module docstring in lines 6-12 and ensure it
reflects the same corrected tool counts to maintain consistency across the
codebase.
In `@tests/test_security.py`:
- Around line 57-59: The test_none_raises method in the test class is catching
too broad a set of exceptions. The resolve_path() function explicitly validates
input and raises ValueError for non-string input, so change the assertRaises
call to expect only ValueError instead of the tuple of ValueError, TypeError,
and AttributeError. This will ensure the test enforces the actual API contract
and catches any regressions in the input validation behavior.
---
Nitpick comments:
In `@notebook_mcp/execution/kernel.py`:
- Line 154: In the unpacking statement where `km, kc = _kernels[key]` is
assigned, the variable `kc` is captured but never used in the subsequent code.
Replace `kc` with `_kc` (or simply `_` if it's more concise) to explicitly
indicate that this variable is intentionally unused and suppress any linting
warnings about unused variables. This clarifies your intent to readers and
follows Python naming conventions for intentionally ignored values.
- Around line 26-31: In the except ImportError block, add `from None` at the end
of the raise ImportError statement to suppress the original traceback and
provide a cleaner error message to users. This explicit exception chaining will
prevent Python from automatically displaying the original exception context,
which helps make the error output more user-friendly and focused on the helpful
installation instructions.
In `@server.py`:
- Around line 850-875: The code in the gallery generation loop directly accesses
private members of viz_server (_make_key, _viz_cache, and _render_gallery_page),
violating encapsulation principles. Create public methods in the viz_server
class to encapsulate these operations, such as a public method to generate and
cache gallery pages (e.g., build_gallery_page or similar) and a public method to
cache visualizations. Then replace all direct accesses to the private members in
the open_gallery function with calls to these new public methods.
In `@tests/test_cell_ops.py`:
- Around line 214-218: The test for clear_all_outputs() function only verifies
that cell outputs are cleared but does not check that execution_count is also
reset. Add an additional assertion within the for loop iterating through
nb2.cells to verify that the execution_count attribute is also reset to None for
each cell, ensuring the test fully validates the contract of the
clear_all_outputs() function.
In `@tests/test_security.py`:
- Around line 40-43: The teardown code is catching all exceptions and silently
suppressing them with a bare pass statement in the except block after
shutil.rmtree, which hides real cleanup issues and makes debugging difficult.
Modify the exception handling to either remove the overly broad exception
handling entirely or, if exception handling is necessary, log the error details
so that cleanup failures can be properly diagnosed instead of being silently
swallowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d142d08-f988-4e3c-b4e3-e1f4a163c106
⛔ Files ignored due to path filters (5)
screenshots/architecture.pngis excluded by!**/*.pngscreenshots/error_analysis.pngis excluded by!**/*.pngscreenshots/pipeline_analysis.pngis excluded by!**/*.pngscreenshots/tool_overview.pngis excluded by!**/*.pngscreenshots/viz_intelligence.pngis excluded by!**/*.png
📒 Files selected for processing (29)
README.mdnotebook_mcp/__init__.pynotebook_mcp/config.pynotebook_mcp/core/__init__.pynotebook_mcp/core/cell_ops.pynotebook_mcp/core/notebook_io.pynotebook_mcp/core/search.pynotebook_mcp/execution/__init__.pynotebook_mcp/execution/executor.pynotebook_mcp/execution/kernel.pynotebook_mcp/intelligence/__init__.pynotebook_mcp/intelligence/data_inspector.pynotebook_mcp/intelligence/pipeline.pynotebook_mcp/intelligence/summarizer.pynotebook_mcp/security.pynotebook_mcp/visualization/__init__.pynotebook_mcp/visualization/describer.pynotebook_mcp/visualization/detector.pynotebook_mcp/visualization/image_handler.pynotebook_mcp/visualization/viz_server.pypyproject.tomlrequirements.txtserver.pytests/__init__.pytests/test_cell_ops.pytests/test_intelligence.pytests/test_search.pytests/test_security.pytests/test_visualization.py
Comment on lines
+103
to
+104
| for node in ast.walk(tree): | ||
| if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): |
There was a problem hiding this comment.
ast.walk violates the documented “top-level only” outline contract.
On Line 103, ast.walk(tree) includes nested functions/classes, but the docstring (Line 73) promises top-level definitions. This will over-report symbols in notebook outlines.
Proposed fix
- for node in ast.walk(tree):
- if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)):
+ for node in tree.body:
+ if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)):📝 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
| for node in ast.walk(tree): | |
| if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): | |
| for node in tree.body: | |
| if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@notebook_mcp/core/search.py` around lines 103 - 104, The code at line 103
uses ast.walk(tree) to find function and class definitions, but ast.walk
traverses the entire AST including nested definitions, which violates the
documented top-level only contract stated in the docstring at line 73. Replace
ast.walk(tree) with direct iteration over tree.body to ensure only top-level
definitions (those at the root level of the module) are included in the outline,
preventing over-reporting of nested functions and classes.
Comment on lines
+69
to
+71
| for mime in ("image/png", "image/jpeg"): | ||
| if mime in data and len(data[mime]) > MAX_OUTPUT_SIZE: | ||
| data[mime] = data[mime][:MAX_OUTPUT_SIZE] |
There was a problem hiding this comment.
Truncating base64 image data at an arbitrary boundary corrupts the image.
Base64 encoding works in 4-character groups and requires proper padding. Slicing at MAX_OUTPUT_SIZE will likely produce invalid base64 that cannot be decoded, effectively corrupting the image output.
Consider one of these approaches:
- Skip truncation for image data entirely (let downstream consumers handle large images)
- Truncate at a valid base64 boundary (
MAX_OUTPUT_SIZE - (MAX_OUTPUT_SIZE % 4)) - Omit large images with a placeholder message
🐛 Proposed fix: omit large images with placeholder
elif msg_type in ("execute_result", "display_data"):
data = content.get("data", {})
for mime in ("image/png", "image/jpeg"):
if mime in data and len(data[mime]) > MAX_OUTPUT_SIZE:
- data[mime] = data[mime][:MAX_OUTPUT_SIZE]
+ # Replace with placeholder rather than corrupt the base64
+ data[mime] = ""
+ data.setdefault("text/plain", f"[Image truncated: {len(data[mime])} bytes exceeded limit]")📝 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
| for mime in ("image/png", "image/jpeg"): | |
| if mime in data and len(data[mime]) > MAX_OUTPUT_SIZE: | |
| data[mime] = data[mime][:MAX_OUTPUT_SIZE] | |
| elif msg_type in ("execute_result", "display_data"): | |
| data = content.get("data", {}) | |
| for mime in ("image/png", "image/jpeg"): | |
| if mime in data and len(data[mime]) > MAX_OUTPUT_SIZE: | |
| # Replace with placeholder rather than corrupt the base64 | |
| original_size = len(data[mime]) | |
| data[mime] = "" | |
| data.setdefault("text/plain", f"[Image truncated: {original_size} bytes exceeded limit]") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@notebook_mcp/execution/executor.py` around lines 69 - 71, The image data
truncation logic in the mime type checking loop is slicing base64-encoded image
data at an arbitrary boundary without considering base64 encoding structure
(4-character groups), which corrupts the image. Replace the direct slicing at
MAX_OUTPUT_SIZE with one of the following approaches: remove truncation for
image mime types entirely, calculate a valid base64 boundary by adjusting the
slice index to be divisible by 4, or omit the image data with a placeholder
message when it exceeds MAX_OUTPUT_SIZE. Update the conditional block that
checks for "image/png" and "image/jpeg" and truncates the data accordingly.
Comment on lines
+173
to
+201
| result: Dict[str, Any] = {"cell_index": cell_index} | ||
|
|
||
| for out in getattr(cell, "outputs", []): | ||
| if out.output_type not in ("execute_result", "display_data"): | ||
| continue | ||
| data = getattr(out, "data", {}) | ||
|
|
||
| # Parse HTML table to extract column names | ||
| if "text/html" in data: | ||
| html_content = data["text/html"] | ||
| # Extract column headers from <th> tags | ||
| headers = re.findall(r"<th>([^<]+)</th>", html_content) | ||
| if headers: | ||
| # First th is usually the index label | ||
| result["columns"] = headers[1:] if len(headers) > 1 else headers | ||
|
|
||
| # Count rows from <tr> tags (subtract header rows) | ||
| rows = html_content.count("<tr>") | ||
| if rows > 0: | ||
| result["visible_rows"] = rows - 1 # subtract header | ||
|
|
||
| # Parse text/plain for shape info from repr | ||
| if "text/plain" in data: | ||
| text = data["text/plain"] | ||
| m = re.search(r"\[(\d+)\s+rows\s+x\s+(\d+)\s+columns\]", text) | ||
| if m: | ||
| result["shape"] = [int(m.group(1)), int(m.group(2))] | ||
|
|
||
| return result |
There was a problem hiding this comment.
Return contract mismatch: function never returns an empty dict.
The docstring says “empty dict if not found”, but Line 173 seeds result with cell_index, so callers always get a non-empty payload even when no DataFrame metadata exists.
💡 Suggested fix
- result: Dict[str, Any] = {"cell_index": cell_index}
+ result: Dict[str, Any] = {}
@@
- return result
+ if result:
+ result["cell_index"] = cell_index
+ return result🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@notebook_mcp/intelligence/data_inspector.py` around lines 173 - 201, The
function initializes result with {"cell_index": cell_index} at the start, which
contradicts the docstring promise to return an empty dict when no DataFrame
metadata is found. Since the cell_index is always added regardless of whether
any actual metadata is extracted, the function never returns a truly empty dict.
Change the initialization to start with an empty result dict, then only add the
cell_index key to result after confirming that at least one piece of DataFrame
metadata (columns, visible_rows, or shape) was successfully extracted from the
cell outputs.
Comment on lines
+109
to
+111
| if all(line.strip().startswith(("import ", "from ", "#", "")) | ||
| for line in source.splitlines() if line.strip()): | ||
| return "imports" |
There was a problem hiding this comment.
imports classification is currently always true for fallback cells.
Line 109 includes "" in startswith(...), which matches every non-empty line. That causes many non-import cells to be classified as "imports" and distorts the pipeline role summary.
💡 Suggested fix
- if all(line.strip().startswith(("import ", "from ", "#", ""))
+ if all(line.strip().startswith(("import ", "from ", "#"))
for line in source.splitlines() if line.strip()):
return "imports"📝 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 all(line.strip().startswith(("import ", "from ", "#", "")) | |
| for line in source.splitlines() if line.strip()): | |
| return "imports" | |
| if all(line.strip().startswith(("import ", "from ", "#")) | |
| for line in source.splitlines() if line.strip()): | |
| return "imports" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@notebook_mcp/intelligence/pipeline.py` around lines 109 - 111, The imports
classification logic in the condition around line 109 is flawed because the
empty string "" in the startswith() tuple matches every non-empty line, causing
incorrect classification of non-import cells. Remove the empty string "" from
the tuple passed to startswith() so that the condition only validates lines that
actually begin with "import ", "from ", or "#" (comments), allowing proper cell
role classification.
Comment on lines
+54
to
+81
| workspace_dir = config.get_workspace() | ||
| if os.path.isabs(path): | ||
| real_target = os.path.realpath(path) | ||
| else: | ||
| real_target = os.path.realpath(os.path.join(workspace_dir, path)) | ||
|
|
||
| if config.is_initialized(): | ||
| real_workspace = os.path.realpath(workspace_dir) | ||
| norm_workspace = os.path.normcase(real_workspace) | ||
| norm_target = os.path.normcase(real_target) | ||
|
|
||
| try: | ||
| contained = ( | ||
| norm_target == norm_workspace | ||
| or os.path.commonpath([norm_workspace, norm_target]) == norm_workspace | ||
| ) | ||
| except ValueError: | ||
| contained = False | ||
|
|
||
| if not contained: | ||
| raise PermissionError( | ||
| f"Path '{path}' resolves to '{real_target}' which is outside " | ||
| f"the workspace root '{real_workspace}'. " | ||
| "Absolute paths and traversal sequences (../) that escape the " | ||
| "workspace are not permitted. (CWE-22 Path Traversal protection.)" | ||
| ) | ||
|
|
||
| return real_target |
There was a problem hiding this comment.
Make resolve_path fail-closed before workspace initialization.
resolve_path skips workspace containment when initialization is false, so it can return paths outside any jail. Since core readers call this function directly, this is a latent traversal/control gap if a caller path misses @require_workspace.
Suggested fix
def resolve_path(path: str) -> str:
@@
- workspace_dir = config.get_workspace()
+ if not config.is_initialized():
+ raise ValueError(
+ "ERROR: Workspace not initialized. Please call initialize_workspace() first."
+ )
+
+ workspace_dir = config.get_workspace()
@@
- if config.is_initialized():
- real_workspace = os.path.realpath(workspace_dir)
- norm_workspace = os.path.normcase(real_workspace)
- norm_target = os.path.normcase(real_target)
-
- try:
- contained = (
- norm_target == norm_workspace
- or os.path.commonpath([norm_workspace, norm_target]) == norm_workspace
- )
- except ValueError:
- contained = False
-
- if not contained:
- raise PermissionError(
- f"Path '{path}' resolves to '{real_target}' which is outside "
- f"the workspace root '{real_workspace}'. "
- "Absolute paths and traversal sequences (../) that escape the "
- "workspace are not permitted. (CWE-22 Path Traversal protection.)"
- )
+ real_workspace = os.path.realpath(workspace_dir)
+ norm_workspace = os.path.normcase(real_workspace)
+ norm_target = os.path.normcase(real_target)
+
+ try:
+ contained = (
+ norm_target == norm_workspace
+ or os.path.commonpath([norm_workspace, norm_target]) == norm_workspace
+ )
+ except ValueError:
+ contained = False
+
+ if not contained:
+ raise PermissionError(
+ f"Path '{path}' resolves to '{real_target}' which is outside "
+ f"the workspace root '{real_workspace}'. "
+ "Absolute paths and traversal sequences (../) that escape the "
+ "workspace are not permitted. (CWE-22 Path Traversal protection.)"
+ )📝 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
| workspace_dir = config.get_workspace() | |
| if os.path.isabs(path): | |
| real_target = os.path.realpath(path) | |
| else: | |
| real_target = os.path.realpath(os.path.join(workspace_dir, path)) | |
| if config.is_initialized(): | |
| real_workspace = os.path.realpath(workspace_dir) | |
| norm_workspace = os.path.normcase(real_workspace) | |
| norm_target = os.path.normcase(real_target) | |
| try: | |
| contained = ( | |
| norm_target == norm_workspace | |
| or os.path.commonpath([norm_workspace, norm_target]) == norm_workspace | |
| ) | |
| except ValueError: | |
| contained = False | |
| if not contained: | |
| raise PermissionError( | |
| f"Path '{path}' resolves to '{real_target}' which is outside " | |
| f"the workspace root '{real_workspace}'. " | |
| "Absolute paths and traversal sequences (../) that escape the " | |
| "workspace are not permitted. (CWE-22 Path Traversal protection.)" | |
| ) | |
| return real_target | |
| if not config.is_initialized(): | |
| raise ValueError( | |
| "ERROR: Workspace not initialized. Please call initialize_workspace() first." | |
| ) | |
| workspace_dir = config.get_workspace() | |
| if os.path.isabs(path): | |
| real_target = os.path.realpath(path) | |
| else: | |
| real_target = os.path.realpath(os.path.join(workspace_dir, path)) | |
| real_workspace = os.path.realpath(workspace_dir) | |
| norm_workspace = os.path.normcase(real_workspace) | |
| norm_target = os.path.normcase(real_target) | |
| try: | |
| contained = ( | |
| norm_target == norm_workspace | |
| or os.path.commonpath([norm_workspace, norm_target]) == norm_workspace | |
| ) | |
| except ValueError: | |
| contained = False | |
| if not contained: | |
| raise PermissionError( | |
| f"Path '{path}' resolves to '{real_target}' which is outside " | |
| f"the workspace root '{real_workspace}'. " | |
| "Absolute paths and traversal sequences (../) that escape the " | |
| "workspace are not permitted. (CWE-22 Path Traversal protection.)" | |
| ) | |
| return real_target |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@notebook_mcp/security.py` around lines 54 - 81, The resolve_path function
only performs the workspace containment check when config.is_initialized() is
true, which means uninitialized workspaces can return paths outside the
workspace boundary without validation. To fix this, move the containment
validation logic that uses os.path.commonpath and normcase comparisons outside
of the config.is_initialized() conditional block so that the path containment
check is always enforced regardless of initialization status, ensuring the
function fails securely.
Comment on lines
+44
to
+50
| def _render_image_page(title: str, mime: str, data: str, description: str = "") -> str: | ||
| """Render an HTML page with a single image and description.""" | ||
| if mime == "image/svg+xml": | ||
| img_tag = f'<div class="svg-container">{data}</div>' | ||
| else: | ||
| img_tag = f'<img src="data:{mime};base64,{data}" alt="{html.escape(title)}" style="max-width:100%;height:auto;">' | ||
|
|
There was a problem hiding this comment.
Raw notebook HTML/SVG is rendered without isolation (XSS risk).
At Line 47 and Line 105, notebook-provided SVG/HTML is injected directly into the response. A crafted notebook output can execute script when opened in the browser.
Suggested hardening direction
def _render_image_page(title: str, mime: str, data: str, description: str = "") -> str:
"""Render an HTML page with a single image and description."""
if mime == "image/svg+xml":
- img_tag = f'<div class="svg-container">{data}</div>'
+ svg_b64 = base64.b64encode(data.encode("utf-8")).decode("ascii")
+ img_tag = (
+ f'<img src="data:image/svg+xml;base64,{svg_b64}" '
+ f'alt="{html.escape(title)}" style="max-width:100%;height:auto;">'
+ )
@@
def _render_html_page(title: str, content: str, description: str = "") -> str:
@@
-<div class="content">{content}</div>
+<div class="content">
+ <iframe
+ sandbox="allow-scripts"
+ referrerpolicy="no-referrer"
+ style="width:100%;min-height:70vh;border:0;"
+ src="data:text/html;base64,{base64.b64encode(content.encode('utf-8')).decode('ascii')}">
+ </iframe>
+</div>Also applies to: 83-107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@notebook_mcp/visualization/viz_server.py` around lines 44 - 50, The
_render_image_page function injects raw SVG data directly into the HTML response
at line 47 without any sanitization or escaping, creating an XSS vulnerability.
Apply html.escape() to the data parameter when creating the SVG container div
element, similar to how the title is escaped in the img tag. Additionally,
review and apply the same sanitization to similar SVG/HTML injection points
mentioned around lines 83-107 to prevent malicious scripts from executing when
notebooks with crafted SVG output are opened in the browser.
Comment on lines
+210
to
+212
| if _httpd is not None: | ||
| return f"http://127.0.0.1:{port}" | ||
|
|
There was a problem hiding this comment.
ensure_server_running can return the wrong port after fallback.
If Line 217–Line 221 binds a fallback port, later calls hit Line 210–Line 212 and return the caller’s requested port instead of the actual bound port.
Suggested fix
- if _httpd is not None:
- return f"http://127.0.0.1:{port}"
+ if _httpd is not None:
+ return f"http://127.0.0.1:{_httpd.server_port}"
@@
- return f"http://127.0.0.1:{port}"
+ return f"http://127.0.0.1:{_httpd.server_port}"Also applies to: 229-230
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@notebook_mcp/visualization/viz_server.py` around lines 210 - 212, The
ensure_server_running function returns the caller's requested port parameter
instead of the actual bound port when a fallback port is used. Track the actual
bound port in a module-level or instance variable whenever a server is
successfully bound (in the fallback binding logic around lines 217-221). Then
modify the return statements in ensure_server_running at lines 210-212 and
229-230 to return the actual bound port from this tracked variable instead of
the port parameter passed to the function.
Comment on lines
+48
to
+50
| full = [ | ||
| "python-notebook-mcp[execution,imaging,export]", | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import tomllib, pathlib
data = tomllib.loads(pathlib.Path("pyproject.toml").read_text(encoding="utf-8"))
name = data["project"]["name"].lower()
full = data["project"]["optional-dependencies"].get("full", [])
self_refs = [d for d in full if d.lower().startswith(f"{name}[")]
print("project:", name)
print("full extra:", full)
print("self references:", self_refs)
print("EXPECTED: self references == []")
PYRepository: UsamaK98/python-notebook-mcp
Length of output: 262
🏁 Script executed:
cat pyproject.toml | head -100Repository: UsamaK98/python-notebook-mcp
Length of output: 1693
🏁 Script executed:
#!/bin/bash
rg "optional-dependencies" -A 20 pyproject.tomlRepository: UsamaK98/python-notebook-mcp
Length of output: 470
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import tomllib, pathlib
data = tomllib.loads(pathlib.Path("pyproject.toml").read_text(encoding="utf-8"))
deps = data["project"].get("optional-dependencies", {})
for extra, deps_list in sorted(deps.items()):
print(f"\n[{extra}]")
for d in deps_list:
print(f" {d}")
PYRepository: UsamaK98/python-notebook-mcp
Length of output: 316
🌐 Web query:
pip setuptools self-referential optional extras dependency resolution issues
💡 Result:
Self-referential optional extras (often called recursive extras) allow a package to define an optional dependency set that includes the package itself with different extras (e.g., [pkg-name][extra-name]) [1][2]. While technically supported by pip since version 21.2, this practice introduces specific nuances and potential resolution issues that developers should be aware of [1][2]. Key Considerations and Resolution Issues: 1. Redundant Installation: When you use a self-reference like my-pkg[tests] in an optional dependency, pip treats the main package my-pkg as a required dependency [3]. Consequently, installing the extra will explicitly install the main package as well, which may be undesirable if you are only trying to resolve internal dependency subsets [3]. 2. Resolution Overhead and Complexity: The pip dependency resolver must handle these self-references as part of the broader dependency graph [4]. While the resolver is generally capable of managing these, complex or deeply nested circular references can sometimes lead to confusing error messages or significant performance degradation, particularly when overlapping dependencies are involved [4]. 3. Metadata Normalization: There is ongoing discussion in the Python packaging community regarding how build backends should handle these extras when generating core metadata [5]. Some backends "flatten" these recursive dependencies into the primary Requires-Dist list, while others may preserve the structure [5]. This can lead to discrepancies between the metadata declared in pyproject.toml and the actual metadata provided by the build backend, which might affect tools like pip-compile that rely on high-fidelity metadata for lockfile generation [5][3]. 4. Best Practices: - Use self-referential extras primarily for defining development environments (e.g., a dev extra that includes tests, docs, and linting extras) to simplify installation for contributors [2]. - If using tools like pip-tools to generate requirements files, be aware that self-references may include the project itself in the output [3]. You may need to use flags like --unsafe-package (in pip-compile) to manage how these references are handled in your lockfiles [3]. - Note that while this feature is functional, it is not always explicitly detailed in all high-level packaging documentation, as it relies on the underlying PEP-compliant interpretation of dependency specifiers [6][7]. In summary, while self-referential extras are a powerful way to organize project dependencies, they should be used judiciously to avoid unnecessary complexity in dependency resolution and lockfile management [3][4].
Citations:
- 1: Document support for self-referential extras pypa/pip#11296
- 2: https://hynek.me/articles/python-recursive-optional-dependencies/
- 3: When using self-referential extras in pyproject.toml, the package is added to the requirements jazzband/pip-tools#2002
- 4: Extremely slow dependency resolution with circular graph of dependencies pypa/pip#12210
- 5: https://discuss.python.org/t/core-metadata-for-self-referential-extras/77793/1
- 6: https://discuss.python.org/t/where-is-nested-recursive-optional-dependencies-documented/35648
- 7: https://github.com/pypa/packaging.python.org/blob/main/source/specifications/dependency-specifiers.rst
Replace the self-referential full extra with concrete dependencies.
The full extra currently depends on the project itself (python-notebook-mcp[execution,imaging,export]), which creates resolver complexity and metadata normalization issues during installation. Flatten this to explicit dependencies:
full = [
- "python-notebook-mcp[execution,imaging,export]",
+ "jupyter_client>=8.0",
+ "ipykernel>=6.0",
+ "Pillow>=10.0",
+ "nbconvert>=7.0",
]📝 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
| full = [ | |
| "python-notebook-mcp[execution,imaging,export]", | |
| ] | |
| full = [ | |
| "jupyter_client>=8.0", | |
| "ipykernel>=6.0", | |
| "Pillow>=10.0", | |
| "nbconvert>=7.0", | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyproject.toml` around lines 48 - 50, The `full` extra in the pyproject.toml
file contains a self-referential dependency on
`python-notebook-mcp[execution,imaging,export]`, which creates resolver
complexity. Replace this self-reference with explicit concrete dependencies by
identifying all the individual packages listed in the execution, imaging, and
export extras sections and including them directly in the `full` extra instead
of referencing the extras by name.
Comment on lines
+60
to
+66
| [project.scripts] | ||
| notebook-mcp = "server:main" | ||
|
|
||
| [tool.setuptools.packages.find] | ||
| where = ["."] | ||
| include = ["notebook_mcp*"] | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import tomllib, pathlib
p = pathlib.Path("pyproject.toml")
data = tomllib.loads(p.read_text(encoding="utf-8"))
target = data["project"]["scripts"]["notebook-mcp"].split(":")[0]
module_file = pathlib.Path(*target.split(".")).with_suffix(".py")
package_dir = pathlib.Path(*target.split("."))
include = data.get("tool", {}).get("setuptools", {}).get("packages", {}).get("find", {}).get("include", [])
py_modules = data.get("tool", {}).get("setuptools", {}).get("py-modules", [])
print("script target module:", target)
print("module file exists:", module_file.exists())
print("package dir exists:", package_dir.is_dir())
print("packages.find.include:", include)
print("py-modules:", py_modules)
print("EXPECTED: target is inside included package OR listed in py-modules")
PYRepository: UsamaK98/python-notebook-mcp
Length of output: 276
Ensure the console entrypoint module is packaged.
notebook-mcp = "server:main" targets the root-level server.py module, but package discovery only includes notebook_mcp*. The server module is neither in the included packages nor listed in py-modules, so it will be excluded from built distributions and break the console script.
Add py-modules = ["server"] to the setuptools configuration to ensure the module is packaged:
Suggested fix
+[tool.setuptools]
+py-modules = ["server"]
+
[tool.setuptools.packages.find]
where = ["."]
include = ["notebook_mcp*"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyproject.toml` around lines 60 - 66, The console entrypoint defined in
[project.scripts] as notebook-mcp = "server:main" references the root-level
server.py module, but the setuptools package discovery configuration only
includes packages matching the pattern notebook_mcp*, which excludes the server
module. Add a py-modules configuration option in the [tool.setuptools] section
and set it to ["server"] to explicitly include the server.py module in the
distribution so that the console script entrypoint can be resolved when the
package is installed.
| <a href="LICENSE"><img src="https://img.shields.io/badge/License-MIT-blue.svg" alt="MIT License"/></a> | ||
| <img src="https://img.shields.io/badge/Python-3.10+-blue.svg" alt="Python 3.10+"/> | ||
| <img src="https://img.shields.io/badge/MCP-Compatible-orange.svg" alt="MCP Compatible"/> | ||
| <img src="https://img.shields.io/badge/Tools-43-green.svg" alt="43 MCP Tools"/> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Count unique tool registrations in server.py to determine actual tool count
rg -o '\btool_name\s*=\s*"[^"]+"' server.py | wc -l
# or count add_tool calls:
rg 'add_tool\(' server.py | wc -l
# or list tool names explicitly:
rg -A1 'add_tool\(' server.py | grep 'tool_name' | sort | uniqRepository: UsamaK98/python-notebook-mcp
Length of output: 76
🏁 Script executed:
git ls-files | head -30Repository: UsamaK98/python-notebook-mcp
Length of output: 952
🏁 Script executed:
# Search for files that might contain tool definitions
fd -e py | grep -E '(server|tool|main|register)' | head -20Repository: UsamaK98/python-notebook-mcp
Length of output: 123
🏁 Script executed:
# Look for patterns defining tools in Python files
rg 'def (register|add|list).*tool' -t py --max-count 10Repository: UsamaK98/python-notebook-mcp
Length of output: 54
🏁 Script executed:
cat server.pyRepository: UsamaK98/python-notebook-mcp
Length of output: 32643
🏁 Script executed:
# Search for tool definitions more broadly in Python files
rg '@|def |class ' server.py | head -50Repository: UsamaK98/python-notebook-mcp
Length of output: 1609
Fix tool count badge: claims 43 but implementation provides 42.
The badge on line 8 claims "Tools-43", but the actual implementation in server.py contains exactly 42 MCP tools:
- Workspace & Navigation: 4
- Notebook CRUD: 7
- Cell Management: 14
- Execution Engine: 8
- Visualization Intelligence: 5
- Notebook Intelligence: 4
Either add a 43rd tool to match the badge or update the badge to "Tools-42".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 8, The badge in the README.md displays "Tools-43" but the
actual implementation in server.py only contains 42 MCP tools. Verify the tool
count in server.py by reviewing the implementations in categories: Workspace &
Navigation (4), Notebook CRUD (7), Cell Management (14), Execution Engine (8),
Visualization Intelligence (5), and Notebook Intelligence (4), which totals 42.
Then update the badge to display "Tools-42" to match the actual tool count, or
alternatively implement a 43rd tool to match the badge claim.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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