feat(core): add NotebookEdit tool for Jupyter notebooks#3900
Merged
Conversation
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
…-2816 # Conflicts: # packages/core/src/tools/priorReadEnforcement.ts
wenshao
previously requested changes
May 9, 2026
…-2816 # Conflicts: # packages/core/src/services/fileReadCache.ts # packages/core/src/tools/read-file.ts # packages/core/src/tools/tool-names.ts
Collaborator
Author
|
Addressed the review feedback in
Local validation: npx eslint packages/core/src/config/config.test.ts packages/core/src/utils/notebook.ts packages/core/src/utils/notebook.test.ts packages/core/src/tools/notebook-edit.ts packages/core/src/tools/notebook-edit.test.ts
npx vitest run packages/core/src/utils/notebook.test.ts packages/core/src/tools/notebook-edit.test.ts packages/core/src/services/fileReadCache.test.ts packages/core/src/tools/read-file.test.ts packages/core/src/core/coreToolScheduler.test.ts
npx vitest run packages/core/src/config/config.test.ts
npm run typecheck --workspace=packages/core
npm run build
npm run bundle
QWEN_SANDBOX=false npx vitest run --root ./integration-tests cli/notebook-edit.test.ts --poolOptions.threads.minThreads 1 --poolOptions.threads.maxThreads 1
git diff --checkI also reran the two unrelated full-core timeout cases individually and both passed. |
wenshao
reviewed
May 11, 2026
wenshao
reviewed
May 12, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Suggestion (cross-cutting): priorReadEnforcement.ts:243 still returns EDIT_REQUIRES_PRIOR_READ for FIFO/socket/device files, while this PR correctly introduces TARGET_NOT_REGULAR_FILE in notebook-edit.ts:364. The shared checkPriorRead path used by EditTool/WriteFileTool would benefit from using the new error type as well, to avoid the read/edit retry loop that the new type was designed to prevent.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
…-2816 # Conflicts: # packages/core/src/services/fileReadCache.ts
Collaborator
Author
|
自测：1. 修改cell 内容： |
wenshao
previously requested changes
May 19, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Cross-file impact (not in PR files):
packages/core/src/utils/readManyFiles.ts:204— This PR makesprocessSingleFileContentreturnisTruncated: truefor large notebooks without settinglinesShownororiginalLineCount.readManyFiles.tsdestructuresfileReadResult.linesShown!→ TypeError. The guard atread-file.ts:278-282was not applied inreadManyFiles.ts. Scenario:@src/notebooks/with a large-output notebook crashes the batch read.packages/core/src/tools/priorReadEnforcement.ts:~253— The "non-text payload" error message directs the model to "use shell tool with a binary-aware writer" for.ipynbfiles, instead of pointing to the newnotebook_edittool. Shell-based notebook editing bypasses all cell-level safety.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
Collaborator
Author
|
Addressed the latest review feedback in
Validation: npm test --workspace=packages/core -- --run src/tools/notebook-edit.test.ts src/utils/readManyFiles.test.ts src/tools/edit.test.ts src/tools/write-file.test.ts
npm run typecheck --workspace=packages/core
./node_modules/.bin/eslint packages/core/src/tools/notebook-edit.ts packages/core/src/tools/notebook-edit.test.ts packages/core/src/utils/readManyFiles.ts packages/core/src/utils/readManyFiles.test.ts packages/core/src/tools/priorReadEnforcement.ts packages/core/src/tools/edit.test.ts packages/core/src/tools/write-file.test.ts --max-warnings 0
./node_modules/.bin/prettier --check packages/core/src/tools/notebook-edit.ts packages/core/src/tools/notebook-edit.test.ts packages/core/src/utils/readManyFiles.ts packages/core/src/utils/readManyFiles.test.ts packages/core/src/tools/priorReadEnforcement.ts packages/core/src/tools/edit.test.ts packages/core/src/tools/write-file.test.ts
npm run build --workspace=packages/coreThe inline suggestion thread was replied to and resolved. |
Collaborator
Author
|
Pushed Summary:
Validation passed locally: npm test --workspace=packages/core -- --run src/utils/notebook.test.ts src/tools/notebook-edit.test.ts src/utils/readManyFiles.test.ts src/tools/edit.test.ts src/tools/write-file.test.ts
npm run typecheck --workspace=packages/core
./node_modules/.bin/eslint packages/core/src/tools/notebook-edit.ts packages/core/src/tools/notebook-edit.test.ts packages/core/src/utils/notebook.ts packages/core/src/utils/notebook.test.ts packages/core/src/utils/readManyFiles.ts packages/core/src/utils/readManyFiles.test.ts packages/core/src/tools/priorReadEnforcement.ts packages/core/src/tools/edit.test.ts packages/core/src/tools/write-file.test.ts --max-warnings 0
./node_modules/.bin/prettier --check packages/core/src/tools/notebook-edit.ts packages/core/src/tools/notebook-edit.test.ts packages/core/src/utils/notebook.ts packages/core/src/utils/notebook.test.ts packages/core/src/utils/readManyFiles.ts packages/core/src/utils/readManyFiles.test.ts packages/core/src/tools/priorReadEnforcement.ts packages/core/src/tools/edit.test.ts packages/core/src/tools/write-file.test.ts
git diff --check
npm run build --workspace=packages/coreI also replied to and resolved each currently unresolved inline review thread. |
Collaborator
Author
|
Pushed Docs updated:
Validation passed: ./node_modules/.bin/prettier --check docs/developers/tools/file-system.md docs/users/configuration/settings.md docs/users/features/approval-mode.md docs/developers/sdk-typescript.md packages/sdk-typescript/README.md docs/developers/sdk-java.md packages/sdk-java/qwencode/README.md packages/sdk-java/qwencode/QWEN.md
git diff --check |
Collaborator
Author
|
Pushed Summary:
Validation passed locally: npm test --workspace=packages/core -- --run src/tools/read-file.test.ts src/utils/notebook.test.ts
npm run typecheck --workspace=packages/core
./node_modules/.bin/eslint packages/core/src/tools/read-file.ts packages/core/src/utils/notebook.ts packages/core/src/tools/read-file.test.ts packages/core/src/utils/notebook.test.ts --max-warnings 0
./node_modules/.bin/prettier --check packages/core/src/tools/read-file.ts packages/core/src/utils/notebook.ts packages/core/src/tools/read-file.test.ts packages/core/src/utils/notebook.test.ts
git diff --check
npm run build --workspace=packages/coreI also checked the GitHub review threads after the push; there are currently no unresolved inline threads to reply to or resolve individually. |
Collaborator
Author
|
Pushed The new test creates a UTF-8 BOM-prefixed Validation passed locally: npm test --workspace=packages/core -- --run src/tools/notebook-edit.test.ts
npm test --workspace=packages/core -- --run src/tools/notebook-edit.test.ts src/tools/read-file.test.ts src/utils/notebook.test.ts
npm run typecheck --workspace=packages/core
./node_modules/.bin/eslint packages/core/src/tools/notebook-edit.test.ts packages/core/src/tools/read-file.ts packages/core/src/utils/notebook.ts packages/core/src/tools/read-file.test.ts packages/core/src/utils/notebook.test.ts --max-warnings 0
./node_modules/.bin/prettier --check packages/core/src/tools/notebook-edit.test.ts packages/core/src/tools/read-file.ts packages/core/src/utils/notebook.ts packages/core/src/tools/read-file.test.ts packages/core/src/utils/notebook.test.ts
git diff --check
npm run build --workspace=packages/core |
yiliang114
approved these changes
May 20, 2026
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the updates. I rechecked the latest revision, and all CI checks are green now.
The two issues I raised earlier have also been addressed: read_file now rejects unsupported pages for .ipynb files, and UTF-8 BOM notebooks are parsed and written back correctly with BOM preservation.
Given the local recheck and the passing CI, this looks good to merge from my side. LGTM.
wenshao
previously requested changes
May 20, 2026
|
|
||
| export function inferNotebookJsonFormat(content: string): NotebookJsonFormat { | ||
| const lineMatch = content.match(/\n( +)"/); | ||
| return { |
Collaborator
There was a problem hiding this comment.
[Critical] inferNotebookJsonFormat regex /\n( +)"/ only detects space-based indentation. Tab-indented notebooks (valid JSON, produced by some editors including VS Code with tab settings) yield indent: undefined, causing serializeNotebook to call JSON.stringify(notebook, null, undefined) — producing minified single-line JSON. A single cell edit would destroy formatting across the entire file.
Suggested change
| return { | |
| export function inferNotebookJsonFormat(content: string): NotebookJsonFormat { | |
| const trailingNewline = content.endsWith('\n'); | |
| const match = content.match(/\n([ \t]+)"/); | |
| const indent = match?.[1] !== undefined | |
| ? (match[1].includes('\t') ? match[1] : match[1].length) | |
| : undefined; | |
| return { indent, trailingNewline }; | |
| } |
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| } | ||
| } | ||
|
|
||
| override async execute(signal: AbortSignal): Promise<ToolResult> { |
Collaborator
There was a problem hiding this comment.
[Suggestion] No debug logging on the successful write path. debugLogger is only invoked for prior-read rejections (line 110) and post-write cache failures (line 635). A successful notebook edit — including which cell was targeted, mode used, requiresReadAfterWrite decision, and whether content was user-modified — produces zero debug output. At 3 AM debugging a "model deleted the wrong cell" report, there's no trace to reconstruct what happened.
Suggested change
| override async execute(signal: AbortSignal): Promise<ToolResult> { | |
| // After successful write, before return: | |
| debugLogger.debug('execute-success', { | |
| path: this.params.notebook_path, | |
| mode: this.params.edit_mode, | |
| cellId: this.params.cell_id, | |
| requiresReadAfterWrite: result.requiresReadAfterWrite, | |
| modifiedByUser: this.modifyMetadata !== undefined, | |
| }); |
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| if (prepared.requiresReadAfterWrite) { | ||
| cache.invalidate(postWriteStats); | ||
| } else { | ||
| cache.recordWrite(this.params.notebook_path, postWriteStats, { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The { cacheable: false } flag here is the sole guard preventing the regular Edit/WriteFile tools from corrupting .ipynb files. checkPriorRead in priorReadEnforcement.ts rejects files where lastReadCacheable === false. But this contract is entirely implicit — no type-level enforcement, no named constant, no runtime assertion. A future tool that writes a notebook without this flag would silently make the notebook text-editable, leading to JSON corruption through text-level edits.
Consider extracting this into a named helper (e.g., recordNotebookWrite(path, stats)) that wraps recordWrite and documents the invariant, or adding a runtime assertion in recordWrite that cross-references .ipynb extension against cacheable.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| // never terminate; the rejection points the model at | ||
| // shell instead. | ||
| // re-reading can change: non-text payloads (binary / image / | ||
| // audio / video / PDF / notebook). read_file returns these as |
Collaborator
There was a problem hiding this comment.
[Suggestion] The comment for EDIT_REQUIRES_PRIOR_READ describes case 3 as "non-text payloads (binary / image / audio / video / PDF / notebook)" but omits FIFO/socket/device files. However, priorReadEnforcement.ts:244 still returns EDIT_REQUIRES_PRIOR_READ for !stats.isFile() (FIFO/socket/device). The comment inaccurately describes this usage, and the new TARGET_NOT_REGULAR_FILE code (line 88) is only used by notebook-edit.ts, making the inconsistency between the two tools more confusing.
Suggested fix: either add FIFO/socket/device to the case 3 description, or update priorReadEnforcement.ts to use TARGET_NOT_REGULAR_FILE for consistency.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| NotebookEditToolParams, | ||
| NotebookEditModifyMetadata | ||
| >(); | ||
| private readonly modifyMetadataByKey = new Map< |
Collaborator
There was a problem hiding this comment.
[Suggestion] modifyMetadataByKey is a regular Map<string, NotebookEditModifyMetadata[]> with no cleanup mechanism. If a user-modify operation is cancelled before consumeModifyMetadata runs, the entry persists forever. Each entry can hold large aiProposedContent and modifiedNotebookContent strings (full serialized notebook JSON). Over a long session with many cancelled modify attempts, this Map grows without bound.
The companion modifyMetadataByParams (WeakMap) is GC-safe, but the string-keyed Map is not. Consider adding TTL-based eviction, a size cap, or restructuring to encode metadata on the params object directly (similar to how EditTool handles it).
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| ); | ||
| } | ||
|
|
||
| protected override validateToolParamValues( |
Collaborator
There was a problem hiding this comment.
[Suggestion] validateToolParamValues has 6 validation branches (empty path, non-absolute path, wrong extension, invalid mode, invalid cell_type, missing source) but no direct unit tests covering the specific error messages. JSON Schema catches some cases, but the custom validation's user-friendly error messages are never asserted in tests.
Add expect(...).toThrow(...) tests for each branch to catch regressions in validation logic or error message changes.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| fileDiff, | ||
| originalContent: prepared.originalContent, | ||
| newContent: prepared.updatedContent, | ||
| onConfirm: async (outcome: ToolConfirmationOutcome) => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The onConfirm(ProceedAlways) callback calls this.config.setApprovalMode(ApprovalMode.AUTO_EDIT), but this behavior is never tested. If the callback is accidentally removed or the parameter changes, no test would detect it.
Suggested change
| onConfirm: async (outcome: ToolConfirmationOutcome) => { | |
| // In the confirmation test: | |
| const editDetails = details as Extract<typeof details, { type: 'edit' }>; | |
| await editDetails.onConfirm(ToolConfirmationOutcome.ProceedAlways); | |
| expect(config.setApprovalMode).toHaveBeenCalledWith(ApprovalMode.AUTO_EDIT); |
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| let llmContent: PartUnion; | ||
| if (result.isTruncated) { | ||
| if ( | ||
| result.isTruncated && |
Collaborator
There was a problem hiding this comment.
[Suggestion] The guard if (result.isTruncated && result.linesShown && result.originalLineCount !== undefined) silently assumes that any isTruncated result WITHOUT linesShown already has truncation info baked into the content string. This is true for notebooks today (they embed "... [N remaining cells truncated...]"), but the assumption is undocumented.
If a future non-text file type sets isTruncated: true without linesShown and without embedding its own truncation message, the LLM will never learn the content was truncated. Add a comment explaining the assumption, or a defensive else if (result.isTruncated) branch with a generic truncation notice.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| getCurrentContent: async ( | ||
| params: NotebookEditToolParams, | ||
| ): Promise<string> => readCurrentContentSnapshot(params), | ||
| getProposedContent: async ( |
Collaborator
There was a problem hiding this comment.
[Suggestion] getProposedContent calls applyNotebookEdit directly, which can throw NotebookEditError (with .type field). The framework in coreToolScheduler checks .errorType (from StructuredToolError) for error classification. Since NotebookEditError.type ≠ .errorType, errors from this path would be classified as UNHANDLED_EXCEPTION instead of the proper type (e.g., NOTEBOOK_CELL_NOT_FOUND).
Wrap with the same conversion used in prepareEdit:
Suggested change
| getProposedContent: async ( | |
| getProposedContent: async (params) => { | |
| const content = await readCurrentContentSnapshot(params); | |
| try { | |
| return applyNotebookEdit(content, params).updatedContent; | |
| } catch (error) { | |
| if (error instanceof NotebookEditError) { | |
| throw new StructuredToolError(error.message, error.type); | |
| } | |
| throw error; | |
| } | |
| }, |
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| } | ||
|
|
||
| function prepareModifiedNotebookContent( | ||
| originalContent: string, |
Collaborator
There was a problem hiding this comment.
[Suggestion] prepareModifiedNotebookContent uses a more conservative requiresReadAfterWrite formula (!hasStableCellIds(original) || !hasStableCellIds(notebook)) compared to applyNotebookEdit (structuralEdit && !(originalHasStable && updatedHasStable)). The difference is intentional (user-modified content may have arbitrary structural changes) but undocumented. A future maintainer reading one formula might "harmonize" the other to match, breaking one of the two paths.
Add a comment explaining why the user-modify path is more conservative.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
reviewed
May 20, 2026
|
|
||
| export function inferNotebookJsonFormat(content: string): NotebookJsonFormat { | ||
| const lineMatch = content.match(/\n( +)"/); | ||
| return { |
Collaborator
There was a problem hiding this comment.
[Critical] inferNotebookJsonFormat regex /\\n( +)"/ only detects space-based indentation. Tab-indented notebooks (valid JSON, produced by some editors including VS Code with tab settings) yield indent: undefined, causing serializeNotebook to call JSON.stringify(notebook, null, undefined) — producing minified single-line JSON. A single cell edit would destroy formatting across the entire file.
Suggested change
| return { | |
| export function inferNotebookJsonFormat(content: string): NotebookJsonFormat { | |
| const trailingNewline = content.endsWith('\n'); | |
| const match = content.match(/\n([ \t]+)"/); | |
| const indent = match?.[1] !== undefined | |
| ? (match[1].includes('\t') ? match[1] : match[1].length) | |
| : undefined; | |
| return { indent, trailingNewline }; | |
| } |
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
reviewed
May 20, 2026
| } | ||
| } | ||
|
|
||
| override async execute(signal: AbortSignal): Promise<ToolResult> { |
Collaborator
There was a problem hiding this comment.
[Suggestion] No debug logging on the successful write path. debugLogger is only invoked for prior-read rejections (line 110) and post-write cache failures (line 635). A successful notebook edit — including which cell was targeted, mode used, requiresReadAfterWrite decision, and whether content was user-modified — produces zero debug output. At 3 AM debugging a "model deleted the wrong cell" report, there's no trace to reconstruct what happened.
Add debugLogger.debug('execute-success', { path, mode, cellId, requiresReadAfterWrite, modifiedByUser }) after the write succeeds.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
reviewed
May 20, 2026
| if (prepared.requiresReadAfterWrite) { | ||
| cache.invalidate(postWriteStats); | ||
| } else { | ||
| cache.recordWrite(this.params.notebook_path, postWriteStats, { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The { cacheable: false } flag here is the sole guard preventing the regular Edit/WriteFile tools from corrupting .ipynb files. checkPriorRead in priorReadEnforcement.ts rejects files where lastReadCacheable === false. But this contract is entirely implicit — no type-level enforcement, no named constant, no runtime assertion. A future tool that writes a notebook without this flag would silently make the notebook text-editable, leading to JSON corruption through text-level edits.
Consider extracting into a named helper (e.g., recordNotebookWrite(path, stats)) that documents the invariant.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
reviewed
May 20, 2026
| // never terminate; the rejection points the model at | ||
| // shell instead. | ||
| // re-reading can change: non-text payloads (binary / image / | ||
| // audio / video / PDF / notebook). read_file returns these as |
Collaborator
There was a problem hiding this comment.
[Suggestion] The comment for EDIT_REQUIRES_PRIOR_READ describes case 3 as "non-text payloads (binary / image / audio / video / PDF / notebook)" but omits FIFO/socket/device files. However, priorReadEnforcement.ts:244 still returns EDIT_REQUIRES_PRIOR_READ for !stats.isFile() (FIFO/socket/device). The comment inaccurately describes this usage, and the new TARGET_NOT_REGULAR_FILE code is only used by notebook-edit.ts, making the inconsistency confusing.
Fix: add FIFO/socket/device to the case 3 description, or update priorReadEnforcement.ts to use TARGET_NOT_REGULAR_FILE for consistency.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
reviewed
May 20, 2026
| NotebookEditToolParams, | ||
| NotebookEditModifyMetadata | ||
| >(); | ||
| private readonly modifyMetadataByKey = new Map< |
Collaborator
There was a problem hiding this comment.
[Suggestion] modifyMetadataByKey is a regular Map<string, NotebookEditModifyMetadata[]> with no cleanup mechanism. If a user-modify operation is cancelled before consumeModifyMetadata runs, the entry persists forever holding multi-KB notebook content strings. Over a long session with many cancelled modify attempts, this Map grows without bound.
Consider adding TTL-based eviction, a size cap, or restructuring to encode metadata on the params object directly (similar to how EditTool handles it).
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
reviewed
May 20, 2026
| return cell ? getCellDisplayId(cell, index) : `cell-${index}`; | ||
| } | ||
|
|
||
| function resolveTargetIndex( |
Collaborator
There was a problem hiding this comment.
[Suggestion] resolveTargetIndex calls isAmbiguousCellId() and findCellIndex() sequentially, each internally calling findCellIndexesByDisplayId() which iterates every cell via forEach. The cell array is scanned twice for the same cellId.
Call findCellIndexesByDisplayId once, check length for ambiguity (>1), zero (not found), and return the index — eliminating the redundant scan.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
reviewed
May 20, 2026
| ); | ||
| } | ||
|
|
||
| protected override validateToolParamValues( |
Collaborator
There was a problem hiding this comment.
[Suggestion] validateToolParamValues has 6 validation branches (empty path, non-absolute path, wrong extension, invalid mode, invalid cell_type, missing source) but no direct unit tests covering the specific error messages. JSON Schema catches some cases, but the custom validation's user-friendly error messages are never asserted.
Add expect(...).toThrow(...) tests for each branch to catch regressions.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
reviewed
May 20, 2026
| fileDiff, | ||
| originalContent: prepared.originalContent, | ||
| newContent: prepared.updatedContent, | ||
| onConfirm: async (outcome: ToolConfirmationOutcome) => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The onConfirm(ProceedAlways) callback calls setApprovalMode(AUTO_EDIT), but this behavior is never tested. If the callback is removed or the parameter changes, no test would detect it.
Add to the confirmation test: await editDetails.onConfirm(ToolConfirmationOutcome.ProceedAlways); expect(config.setApprovalMode).toHaveBeenCalledWith(ApprovalMode.AUTO_EDIT);
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
reviewed
May 20, 2026
| let llmContent: PartUnion; | ||
| if (result.isTruncated) { | ||
| if ( | ||
| result.isTruncated && |
Collaborator
There was a problem hiding this comment.
[Suggestion] The guard if (result.isTruncated && result.linesShown && result.originalLineCount !== undefined) silently assumes any isTruncated result WITHOUT linesShown already has truncation info in the content. True for notebooks today, but undocumented. A future non-text file type that sets isTruncated: true without linesShown and without embedding its own truncation message would silently lose the truncation signal.
Add a comment explaining the assumption, or a defensive else if (result.isTruncated) branch.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
reviewed
May 20, 2026
| getCurrentContent: async ( | ||
| params: NotebookEditToolParams, | ||
| ): Promise<string> => readCurrentContentSnapshot(params), | ||
| getProposedContent: async ( |
Collaborator
There was a problem hiding this comment.
[Suggestion] getProposedContent calls applyNotebookEdit directly, which can throw NotebookEditError (with .type field). The framework checks .errorType (from StructuredToolError). Since NotebookEditError.type ≠ .errorType, errors would be classified as UNHANDLED_EXCEPTION instead of the proper type.
Wrap with: try { return applyNotebookEdit(content, params).updatedContent; } catch (e) { if (e instanceof NotebookEditError) throw new StructuredToolError(e.message, e.type); throw e; }
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
reviewed
May 20, 2026
| } | ||
|
|
||
| function prepareModifiedNotebookContent( | ||
| originalContent: string, |
Collaborator
There was a problem hiding this comment.
[Suggestion] prepareModifiedNotebookContent uses a more conservative requiresReadAfterWrite formula (!hasStableCellIds(original) || !hasStableCellIds(notebook)) compared to applyNotebookEdit (structuralEdit && !(originalHasStable && updatedHasStable)). The difference is intentional (user-modified content may have arbitrary structural changes) but undocumented. A future maintainer might "harmonize" them, breaking one path.
Add a comment explaining why the user-modify path is more conservative.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
reviewed
May 20, 2026
| if (index === -1) { | ||
| throw new NotebookEditError( | ||
| `Cell with ID "${cellId}" not found in notebook.`, | ||
| ToolErrorType.NOTEBOOK_CELL_NOT_FOUND, |
Collaborator
There was a problem hiding this comment.
[Suggestion] resolveTargetIndex returns NOTEBOOK_CELL_NOT_FOUND for all modes including insert, but the insert-mode-with-nonexistent-cell_id scenario is not covered by tests. Only insert-without-cell_id (insert at beginning) and insert-with-existing-cell_id are tested.
Suggested change
| ToolErrorType.NOTEBOOK_CELL_NOT_FOUND, | |
| // Add test: insert mode with cell_id="nonexistent" → expect NOTEBOOK_CELL_NOT_FOUND |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const code = (error as NodeJS.ErrnoException | undefined)?.code; | ||
| if (code === 'ENOENT') { | ||
| if (options.expectExisting) { | ||
| return rejectNotebookPriorRead(notebookPath, 'missing-after-read', { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The TOCTOU path where expectExisting=true and the file disappears between checks (ENOENT → FILE_CHANGED_SINCE_READ) lacks test coverage. This is a key safety path: the model reads a notebook, but before the edit executes, the file is deleted externally.
Suggested change
| return rejectNotebookPriorRead(notebookPath, 'missing-after-read', { | |
| // Add test: seed a read, delete the file before execute(), verify FILE_CHANGED_SINCE_READ |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return `Notebook path must be absolute: ${params.notebook_path}`; | ||
| } | ||
|
|
||
| if (path.extname(params.notebook_path).toLowerCase() !== '.ipynb') { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The non-.ipynb extension rejection in validateToolParamValues is not tested. All existing tests use .ipynb paths.
Suggested change
| if (path.extname(params.notebook_path).toLowerCase() !== '.ipynb') { | |
| // Add test: notebook_path="/tmp/not-a-notebook.txt" → expect rejection with "must be a Jupyter notebook (.ipynb)" |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| let llmContent: PartUnion; | ||
| if (result.isTruncated) { | ||
| if ( | ||
| result.isTruncated && |
Collaborator
There was a problem hiding this comment.
[Suggestion] The isTruncated && linesShown && originalLineCount !== undefined guard at this line correctly prevents notebook truncation headers from being added, but the notebook cacheable: false path (which flows through this guard's else branch and records lastReadCacheable: false) is not tested for the file_unchanged fast-path behavior. The existing notebook test only does a single read_file call.
Suggested change
| result.isTruncated && | |
| // Add test: read_file on .ipynb twice → both returns should be full notebook content, not file_unchanged placeholder |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const lineMatch = content.match(/\n( +)"/); | ||
| return { | ||
| indent: lineMatch?.[1]?.length, | ||
| trailingNewline: content.endsWith('\n'), |
Collaborator
There was a problem hiding this comment.
[Suggestion] inferNotebookJsonFormat's trailingNewline: true branch (when the raw notebook JSON ends with \n) is not directly tested. Existing tests only cover trailingNewline: false (compact JSON).
Suggested change
| trailingNewline: content.endsWith('\n'), | |
| // Add test: notebook JSON ending with \n → inferNotebookJsonFormat returns trailingNewline: true, serializeNotebook preserves it |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
previously requested changes
May 20, 2026
| }); | ||
| } | ||
|
|
||
| if (stats.isDirectory()) { |
Collaborator
There was a problem hiding this comment.
[Critical] checkPriorNotebookRead 中 stats.isDirectory() 分支（第 356-361 行）——当 notebook_path 解析为目录时，返回 TARGET_IS_DIRECTORY ——没有专门的测试覆盖。
如果用户以某种方式绕过 .ipynb 扩展名检查（例如，目录命名为 foo.ipynb），此路径依赖于未测试的代码来提供结构化的 TARGET_IS_DIRECTORY 错误。
Suggested change
| if (stats.isDirectory()) { | |
| // 在 notebook-edit.test.ts 中添加: | |
| // 在 tempDir 中创建一个名为 dir.ipynb 的目录，seedNotebookRead 它， | |
| // 然后断言 buildInvocation(...).execute() 返回 ToolErrorType.TARGET_IS_DIRECTORY |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException | undefined)?.code; | ||
| if (code === 'ENOENT') { | ||
| if (options.expectExisting) { |
Collaborator
There was a problem hiding this comment.
[Critical] checkPriorNotebookRead 中 ENOENT + expectExisting: true 分支（第 337-343 行）——当文件在两次 stat 检查之间消失时，返回 FILE_CHANGED_SINCE_READ ——缺乏测试。
这是 prepareEdit 中的双重检查模式。现有的 "tracks file history before the final freshness check" 测试覆盖了外部修改路径，而非完全消失。如果文件消失，应返回正确的错误类型以区分"文件已更改"与"文件不再存在"。
Suggested change
| if (options.expectExisting) { | |
| // 在 notebook-edit.test.ts 中添加: | |
| // seed 一个 notebook 读取，然后在 buildInvocation 之后但在 execute 之前删除文件路径。 | |
| // 验证错误类型为 ToolErrorType.FILE_CHANGED_SINCE_READ，消息包含 "disappeared" |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return { ok: true }; | ||
| } | ||
|
|
||
| return rejectNotebookPriorRead(notebookPath, 'stat-failed', { |
Collaborator
There was a problem hiding this comment.
[Critical] checkPriorNotebookRead 中非 ENOENT 的 stat 失败分支（第 348-353 行）——当 fs.promises.stat 抛出错误且错误码既不是 ENOENT 也不是 undefined 时，返回 PRIOR_READ_VERIFICATION_FAILED ——缺乏测试。
文件系统问题如 EACCES、EBUSY 或 NFS 故障需要正确的错误类型才能被调用者和指标理解。
Suggested change
| return rejectNotebookPriorRead(notebookPath, 'stat-failed', { | |
| // 在 notebook-edit.test.ts 中添加: | |
| // 模拟 fs.promises.stat，抛出带有 EACCES 代码的 ErrnoException， | |
| // seed 一个读取以确保文件存在，然后断言执行返回 ToolErrorType.PRIOR_READ_VERIFICATION_FAILED |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| async function checkPriorNotebookRead( | ||
| config: Config, | ||
| notebookPath: string, |
Collaborator
There was a problem hiding this comment.
[Suggestion] checkPriorNotebookRead 在 config.getFileReadCacheDisabled() 为 true 时直接返回 { ok: true }，完全跳过所有文件系统检查——包括 stat、isDirectory、isFile 以及 expectExisting 检查。
在缓存禁用模式下，notebook_edit 可以对已删除的文件静默创建新文件，或对目录/特殊文件进行操作。expectExisting: true 场景（文件在读后消失）在缓存禁用时毫无防护。
Suggested change
| notebookPath: string, | |
| // 即使缓存禁用，也应保留基本的文件系统检查： | |
| try { | |
| const stats = await fs.promises.stat(notebookPath); | |
| if (stats.isDirectory()) return rejectNotebookPriorRead(...); | |
| if (!stats.isFile()) return rejectNotebookPriorRead(...); | |
| } catch (error) { | |
| const code = (error as NodeJS.ErrnoException)?.code; | |
| if (code === 'ENOENT' && options.expectExisting) return rejectNotebookPriorRead(...); | |
| if (code === 'ENOENT') return { ok: true }; | |
| return rejectNotebookPriorRead(...); | |
| } | |
| if (config.getFileReadCacheDisabled()) return { ok: true }; | |
| // ... rest of cache checks |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| message, | ||
| type: ToolErrorType.FILE_WRITE_FAILURE, | ||
| }, | ||
| }; |
Collaborator
There was a problem hiding this comment.
[Suggestion] execute() 在写入阶段捕获错误并将其包装为 Error writing notebook: ${message}，但丢弃了 notebook_path 和 cell_id 上下文。
如果多个 notebook 编辑同时进行且其中一个失败，运维人员只有错误消息字符串可供关联——没有路径、cell ID 或编辑模式。
Suggested change
| }; | |
| llmContent: `Error writing notebook ${this.params.notebook_path} (${prepared.mode} cell ${prepared.editedCellId}): ${message}`, |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| function rejectNotebookPriorRead( | ||
| notebookPath: string, | ||
| reason: string, |
Collaborator
There was a problem hiding this comment.
[Suggestion] NotebookPriorReadDecision 类型与 priorReadEnforcement.ts 中的 PriorReadDecision 结构重复。两者都定义了 { ok: true } | { ok: false; type: ToolErrorType; rawMessage: string; displayMessage: string }。
未来如果有人向 PriorReadDecision 添加新字段（例如 retry-after 提示），NotebookPriorReadDecision 将会静默发散。
Suggested change
| reason: string, | |
| // 选项 1: 直接复用 PriorReadDecision | |
| import type { PriorReadDecision } from '../priorReadEnforcement.js'; | |
| // 选项 2: 提取共享接口 | |
| export interface RejectionDetail { | |
| type: ToolErrorType; | |
| rawMessage: string; | |
| displayMessage: string; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| override async execute(signal: AbortSignal): Promise<ToolResult> { | ||
| let prepared: PreparedNotebookEdit; | ||
| try { |
Collaborator
There was a problem hiding this comment.
[Suggestion] prepareEdit 被 getConfirmationDetails() 和 execute() 各调用一次，每次都会重新读取整个 notebook 文件并重新计算编辑结果。
确认时展示的 diff 基于第一次读取（T1），但实际写入使用第二次读取（T2）的内容。如果文件在 T1 和 T2 之间被外部修改，用户批准的 diff 与实际写入内容不匹配。execute() 中的 checkPriorNotebookRead 互锁会捕获 mtime/size 漂移并拒绝，但错误消息说 "modified since you last read it"——在用户刚批准确认后就收到过期读取错误，看起来很困惑。
Suggested change
| try { | |
| // 将 prepareEdit 结果缓存在 invocation 实例上： | |
| private preparedEditCache: PreparedNotebookEdit | undefined; | |
| private async prepareEdit(abortSignal: AbortSignal): Promise<PreparedNotebookEdit> { | |
| if (this.preparedEditCache) return this.preparedEditCache; | |
| // ... 现有逻辑 | |
| this.preparedEditCache = result; | |
| return result; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
|
Thanks @wenshao and @yiliang114 for the detailed review. I re-checked every Resolved functional criticals (verified in current code)
One issue worth fixing before merge
const match = content.match(/\n([ \t]+)"/);
const indent = match?.[1] ? (match[1].includes('\t') ? match[1] : match[1].length) : undefined;Suggest as follow-ups (non-blocking)
CI is green and there's one approval. Proposal: land the tab-indent fix, merge, and open a follow-up for the test-coverage + hardening items. Happy to push the tab-indent fix. 中文感谢 @wenshao 和 @yiliang114 的细致 review。我对照当前 head（ 已修复的功能性 critical（已在当前代码核实）
一个建议合并前修掉的问题
建议转为 follow-up（不阻塞）
CI 全绿，且已有一个 approval。建议：合入 tab 缩进修复后即可 merge，测试覆盖与防御性项另开 follow-up。我可以来提交 tab 缩进的修复。 |
pomelo-nwu
approved these changes
May 20, 2026
Dismissing to merge the complete NotebookEdit feature now. The remaining review items are accepted as follow-up work: one tab-indentation formatting fix and a few low-risk unit-test additions will be handled in a subsequent PR; other hardening suggestions are non-blocking follow-ups.
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
4 participants
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
Closes #2816.
Partially addresses #351.
Qwen Code already has structured notebook reading through
read_file:.ipynbfiles are parsed and rendered as ordered notebook cells with cell IDs, source, and outputs. This PR builds on that existing read path and adds the missing write/edit side:notebook_edit/NotebookEdittool for safe cell-level.ipynbedits..ipynbmutation out of generic textEdit/WriteFilepaths while allowing structured NotebookEdit operations after a fresh full notebook read.ModifiableDeclarativeTool.CommitAttributionService.read_fileprecedesnotebook_edit, and checks the final notebook JSON.Existing notebook read support
The current codebase already detects
.ipynbas a notebook file type and routes it through the notebook reader. That reader parses the JSON notebook and returns a model-friendly representation instead of exposing raw notebook JSON. The rendered form includes:cell-Nfallbacks,That behavior is intentionally read-only today. It gives the model enough context to understand and reference notebook cells, but it does not provide a structured mutation primitive. Using the existing text
Edit/WriteFiletools on raw.ipynbJSON is unsafe because a notebook is a structured document: editing the JSON text directly can corrupt cell metadata, outputs, execution counts, source array formatting, or notebook-level invariants.Design
This PR closes that gap by making NotebookEdit the structured counterpart to the existing notebook read path.
The intended workflow is:
read_fileon a.ipynbfile.read_filereturns the existing structured notebook rendering with cell IDs.notebook_edit, targeting one rendered cell by real cell id or displayedcell-Nfallback.The tool supports three edit modes:
replace: replace an existing cell's source and optionally convert its type.insert: insert a new cell after a target cell, or at the beginning when no target is provided.delete: remove an existing cell.NotebookEdit performs notebook-aware normalization during writes:
execution_count.qwen-cell-Nids, avoiding collisions with renderedcell-Nfallback ids.FileSystemService.Review feedback fixes
Latest follow-up review fixes:
read_filenow normalizes an emptypagesparameter to unset and rejects non-emptypagesfor.ipynbfiles, so notebook reads cannot be cached as partial merely because a PDF-only parameter was supplied.parseNotebook()now strips a leading UTF-8 BOM beforeJSON.parse, allowing BOM-prefixed notebooks to be read and used as the prior-read basis for NotebookEdit.This update addresses the requested changes from review:
read_file. A fallback-like real id such ascell-1no longer also makescell-0target the first cell by position.cell-Ndisplay id, NotebookEdit returnsINVALID_TOOL_PARAMSinstead of guessing.qwen-cell-Nnamespace and avoid all rendered display ids, so they cannot collide with positional fallback ids.ModifiableDeclarativeTool, so IDE/inline changes to the proposed notebook diff are applied instead of silently discarded.CommitAttributionService.recordEdit; user-modified proposals follow the existing Edit/WriteFile behavior and skip AI attribution.notebook_editin the expected default toolset.Prior-read and cache behavior
Notebook edits still need the same safety guarantee as text edits: the model should only mutate a file it has already read. However, the existing notebook reader returns a structured rendering, not the exact raw JSON bytes, so generic text edit prior-read semantics are too broad.
This PR keeps those concerns separated:
EditandWriteFilecontinue to reject notebooks as non-cacheable structured payloads.This preserves the existing read behavior while adding a controlled write path that matches the representation the model actually saw.
Testing
Latest unit-test coverage update (
1eaf0112a):Latest follow-up validation (
af0303a28):npm test --workspace=packages/core -- --run src/tools/read-file.test.ts src/utils/notebook.test.ts npm run typecheck --workspace=packages/core ./node_modules/.bin/eslint packages/core/src/tools/read-file.ts packages/core/src/utils/notebook.ts packages/core/src/tools/read-file.test.ts packages/core/src/utils/notebook.test.ts --max-warnings 0 ./node_modules/.bin/prettier --check packages/core/src/tools/read-file.ts packages/core/src/utils/notebook.ts packages/core/src/tools/read-file.test.ts packages/core/src/utils/notebook.test.ts git diff --check npm run build --workspace=packages/coreLatest docs-update validation (
8b1d7ebdd):Latest review-update validation (
d0e8e9adc):npm test --workspace=packages/core -- --run src/utils/notebook.test.ts src/tools/notebook-edit.test.ts src/utils/readManyFiles.test.ts src/tools/edit.test.ts src/tools/write-file.test.ts npm run typecheck --workspace=packages/core ./node_modules/.bin/eslint packages/core/src/tools/notebook-edit.ts packages/core/src/tools/notebook-edit.test.ts packages/core/src/utils/notebook.ts packages/core/src/utils/notebook.test.ts packages/core/src/utils/readManyFiles.ts packages/core/src/utils/readManyFiles.test.ts packages/core/src/tools/priorReadEnforcement.ts packages/core/src/tools/edit.test.ts packages/core/src/tools/write-file.test.ts --max-warnings 0 ./node_modules/.bin/prettier --check packages/core/src/tools/notebook-edit.ts packages/core/src/tools/notebook-edit.test.ts packages/core/src/utils/notebook.ts packages/core/src/utils/notebook.test.ts packages/core/src/utils/readManyFiles.ts packages/core/src/utils/readManyFiles.test.ts packages/core/src/tools/priorReadEnforcement.ts packages/core/src/tools/edit.test.ts packages/core/src/tools/write-file.test.ts git diff --check npm run build --workspace=packages/coreLatest review-update validation (
d3de6e005):npm test --workspace=packages/core -- --run src/tools/notebook-edit.test.ts src/utils/readManyFiles.test.ts src/tools/edit.test.ts src/tools/write-file.test.ts npm run typecheck --workspace=packages/core ./node_modules/.bin/eslint packages/core/src/tools/notebook-edit.ts packages/core/src/tools/notebook-edit.test.ts packages/core/src/utils/readManyFiles.ts packages/core/src/utils/readManyFiles.test.ts packages/core/src/tools/priorReadEnforcement.ts packages/core/src/tools/edit.test.ts packages/core/src/tools/write-file.test.ts --max-warnings 0 ./node_modules/.bin/prettier --check packages/core/src/tools/notebook-edit.ts packages/core/src/tools/notebook-edit.test.ts packages/core/src/utils/readManyFiles.ts packages/core/src/utils/readManyFiles.test.ts packages/core/src/tools/priorReadEnforcement.ts packages/core/src/tools/edit.test.ts packages/core/src/tools/write-file.test.ts npm run build --workspace=packages/coreLatest review-update validation (
5ad5cb4b2):npm test --workspace=packages/core -- --run src/utils/notebook.test.ts src/tools/notebook-edit.test.ts src/tools/read-file.test.ts npm run typecheck --workspace=packages/core ./node_modules/.bin/eslint packages/core/src/tools/notebook-edit.ts packages/core/src/tools/notebook-edit.test.ts packages/core/src/utils/notebook.ts packages/core/src/utils/notebook.test.ts packages/core/src/tools/read-file.ts packages/core/src/tools/read-file.test.ts packages/core/src/tools/tool-error.ts --max-warnings 0 ./node_modules/.bin/prettier --check packages/core/src/tools/notebook-edit.ts packages/core/src/tools/notebook-edit.test.ts packages/core/src/utils/notebook.ts packages/core/src/utils/notebook.test.ts packages/core/src/tools/read-file.ts packages/core/src/tools/read-file.test.ts packages/core/src/tools/tool-error.ts npm run build --workspace=packages/coreEarlier validation retained below.
Validated locally with:
Also ran
npm run test --workspace=packages/core. The originalconfig.test.tsfailures are fixed. The full parallel run later hit two unrelated 5s timeout flakes ingrep.test.tsandbackground-agent-resume.test.ts; both timed-out tests passed when rerun individually:Also ran
npx tsc --noEmit -p integration-tests/tsconfig.json; it currently fails on existing integration-test baseline type errors outside this new test file, includingchannel-plugin.test.ts,write_file.test.ts,@lydell/node-ptytypings, and terminal-capture DOM globals.Review focus / test areas
read_filerendering.NotebookEdit, includingEditmeta-category matching.Edit/WriteFilefrom mutating notebook JSON directly.cell-Nids, including ambiguous fallback-like ids.NotebookEdittool conversion.