feat(agent-block): Add support for Agent block - #358
Draft
tkislan wants to merge 36 commits into
Draft
Conversation
… instead of existing implementation
…ecution functions and improve ephemeral cell handling
…handling of ephemeral cells in serialization and decoration
Contributor
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end agent cell support for Deepnote notebooks: agent cells are detected and executed immediately via a new agent execution handler that serializes notebook context, streams agent events into cell outputs, and manages ephemeral cells (insert, execute, cleanup). Kernel execution is preserved for non-agent cells and only starts when kernel-targeted cells remain. Introduces agent block conversion, status-bar and decoration providers for agent/ephemeral cells, secret store helpers and commands for an OpenAI API key, excludes ephemeral cells from serialization, extends the renderer for ephemeral markdown, and adds comprehensive unit tests. Sequence Diagram(s)sequenceDiagram
participant Controller as NotebookController
participant Filter as ExecutionFilter
participant AgentExec as AgentCellExecutor
participant Serializer as ContextSerializer
participant EphemeralMgr as EphemeralManager
participant Kernel as KernelExecutor
Controller->>Filter: executeCells(request)
Filter->>Filter: split cells -> agentCells + kernelCells
alt agentCells exist
Filter->>AgentExec: executeAgentCell(agentCell)
AgentExec->>Serializer: serializeNotebookContext(notebook)
AgentExec->>AgentExec: clear outputs, start execution
AgentExec->>EphemeralMgr: removeEphemeralCellsForAgent(agentBlockId)
AgentExec->>AgentExec: executeAgentBlock (stream events)
AgentExec->>AgentExec: stream text/tool deltas -> update outputs
AgentExec->>EphemeralMgr: insertEphemeralCell(code/markdown)
EphemeralMgr->>Kernel: executeEphemeralCell(ephemeralCell)
Kernel-->>EphemeralMgr: execution result
EphemeralMgr-->>AgentExec: propagate outputs
AgentExec->>AgentExec: end execution (success/error)
end
alt kernelCells exist
Filter->>Kernel: start kernel and execute kernelCells
Kernel-->>Controller: kernel execution complete
else no kernelCells
Filter-->>Controller: return (no kernel started)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooks/deepnote/agentCellExecutionHandler.ts`:
- Around line 110-113: The code directly reads OPENAI_API_KEY from process.env
in agentCellExecutionHandler.ts (openAiToken = process.env.OPENAI_API_KEY) which
is unsafe for production; replace this direct env access with a secure secret
retrieval call (e.g., a new getOpenAiApiKey() that fetches from your secret
manager/credentials vault or from an injected secure config) and update callers
to inject the key instead of relying on process.env; ensure the secret is never
logged or included in error messages and keep the existing null-check/throw
behavior but reference the secure getter (getOpenAiApiKey) or injected parameter
in place of process.env.OPENAI_API_KEY.
- Around line 274-278: The success check in the return object of
agentCellExecutionHandler is too permissive—replace the current expression
`cell.executionSummary?.success !== false` with an explicit true check like
`cell.executionSummary?.success === true` (so only an explicit success is
reported; undefined/in-progress will not be treated as success); update the
return here (where `success`, `outputs:
cell.outputs.map(translateCellDisplayOutput)`, and `executionCount:
cell.executionSummary?.executionOrder ?? null` are constructed) to use that
strict equality.
In `@src/notebooks/deepnote/agentCellStatusBarProvider.ts`:
- Around line 69-71: The dispose() method currently uses an expression-bodied
arrow in this.disposables.forEach((d) => d.dispose()) which triggers the Biome
callback-return lint; change the callback to a block body or replace the forEach
with a for...of loop so the disposables are disposed without returning a
value—e.g., update dispose() to iterate over the disposables array and call
dispose() inside a statement block (reference: dispose method and disposables
property).
- Around line 142-149: getMaxIterations currently only enforces a lower bound;
add an upper-bound check so the returned value is an integer between
MIN_ITERATIONS and MAX_ITERATIONS (e.g., require value <= MAX_ITERATIONS). In
setMaxIterations replace permissive parseInt usage with strict integer
validation (use a full-match regex like /^\d+$/) and then parse with Number() so
inputs like "5.5" or "10abc" are rejected; after parsing ensure the numeric
value is an integer and within MIN_ITERATIONS..MAX_ITERATIONS before accepting
or falling back to DEFAULT_MAX_ITERATIONS. Update both occurrences in
setMaxIterations that currently call parseInt to use this strict validation and
range check, and reference the getMaxIterations and setMaxIterations functions
when making the change.
In `@src/notebooks/deepnote/converters/agentBlockConverter.unit.test.ts`:
- Around line 1-5: Reorder the imports so third-party modules are grouped
together and local imports come after: move the dedent import to be alongside
the other external imports (DeepnoteBlock, chai's assert, and vscode's
NotebookCellData/NotebookCellKind) and place the local AgentBlockConverter
import ('./agentBlockConverter') after that group; ensure the symbols
DeepnoteBlock, assert, NotebookCellData, NotebookCellKind, and dedent remain
imported and only the order changes to comply with the "third-party then local"
guideline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45a324a0-84a7-463d-903d-d15c32e2b30d
📒 Files selected for processing (16)
src/notebooks/controllers/vscodeNotebookController.tssrc/notebooks/deepnote/agentCellExecutionHandler.tssrc/notebooks/deepnote/agentCellExecutionHandler.unit.test.tssrc/notebooks/deepnote/agentCellStatusBarProvider.tssrc/notebooks/deepnote/agentCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/converters/agentBlockConverter.tssrc/notebooks/deepnote/converters/agentBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/notebooks/deepnote/deepnoteTestHelpers.tssrc/notebooks/deepnote/ephemeralCellDecorationProvider.tssrc/notebooks/deepnote/ephemeralCellStatusBarProvider.tssrc/notebooks/deepnote/ephemeralCellStatusBarProvider.unit.test.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/serviceRegistry.web.tssrc/renderers/client/markdown.ts
…ss helper - Introduced `createMockChildProcess` in `deepnoteTestHelpers.ts` for consistent mock process creation in tests. - Updated `DeepnoteLspClientManager` and `DeepnoteServerStarter` to include the mock process in server info. - Removed unnecessary `runtimeCoreServerInfo` from `ProjectContext` and adjusted related logic to use the new `serverInfo` structure. - Ensured all relevant tests are updated to reflect these changes, improving test reliability and maintainability.
- Added a warning log when no project context is found, preventing server stop attempts. - Updated the `stopServerForEnvironment` method to require a non-null project context, ensuring safer operation handling.
- Updated the DeepnoteServerStarter class to consistently use fileKey for managing pending operations and project contexts, improving clarity and reducing potential errors. - Adjusted logging messages to reflect the change, ensuring accurate information is logged during server operations.
- Eliminated the port allocation serialization logic from the DeepnoteServerStarter class, as it is now handled by the @deepnote/runtime-core's startServer method. - Updated related logging messages to reflect the changes in server startup processes. - Adjusted unit tests to focus on SQL environment variable gathering and lifecycle orchestration, removing tests related to port reservation.
…g improvements - Introduced a new `serverOutputByFile` map to track stdout and stderr outputs for each server instance, limiting the output length to improve performance and manageability. - Updated error handling in the server startup process to capture and report both stdout and stderr in case of failures, providing better diagnostics. - Adjusted the `dispose` method to ensure all internal states, including the new output tracking, are cleared appropriately. - Enhanced unit tests to validate the new output tracking functionality and ensure proper handling of cancellation errors.
- Modified the error reporting logic to ensure that stderr output is captured only when available, enhancing clarity in error messages. - This change aims to streamline the error handling process during server startup, providing more accurate feedback in case of failures.
…k/deepnote-agent-block
- Introduced `getOpenAiApiKey` function to retrieve the OpenAI API key from configuration, improving error handling when the key is not set. - Updated `executeAgentCell` and `executeEphemeralCell` functions to utilize the new API key retrieval method and handle cancellation tokens. - Enhanced `AgentCellStatusBarProvider` to validate max iterations using Zod schema, ensuring robust input handling and defaulting to safe values. - Added unit tests for new functionality and edge cases in both execution handling and status bar provider.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooks/deepnote/agentCellExecutionHandler.ts`:
- Around line 142-147: The onLog callback in agentCellExecutionHandler.ts
contains commented-out accumulation code and a TODO; either remove the dead code
or implement it: add an accumulated string variable in the enclosing scope, make
onLog async (or forward logs to an async helper), append incoming message to
accumulated, then call
execution.replaceOutputItems(NotebookCellOutputItem.text(accumulated), output)
to update the cell output; if you choose to drop it, delete the commented lines
and the TODO and keep only logger.info('Agent log', message). Reference: onLog
callback, accumulated variable, execution.replaceOutputItems,
NotebookCellOutputItem.text, and output.
- Around line 41-64: serializeNotebookContext instantiates a new
DeepnoteDataConverter on every call which is wasteful if called frequently;
modify serializeNotebookContext to use a shared or injected converter instance
instead of creating one per invocation (e.g., accept a DeepnoteDataConverter
parameter or read from a module-scoped singleton), and update callers to pass or
rely on the shared converter so convertCellToBlock usage inside
serializeNotebookContext reuses the same converter.
In `@src/notebooks/deepnote/agentCellExecutionHandler.unit.test.ts`:
- Around line 310-324: The test creates a CancellationTokenSource named
tokenSource and cancels it but never disposes it; update the test for 'returns
success false immediately when token is pre-cancelled' to ensure
tokenSource.dispose() is called after use (e.g., in a finally block or via
afterEach cleanup) so the CancellationTokenSource is properly disposed; locate
the tokenSource variable in this test and add the dispose call around
executeEphemeralCell(tokenSource.token) to clean up resources.
In `@src/notebooks/deepnote/agentCellStatusBarProvider.ts`:
- Line 27: MaxIterationsSchema currently only enforces a minimum via
MIN_ITERATIONS so values >100 slip through; update MaxIterationsSchema to also
enforce an upper bound (e.g., .max(100)) or reference a new constant like
MAX_ITERATIONS = 100 if you prefer a named limit, ensuring you use
z.coerce.number().int().min(MIN_ITERATIONS).max(MAX_ITERATIONS) (or .max(100))
to validate both ends; modify the schema definition where MaxIterationsSchema is
declared and add the MAX_ITERATIONS constant if not already present.
In `@src/notebooks/deepnote/ephemeralCellDecorationProvider.ts`:
- Around line 69-71: The dispose method on EphemeralCellDecorationProvider
currently iterates disposables with this.disposables.forEach((d) =>
d.dispose());—replace the forEach with a for...of loop to align with the pattern
used in AgentCellStatusBarProvider and to ensure proper synchronous disposal and
error handling: iterate over this.disposables using for (const d of
this.disposables) and call d.dispose() inside the loop (referencing the dispose
method and the disposables array to locate the change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5e9fa43e-8179-4408-8722-29b13fbca570
📒 Files selected for processing (10)
build/esbuild/build.tssrc/notebooks/deepnote/agentCellExecutionHandler.tssrc/notebooks/deepnote/agentCellExecutionHandler.unit.test.tssrc/notebooks/deepnote/agentCellStatusBarProvider.tssrc/notebooks/deepnote/agentCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/dataConversionUtils.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/ephemeralCellDecorationProvider.tssrc/notebooks/deepnote/ephemeralCellStatusBarProvider.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 1641-1646: The package.json setting "deepnote.agent.openAiApiKey"
stores the API key in plain settings; remove that configuration entry and
instead read/write the key via VS Code SecretStorage (use
context.secrets.get/set) like the existing apiAccess.ts usage; update the code
that previously read configuration for deepnote.agent.openAiApiKey to check
context.secrets.get("openAiApiKey") and, if missing, prompt the user with an
input dialog (and offer a command to set/clear the secret), and reuse the helper
functions or patterns from apiAccess.ts to centralize secret handling and
prompting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
- Added commands to set and clear the OpenAI API key, enhancing user interaction. - Introduced a new `deepnoteSecretStore` module for managing secrets, including functions to get, set, and clear the OpenAI API key. - Updated `agentCellExecutionHandler` to utilize the new secret management functions, improving error handling when the API key is not set. - Enhanced unit tests to cover the new secret management functionality and ensure robust error handling.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/notebooks/deepnote/agentCellStatusBarProvider.ts (1)
207-224: 🛠️ Refactor suggestion | 🟠 MajorReuse
MaxIterationsSchemafor consistent validation.
parseIntis lenient:"5.5"becomes5,"10abc"becomes10. The existing Zod schema handles this properly and is already used ingetMaxIterations.,
♻️ Suggested fix
validateInput: (value) => { - const num = parseInt(value, 10); - if (isNaN(num) || !Number.isInteger(num)) { - return l10n.t('Please enter a whole number'); - } - if (num < MIN_ITERATIONS || num > MAX_ITERATIONS) { + const result = MaxIterationsSchema.safeParse(value); + if (!result.success) { return l10n.t('Value must be between {0} and {1}', MIN_ITERATIONS, MAX_ITERATIONS); } return undefined; }- const newValue = parseInt(input, 10); + const newValue = MaxIterationsSchema.parse(input); if (newValue === currentValue) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/agentCellStatusBarProvider.ts` around lines 207 - 224, The validateInput logic should reuse the existing MaxIterationsSchema instead of using parseInt; replace the parseInt/isNaN checks in validateInput with MaxIterationsSchema.safeParse(input) (or parse and catch) and return l10n.t(...) on failure, ensuring the schema enforces integer-only and range constraints consistent with MIN_ITERATIONS and MAX_ITERATIONS; after the prompt returns, set newValue from the validated schema result (the parsed numeric value) rather than calling parseInt again; refer to validateInput, MaxIterationsSchema, getMaxIterations, MIN_ITERATIONS, MAX_ITERATIONS, and newValue when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooks/deepnote/agentCellExecutionHandler.ts`:
- Around line 240-245: The code currently assumes workspace.applyEdit(edit)
succeeded and returns insertIndex blindly; change it to check the boolean result
of await workspace.applyEdit(edit) and verify the notebook now contains the
inserted cell (e.g. notebook.cellCount > insertIndex or try
notebook.cellAt(insertIndex) exists). If applyEdit returns false or the
verification fails, throw an Error (or return a sentinel/failure value as per
project convention) instead of returning insertIndex so callers won't operate on
an invalid index; use the same local symbols edit, insertIndex, notebook,
WorkspaceEdit, NotebookEdit.insertCells and workspace.applyEdit to locate and
implement the checks.
- Around line 136-138: The handler onAgentEvent currently logs the full
serialized AgentStreamEvent (logger.info('Agent event', JSON.stringify(event)))
which can leak user/tool content and bloat logs; change this to log only minimal
metadata such as event.type, any safe IDs or timestamps, and the transition
detected using lastAgentEventType (e.g., logger.info('Agent event', { type:
event.type, prevType: lastAgentEventType, timestamp: ... })) and remove
JSON.stringify(event) so no full payload is written to logs.
- Around line 264-283: The code rejects completionDeferred when
token.isCancellationRequested but still proceeds to run
commands.executeCommand('notebook.cell.execute'), allowing work after
cancellation; update the handler (around token, completionDeferred,
CancellationError and before commands.executeCommand) to short-circuit: if token
&& token.isCancellationRequested (or if completionDeferred has already been
rejected/settled) then clear the timeout, dispose any disposables, and
return/throw so commands.executeCommand is not invoked; ensure the same
early-exit path is taken when token.onCancellationRequested fires so cancelled
executions never call notebook.cell.execute.
In `@src/notebooks/deepnote/agentCellExecutionHandler.unit.test.ts`:
- Around line 366-384: The test for executeEphemeralCell should also assert that
no execution request was sent when the token is pre-cancelled: after calling
executeEphemeralCell with the pre-cancelled CancellationTokenSource, add an
assertion that notebook.cell.execute was never invoked (i.e., verify/expect the
mocked notebook cell execution method did not get called), and keep the existing
assertion on the returned result; refer to executeEphemeralCell,
mockedVSCodeNamespaces.commands.executeCommand and the notebook.cell.execute
mock when adding this check.
In `@src/notebooks/deepnote/ephemeralCellDecorationProvider.ts`:
- Around line 117-123: The current loop in ephemeralCellDecorationProvider
builds a Range per line (lineRanges) and calls
editor.setDecorations(this.ephemeralDecorationType, lineRanges), which is
wasteful; replace it by creating a single full-cell Range spanning from the
start of the first line to the end of the last line (use
editor.document.lineAt(0).range.start and
editor.document.lineAt(editor.document.lineCount - 1).range.end) and pass an
array with that single Range to
editor.setDecorations(this.ephemeralDecorationType, [fullRange]) so you avoid
allocating per-line Range objects while preserving the same decoration coverage.
---
Duplicate comments:
In `@src/notebooks/deepnote/agentCellStatusBarProvider.ts`:
- Around line 207-224: The validateInput logic should reuse the existing
MaxIterationsSchema instead of using parseInt; replace the parseInt/isNaN checks
in validateInput with MaxIterationsSchema.safeParse(input) (or parse and catch)
and return l10n.t(...) on failure, ensuring the schema enforces integer-only and
range constraints consistent with MIN_ITERATIONS and MAX_ITERATIONS; after the
prompt returns, set newValue from the validated schema result (the parsed
numeric value) rather than calling parseInt again; refer to validateInput,
MaxIterationsSchema, getMaxIterations, MIN_ITERATIONS, MAX_ITERATIONS, and
newValue when making these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e1298466-ae5e-4a9e-aaf3-1c4f03b06f10
📒 Files selected for processing (8)
package.jsonpackage.nls.jsonsrc/notebooks/deepnote/agentCellExecutionHandler.tssrc/notebooks/deepnote/agentCellExecutionHandler.unit.test.tssrc/notebooks/deepnote/agentCellStatusBarProvider.tssrc/notebooks/deepnote/deepnoteSecretStore.tssrc/notebooks/deepnote/deepnoteSecretStore.unit.test.tssrc/notebooks/deepnote/ephemeralCellDecorationProvider.ts
Comment on lines
+366
to
+384
| test('returns success false immediately when token is pre-cancelled', async () => { | ||
| const cell = createMockCell({ index: 0 }); | ||
| const tokenSource = new CancellationTokenSource(); | ||
| tokenSource.cancel(); | ||
|
|
||
| when(mockedVSCodeNamespaces.commands.executeCommand(anything(), anything())).thenResolve(); | ||
|
|
||
| try { | ||
| const result = await executeEphemeralCell(cell, tokenSource.token); | ||
|
|
||
| expect(result).to.deep.equal({ | ||
| success: false, | ||
| outputs: [], | ||
| executionCount: null | ||
| }); | ||
| } finally { | ||
| tokenSource.dispose(); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Strengthen cancellation test by asserting no execution command is sent.
The current assertion checks return value only. It should also verify notebook.cell.execute is not invoked for a pre-cancelled token.
💡 Proposed test addition
import { anything, capture, instance, mock, reset, when } from 'ts-mockito';
+import { verify } from 'ts-mockito';
@@
test('returns success false immediately when token is pre-cancelled', async () => {
@@
try {
const result = await executeEphemeralCell(cell, tokenSource.token);
expect(result).to.deep.equal({
success: false,
outputs: [],
executionCount: null
});
+ verify(mockedVSCodeNamespaces.commands.executeCommand(anything(), anything())).never();
} finally {
tokenSource.dispose();
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notebooks/deepnote/agentCellExecutionHandler.unit.test.ts` around lines
366 - 384, The test for executeEphemeralCell should also assert that no
execution request was sent when the token is pre-cancelled: after calling
executeEphemeralCell with the pre-cancelled CancellationTokenSource, add an
assertion that notebook.cell.execute was never invoked (i.e., verify/expect the
mocked notebook cell execution method did not get called), and keep the existing
assertion on the returned result; refer to executeEphemeralCell,
mockedVSCodeNamespaces.commands.executeCommand and the notebook.cell.execute
mock when adding this check.
Comment on lines
+117
to
+123
| const lineRanges: Range[] = []; | ||
| for (let i = 0; i < editor.document.lineCount; i++) { | ||
| const line = editor.document.lineAt(i); | ||
| lineRanges.push(line.range); | ||
| } | ||
|
|
||
| editor.setDecorations(this.ephemeralDecorationType, lineRanges); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use a single full-cell range instead of per-line ranges.
This loop allocates one Range per line on every refresh. For long cells, that is avoidable overhead.
💡 Proposed refactor
- const lineRanges: Range[] = [];
- for (let i = 0; i < editor.document.lineCount; i++) {
- const line = editor.document.lineAt(i);
- lineRanges.push(line.range);
- }
-
- editor.setDecorations(this.ephemeralDecorationType, lineRanges);
+ if (editor.document.lineCount === 0) {
+ editor.setDecorations(this.ephemeralDecorationType, []);
+ continue;
+ }
+
+ const first = editor.document.lineAt(0).range.start;
+ const last = editor.document.lineAt(editor.document.lineCount - 1).range.end;
+ editor.setDecorations(this.ephemeralDecorationType, [new Range(first, last)]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notebooks/deepnote/ephemeralCellDecorationProvider.ts` around lines 117 -
123, The current loop in ephemeralCellDecorationProvider builds a Range per line
(lineRanges) and calls editor.setDecorations(this.ephemeralDecorationType,
lineRanges), which is wasteful; replace it by creating a single full-cell Range
spanning from the start of the first line to the end of the last line (use
editor.document.lineAt(0).range.start and
editor.document.lineAt(editor.document.lineCount - 1).range.end) and pass an
array with that single Range to
editor.setDecorations(this.ephemeralDecorationType, [fullRange]) so you avoid
allocating per-line Range objects while preserving the same decoration coverage.
Contributor
Author
|
@coderabbitai pause |
Contributor
✅ Action performedReviews paused. |
Resolve conflicts against main: - package.json / package-lock.json: take main's dependency versions. The branch's only addition (@deepnote/runtime-core) is already on main at a higher version via #355. - deepnoteServerStarter.unit.test.ts: take main's version. The branch never touched this file in the agent-block commits; the conflict was entirely stale runtime-core content that main already carries as the squash of #355. - deepnoteKernelAutoSelector.node.ts: keep the agent-cell import alongside main's getNotebookKey, and drop IDeepnoteInitNotebookRunner (main removed its last usage). - deepnoteKernelAutoSelector.node.unit.test.ts: take main's ServerHandleRegistry import; createMockChildProcess is already on main. - deepnoteSerializer.unit.test.ts: keep the ephemeral-cell exclusion test, nested inside the serializeNotebook suite that main extended, and update it for main's storeOriginalProject(projectId, notebookId, project) signature. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bz32QuAABW7Xk9wKXc7Sk8
PR #358 pinned @deepnote/runtime-core ^0.2.0, which exports no agent API at all, so the branch never compiled on its own — it was written against an unreleased build. Merging main moves the dependency to ^0.4.0, which does ship the agent API but with a tightened contract: - serializeNotebookContextFromBlocks() no longer accepts a null notebookName, so pass the document's deepnoteNotebookName (empty string when absent). - The addMarkdownBlock / addAndExecuteCodeBlock tool callbacks now return a string rather than a {success} object. That string is the tool result fed back to the model, so mirror the wording runtime-core uses in its own ExecutionEngine implementation of the same tools: the agent now sees the executed cell's real output instead of only whether it succeeded. extractOutputsText() reads a stream output's `text` only when it is a string, but translateCellDisplayOutput() emits nbformat's multiline array form, so normalize before extracting — otherwise every print() from an ephemeral cell would be dropped from the tool result. Also teach the runtime-core test mock about the agent exports. The mock is a main-only file the branch never had, and the ESM loader swaps it in for the whole module, so without them the import binding fails and no unit test in the suite can load. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bz32QuAABW7Xk9wKXc7Sk8
z.coerce.number() turns true into 1, which then satisfies .int().min(1).max(100), so a boolean deepnote_max_iterations was accepted as an iteration count of 1 instead of falling back to the default of 20 — contradicting the test that already documented the intended behaviour. Pre-existing on the branch rather than a merge regression; it only surfaced now because the branch compiles for the first time, so its tests could finally run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bz32QuAABW7Xk9wKXc7Sk8
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #358 +/- ##
===========================
===========================
🚀 New features to boost your workflow:
|
… any The ephemeral-cell markdown wrapper reached for `any` in four places, which @typescript-eslint/no-explicit-any rejects — the rule is 'error' repo-wide and is only relaxed for tests and *.d.ts, so this failed CI lint. None of the casts were necessary. RendererApi exposes extension hooks through an index signature, so extendMarkdownIt already arrives as `unknown` and just needs narrowing to a call signature. markdown-it itself ships no types and is only a transitive dependency, so describe the small surface this renderer actually touches rather than pulling in @types/markdown-it. Narrowing on `typeof extendMarkdownIt === 'function'` also replaces a bare truthiness check on the renderer, so a markdown renderer without the hook no longer throws. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bz32QuAABW7Xk9wKXc7Sk8
Remediation for 13 findings from a merged, adversarially verified review of this branch. Grouped here because the changes interleave across the same files; each is independently described below. Correctness - Model picker read/wrote `deepnote_model`, but execution and the file schema use `deepnote_agent_model` — the picker was inert in both directions and persisted a key no consumer reads into the .deepnote file. Route both sides through one constant, and write the literal 'auto' rather than deleting the key: convertCellToBlock does not re-run the zod schema, so a missing key reaches runtime-core as undefined and is passed to openai() as a model name. Drop 'sonnet' from the options — createOpenAI gets no baseURL here, so an Anthropic model name 404s. - Remove the max-iterations control. Nothing consumes `deepnote_max_iterations`: executeAgentBlock hardcodes maxTurns = 10 and AgentBlockContext exposes no turn limit, so the UI advertised a default of 20 and a 1-100 range over a setting that changed nothing. - Rich outputs reached the agent comma-mangled. translateCellDisplayOutput splits `text/*` into nbformat line arrays for execute_result/display_data too, and extractOutputText stringifies them with String(...), which joins with commas — so every df.head() the agent read had a comma glued to the start of each line after the first. Only stream text was being joined. - insertEphemeralCell ignored applyEdit's result and returned a bare index. cellAt clamps rather than throwing, so a rejected edit or a concurrent structural change handed back a pre-existing user cell, which the agent then executed and reported as its own result. Check the boolean and resolve the inserted cell by __deepnoteBlockId instead. - Both execute handlers ran every agent cell before any kernel cell, regardless of document order, so agent-generated code executed against a kernel that had not run the setup cells above it. Walk in document order. Reliability - executeEphemeralCell rejected the completion deferred on an already cancelled token but still dispatched notebook.cell.execute, so the kernel ran the generated code after the user cancelled. Throw before dispatch. Also propagate the failure reason instead of collapsing cancellation, timeout and command failure alike into "(no output)", which invited the agent to retry. - Run All aborted silently when an agent cell had already deleted the ephemeral cells queued alongside it: createNotebookCellExecution throws for a removed cell, and nothing caught it. Filter out cells whose index is -1. - Acquire the OpenAI key before the destructive ephemeral cleanup. It prompts and throws on dismissal, so cancelling the prompt destroyed the previous run's results for a run that never started. Security - The placeholder execute handler had no workspace-trust check, while the real controller did. Agent blocks can spawn MCP servers declared in the project file, so gate both paths — the manifest already promises cell execution is unsupported in untrusted workspaces. - Pass project-level `mcpServers` from project.settings, matching what the CLI's ExecutionEngine provides. The empty array was a stub from the initial implementation; it dropped the project-level tier (declared in the file, intended to be configured) while runtime-core still merged in the block-level tier, which is the invisible one. Performance - Stream agent output as incremental stdout items rather than re-encoding and re-sending the whole transcript on every token: that was O(n^2) bytes across the extension-host boundary, and since runtime-core awaits onAgentEvent inside its stream loop the cost was added to the run's wall clock. Maintainability and tests - Move isAgentCell next to isEphemeralCell in dataConversionUtils so the status bar provider stops duplicating it and no longer needs the runtime-core-backed handler module in its import graph. Delete the getOpenAiApiKey wrapper, whose name collided with a differently-behaving export one import away. - Log stream events at trace with type only. At info they wrote model text and reasoning into the user-visible output channel on every token. Keep the explicit stack log: logger.error(msg, error) only renders the stack for errors branded isJupyterError. - createMockNotebook now reads through a caller-supplied cells array, so tests can exercise the insert/remove/ordering logic that previously had no coverage at all. Adds regression tests for the output mangling, the cancelled key prompt, failed inserts, cross-agent deletion and delta streaming; each was run against the unfixed code and observed failing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LaXoas2nAryMghEk3FQS2o
…n gaps Four defects from a review pass over this branch, each with a regression test written against the unfixed code first. - The placeholder controller's execute handler passed its own captured controller to executeAgentCell after environment setup had already disposed and deselected it, so the agent cell was skipped with nothing but a log line. Use the real controller that owns the notebook by then. - executeEphemeralCell awaited the dispatch before the completion deferred, so the timeout could not end a run whose command never resolved, and a rejection arriving in between was reported as unhandled. Wait on both together. - A rejected ephemeral cleanup edit was only logged, leaving the previous run's cells in the notebook context sent to the model and in Run All's kernel batch. Fail the run instead. - Project integrations were never passed to executeAgentBlock, so runtime-core dropped the integration IDs and dntk.execute_sql instructions from its system prompt entirely. Also drop the isAgentCell re-export from the handler. Both consumers imported executeAgentCell alongside it, so it bought nothing and only made it easy to pull @deepnote/runtime-core into a module graph that has no need for it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LaXoas2nAryMghEk3FQS2o
isAgentCell lives in dataConversionUtils, not the execution handler; its tests only sat in the handler's suite because the handler used to re-export it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LaXoas2nAryMghEk3FQS2o
Runs an agent block end to end against a stand-in OpenAI API, covering the path no test reached before: the agent generating Python via add_code_block, the extension executing it on a real kernel, and the kernel's output going back to the agent. The mock is @copilotkit/aimock, fetched with a pinned npx rather than added to package.json — it declares jest and vitest as peers, and resolving those against this tree forces overrides that would outlive the test. The scripted legs match on toolResultContains rather than a request counter, so the agent can only advance if the extension really ran the generated code and fed the real output back; under --strict a broken round-trip fails loudly instead of taking a different path. Being pure request-shape predicates, they also replay correctly on a Mocha retry, which does not re-run `before`. OPENAI_BASE_URL is set at the spec's module scope: ExTester launches VS Code from a root beforeAll and the extension host inherits its environment at spawn time, so a hook is too late, while Mocha loads spec files before running any hook. Without it runtime-core falls back to the real api.openai.com, so startMockOpenAiServer refuses to run when it is unset. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LaXoas2nAryMghEk3FQS2o
Mocha retries the test but not `before`/`after`, so a server started once outlived a failed attempt and still held the port when the retry began — where the pre-flight check rejected it as a leftover, failing the retry for a different reason than the original and losing the real signal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LaXoas2nAryMghEk3FQS2o
…h attempt The scripted legs are what a test is about, so they belong with it rather than in a shared hook — a second test would script different ones. The release runs on both sides of the test, not just after: Mocha retries the test but not before/after, so a server surviving a failed attempt still holds the port when the retry starts, where the pre-flight check rejects it as a leftover and the retry fails for a reason unrelated to the original. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LaXoas2nAryMghEk3FQS2o
`require` is resolved by the mocha CLI's handleRequires, not by the Mocha constructor. ExTester hands this config straight to `new Mocha(config)`, which reads `rootHooks` and ignores `require` — so rootHooks.js was never loaded and the between-test toast dismissal it defines has never run for any suite. Resolving the module here and passing `rootHooks` works under both the programmatic API and the CLI. It also means a missing build fails at config load rather than silently dropping the hooks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LaXoas2nAryMghEk3FQS2o
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
Behavior Changes
Tests