feat(runtime-core): Refactor run agent block, and extract some implementation into the CLI, so that runtime can be reused in vscode extension#342
Open
tkislan wants to merge 37 commits into
Open
Conversation
…entation into the CLI, so that runtime can be reused in vscode extension
Contributor
📝 WalkthroughWalkthroughThis change moves notebook and kernel mutation out of executeAgentBlock: the execution engine now constructs an AgentBlockContext (including openAiToken, mcpServers, serialized notebookContext, and helper functions addAndExecuteCodeBlock/addMarkdownBlock) and manages inserting/executing blocks and collecting their outputs. executeAgentBlock no longer mutates the notebook or returns added block IDs/outputs — it returns only finalOutput and may stream events (onAgentEvent is awaitable). Helpers for serializing notebook context and attaching collected outputs were added and exported. Sequence Diagram(s)sequenceDiagram
participant ExecEngine as Execution Engine
participant Agent as AgentHandler
participant OpenAI as OpenAI API
participant Kernel as Kernel
participant Notebook as Notebook Store
ExecEngine->>ExecEngine: build AgentBlockContext (openAiToken, mcpServers, notebookContext, addAndExecuteCodeBlock, addMarkdownBlock)
ExecEngine->>Agent: executeAgentBlock(context, prompt)
Agent->>OpenAI: stream completions (uses context.openAiToken)
loop streaming
OpenAI-->>Agent: partial tokens / events
Agent->>ExecEngine: context.onAgentEvent(streamEvent) (awaited)
alt request to insert code
Agent->>ExecEngine: context.addAndExecuteCodeBlock(code)
ExecEngine->>Notebook: create code block
ExecEngine->>Kernel: kernel.execute(code)
Kernel-->>ExecEngine: execution outputs
ExecEngine->>ExecEngine: collect block outputs
ExecEngine->>ExecEngine: emit onOutput / onBlockDone
end
alt request to insert markdown
Agent->>ExecEngine: context.addMarkdownBlock(content)
ExecEngine->>Notebook: create markdown block
ExecEngine-->>Agent: acknowledge
end
end
Agent-->>ExecEngine: finalOutput
ExecEngine->>ExecEngine: finalize collected outputs (serialize/attach) and finish
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 3/5 reviews remaining, refill in 21 minutes and 26 seconds. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #342 +/- ##
==========================================
+ Coverage 82.24% 82.80% +0.56%
==========================================
Files 144 144
Lines 5868 5869 +1
Branches 1096 1095 -1
==========================================
+ Hits 4826 4860 +34
+ Misses 1042 1009 -33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime-core/src/execution-engine.test.ts (1)
833-847: 🛠️ Refactor suggestion | 🟠 MajorCover the new agent helper path with one real runtime test.
Because
executeAgentBlockis mocked at the module boundary, these cases never exerciseAgentBlockContext.addAndExecuteCodeBlock,AgentBlockContext.addMarkdownBlock, or the asynconAgentEventflow added by this refactor. A regression in inserted-block reporting would still pass here.As per coding guidelines, "Create comprehensive tests for all new features using Vitest" and "Test edge cases, error handling, and special characters in test files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/execution-engine.test.ts` around lines 833 - 847, Add one real runtime test that does not mock executeAgentBlock at the module boundary so the new helper path is exercised: undo or avoid the module mock for executeAgentBlock (remove or restore mockExecuteAgentBlock via vi.unmock/vi.importActual) and call the actual execution flow with the AGENT_FIXTURE so AgentBlockContext.addAndExecuteCodeBlock, AgentBlockContext.addMarkdownBlock, and the async onAgentEvent flow run; then assert that inserted block reporting (addedBlockIds/blockOutputs/finalOutput and any events) matches expected values. Ensure the test triggers error/edge cases and special characters from the fixture so the real helper logic and event handling are covered rather than returning a mocked result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/runtime-core/src/agent-handler.ts`:
- Around line 18-26: The AgentBlockContext.onAgentEvent type currently forces
Promise<void> which prevents sync handlers and leaves async errors unobserved;
change the onAgentEvent signature to accept void | Promise<void> in the
AgentBlockContext interface and then update the stream processing loop to await
the result of each call to onAgentEvent for events like "text_delta",
"reasoning_delta", "tool_called", and "tool_output" so both sync and async
handlers are supported and errors are propagated; also update the corresponding
ExecutionOptions type in execution-engine.ts to match the new void |
Promise<void> signature so callers compile.
In `@packages/runtime-core/src/execution-engine.ts`:
- Around line 249-287: addAndExecuteCodeBlock currently only records
outputs/executionCount and loses success/error details; change it to capture and
persist the full execution result from kernel.execute (including success,
outputs, executionCount, and any error or computed outputText) into blockOutputs
and collectedOutputs so that a failing result (result.success === false) is not
treated as success and thrown exceptions are converted to a unified result
object and passed to the same flow; specifically update the logic around
kernel.execute, the construction pushed to blockOutputs and collectedOutputs,
and the returned value so both tool responses and host callbacks (e.g.,
onBlockDone) receive the identical full result object rather than just
outputs/executionCount.
---
Outside diff comments:
In `@packages/runtime-core/src/execution-engine.test.ts`:
- Around line 833-847: Add one real runtime test that does not mock
executeAgentBlock at the module boundary so the new helper path is exercised:
undo or avoid the module mock for executeAgentBlock (remove or restore
mockExecuteAgentBlock via vi.unmock/vi.importActual) and call the actual
execution flow with the AGENT_FIXTURE so
AgentBlockContext.addAndExecuteCodeBlock, AgentBlockContext.addMarkdownBlock,
and the async onAgentEvent flow run; then assert that inserted block reporting
(addedBlockIds/blockOutputs/finalOutput and any events) matches expected values.
Ensure the test triggers error/edge cases and special characters from the
fixture so the real helper logic and event handling are covered rather than
returning a mocked result.
🪄 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: 5a7edfc5-3620-40b0-95cd-66d1fc17f014
📒 Files selected for processing (4)
packages/cli/src/commands/run.tspackages/runtime-core/src/agent-handler.tspackages/runtime-core/src/execution-engine.test.tspackages/runtime-core/src/execution-engine.ts
…reusable functions
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime-core/src/agent-handler.ts (1)
237-239: 🧹 Nitpick | 🔵 TrivialConsider explicit undefined handling.
JSON.stringify(undefined)returnsundefined. Current nullish coalescing works, but explicit check is clearer.💡 Optional clarity improvement
- const outputStr = typeof toolOutput === 'string' ? toolOutput : (JSON.stringify(toolOutput) ?? '') + const outputStr = toolOutput === undefined ? '' : typeof toolOutput === 'string' ? toolOutput : JSON.stringify(toolOutput)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/agent-handler.ts` around lines 237 - 239, The code that builds outputStr in agent-handler.ts should explicitly handle undefined instead of relying on JSON.stringify(undefined) returning undefined; update the logic in the block around toolOutput / outputStr (variables toolOutput and outputStr, and the context.onAgentEvent call) so that if toolOutput is undefined you set outputStr to '' (empty string), otherwise if it's a string keep it, and otherwise set it to JSON.stringify(toolOutput); ensure the resulting outputStr is always a string before calling context.onAgentEvent with type 'tool_output'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/runtime-core/src/agent-handler.ts`:
- Line 104: Remove the leftover commented-out line "const outputs =
collectedOutputs.get(block.id)" in the agent handler; locate the commented
statement near the logic that references collectedOutputs and block.id (in the
function handling agent blocks) and delete it so no dead commented code remains,
ensuring there are no other unused commented variables left after the refactor.
- Around line 69-76: The map callback in blocks.map is shadowing and redundantly
re-reading collectedOutputs.get(block.id): remove the inner const outputs =
collectedOutputs.get(block.id) and the nested if, use the single outputs
variable obtained once (const outputs = collectedOutputs.get(block.id)) and,
when outputs is non-null and outputs.outputs.length > 0, return { ...block,
outputs: outputs.outputs }; otherwise return the original block — update the
callback in agent-handler.ts accordingly to eliminate the unreachable shadowed
declaration.
---
Outside diff comments:
In `@packages/runtime-core/src/agent-handler.ts`:
- Around line 237-239: The code that builds outputStr in agent-handler.ts should
explicitly handle undefined instead of relying on JSON.stringify(undefined)
returning undefined; update the logic in the block around toolOutput / outputStr
(variables toolOutput and outputStr, and the context.onAgentEvent call) so that
if toolOutput is undefined you set outputStr to '' (empty string), otherwise if
it's a string keep it, and otherwise set it to JSON.stringify(toolOutput);
ensure the resulting outputStr is always a string before calling
context.onAgentEvent with type 'tool_output'.
🪄 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: 7e67686a-1083-41b3-bf9a-a0ba5b6b0e4e
📒 Files selected for processing (2)
packages/runtime-core/src/agent-handler.tspackages/runtime-core/src/index.ts
…ution and ensure proper handling of event callbacks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime-core/src/agent-handler.ts (1)
171-203:⚠️ Potential issue | 🟠 MajorCover MCP startup with the same cleanup path.
createMCPClient(...)andclient.tools()still run before the currenttry/finally. If one server fails after earlier clients started, those stdio transports stay open. Track clients incrementally inside the cleanup scope.Also applies to: 222-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/agent-handler.ts` around lines 171 - 203, The MCP clients and their tool enumeration are created outside the try/finally, so a failure partway leaves open stdio transports; move the whole client startup and tools collection into the protected scope and track started clients incrementally (e.g., push each created client from mergedMcpConfig.map/createMCPClient into a local startedClients array as you create them), then in the finally iterate startedClients and call the client's shutdown method (client.close() or client.dispose() as available) to ensure Experimental_StdioMCPTransport stdio pipes are cleaned up; also ensure addCodeBlockTool and addMarkdownBlockTool registration (context.addAndExecuteCodeBlock / context.addMarkdownBlock) happens after clients/tools are successfully created so partial startups are cleaned up consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/runtime-core/src/agent-handler.ts`:
- Around line 69-73: The mapping currently only overrides a block when
collectedOutputs.get(block.id) exists and has outputs.length > 0, which allows
stale block.outputs to persist when an entry exists with outputs: []; change the
condition so any presence of an entry in collectedOutputs for block.id (i.e.,
outputs != null / outputs !== undefined) causes the block to be returned with
outputs set to outputs.outputs (including the empty array); leave the original
block untouched only when there is no map entry for that id (refer to the
blocks.map callback, collectedOutputs.get(block.id), and the outputs.outputs
property).
- Around line 15-23: Update the callback result types so successful and failed
tool returns are serializable and include the execution output: replace
AddAndExecuteCodeBlockResult with a discriminated union where success: true
includes a required executionText (string) carrying stdout/stderr/results, and
success: false includes a SerializableError type (e.g., { message: string;
name?: string; stack?: string; code?: string }) instead of raw Error; do the
same for AddMarkdownBlockResult (e.g., success true includes renderedText or
content string). Then update the AgentBlockContext signatures for
addAndExecuteCodeBlock and addMarkdownBlock to return the new result types so
the agent always receives serializable output and meaningful error information.
---
Outside diff comments:
In `@packages/runtime-core/src/agent-handler.ts`:
- Around line 171-203: The MCP clients and their tool enumeration are created
outside the try/finally, so a failure partway leaves open stdio transports; move
the whole client startup and tools collection into the protected scope and track
started clients incrementally (e.g., push each created client from
mergedMcpConfig.map/createMCPClient into a local startedClients array as you
create them), then in the finally iterate startedClients and call the client's
shutdown method (client.close() or client.dispose() as available) to ensure
Experimental_StdioMCPTransport stdio pipes are cleaned up; also ensure
addCodeBlockTool and addMarkdownBlockTool registration
(context.addAndExecuteCodeBlock / context.addMarkdownBlock) happens after
clients/tools are successfully created so partial startups are cleaned up
consistently.
🪄 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: 84e86e00-c5ff-4e1a-8d96-c6677c2b1b9e
📒 Files selected for processing (1)
packages/runtime-core/src/agent-handler.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/runtime-core/src/agent-handler.ts (2)
173-184: 🧹 Nitpick | 🔵 TrivialMCP transport failures will reject the entire agent run.
If any MCP server fails to connect (bad command, missing binary),
Promise.allrejects and no cleanup runs. Consider graceful degradation or wrapping each client creation.♻️ Possible approach
- const mcpClients = await Promise.all( - mergedMcpConfig.map(s => + const mcpClientResults = await Promise.allSettled( + mergedMcpConfig.map(s => createMCPClient({ transport: new Experimental_StdioMCPTransport({ command: s.command, args: s.args, env: resolveEnvVars(s.env), stderr: 'pipe', }), }) ) ) + const mcpClients = mcpClientResults + .filter((r): r is PromiseFulfilledResult<Awaited<ReturnType<typeof createMCPClient>>> => r.status === 'fulfilled') + .map(r => r.value) + const failedCount = mcpClientResults.filter(r => r.status === 'rejected').length + if (failedCount > 0) { + context.onLog?.(`[agent] ${failedCount} MCP server(s) failed to connect`) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/agent-handler.ts` around lines 173 - 184, The current use of Promise.all over mergedMcpConfig to createMCPClient instances means a single Experimental_StdioMCPTransport failure will reject the entire agent run and skip cleanup; change the creation to handle per-client failures (e.g., map mergedMcpConfig to individual try/catch or Promise.allSettled, logging errors for failed createMCPClient calls and only collecting successful clients into mcpClients) so that createMCPClient/Experimental_StdioMCPTransport failures degrade gracefully and allow normal cleanup and operation with remaining clients.
158-161:⚠️ Potential issue | 🔴 CriticalReplace
gpt-5with an available model name.The model
'gpt-5'does not exist. As of March 2025, the latest available OpenAI model isGPT-4.5. Update the fallback to use an actual model identifier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/agent-handler.ts` around lines 158 - 161, The fallback model string is invalid: update the model selection logic that sets the modelName (using block.metadata.deepnote_agent_model and process.env.OPENAI_MODEL) to use a real OpenAI model identifier instead of "gpt-5" — replace that fallback with the current available model (e.g., "gpt-4.5") so modelName resolves to a valid model when deepnote_agent_model === 'auto' and OPENAI_MODEL is unset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/runtime-core/src/agent-handler.ts`:
- Around line 173-184: The current use of Promise.all over mergedMcpConfig to
createMCPClient instances means a single Experimental_StdioMCPTransport failure
will reject the entire agent run and skip cleanup; change the creation to handle
per-client failures (e.g., map mergedMcpConfig to individual try/catch or
Promise.allSettled, logging errors for failed createMCPClient calls and only
collecting successful clients into mcpClients) so that
createMCPClient/Experimental_StdioMCPTransport failures degrade gracefully and
allow normal cleanup and operation with remaining clients.
- Around line 158-161: The fallback model string is invalid: update the model
selection logic that sets the modelName (using
block.metadata.deepnote_agent_model and process.env.OPENAI_MODEL) to use a real
OpenAI model identifier instead of "gpt-5" — replace that fallback with the
current available model (e.g., "gpt-4.5") so modelName resolves to a valid model
when deepnote_agent_model === 'auto' and OPENAI_MODEL is unset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a0a03fc-f6af-401c-820d-23b7f0abaa6f
📒 Files selected for processing (1)
packages/runtime-core/src/agent-handler.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime-core/src/agent-handler.ts (1)
171-205:⚠️ Potential issue | 🟠 MajorClean up MCP clients if setup fails.
Cleanup only starts at Line 244. If one
createMCPClient()orclient.tools()call rejects here, any earlier stdio transports stay alive because they were created before thetry/finallyscope. Move MCP setup into the cleanup scope, or close the clients accumulated so far before rethrowing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/agent-handler.ts` around lines 171 - 205, The MCP client setup can leak processes if createMCPClient() or client.tools() rejects; ensure partial clients are closed on failure by wrapping the creation and tools-fetching in a try/catch/finally and, on any error, iterate the already-created mcpClients and call their shutdown/close/dispose method (e.g., client.close() or client.shutdown(), using the actual API) to stop underlying Experimental_StdioMCPTransport processes before rethrowing; similarly, ensure any created clients are closed in the existing cleanup/finally block so mergedMcpConfig, createMCPClient, mcpClients, mcpToolSets and client.tools() failures do not leave stray stdio transports running.
♻️ Duplicate comments (1)
packages/runtime-core/src/execution-engine.ts (1)
252-286:⚠️ Potential issue | 🟠 MajorKeep the full result for agent-inserted code blocks.
Line 283 collapses successful executions to
{ success: true }, the failure path still returns rawErrorobjects, and Lines 332-339 emit every inserted block assuccess: trueanyway. Thrown executions also never reachonBlockDonebecause nothing is recorded inblockOutputs. Keep one serializable execution result and reuse it for both the tool response and host callbacks.Also applies to: 323-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/execution-engine.ts` around lines 252 - 286, The addAndExecuteCodeBlock function currently discards successful execution details and only returns {success: true} while errors return raw Error objects and failed runs aren't recorded in blockOutputs/collectedOutputs, so change addAndExecuteCodeBlock to capture the full, serializable execution result from kernel.execute (including outputs and executionCount and a serializable error/message on failure), store that same result into blockOutputs and collectedOutputs for every run (success or failure), and return that single unified result structure so the tool response and any host callbacks (e.g., onBlockDone consumers) consume the exact same serializable execution result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/runtime-core/src/execution-engine.ts`:
- Around line 236-242: Replace direct reads of process.env for agent provider
settings with the runtime-provided environment in RuntimeConfig.env: use the
existing runtimeConfig.env (the env passed into the engine at RuntimeConfig.env)
to obtain OPENAI_API_KEY and related overrides instead of process.env (affecting
the apiKey check that currently reads process.env.OPENAI_API_KEY and the
forwarded context created around lines 310-315). Ensure the error message
reflects the actual env source and mentions available overrides (e.g.,
OPENAI_API_KEY, OPENAI_BASE_URL, model override keys) when missing, and update
the logic that builds the forwarded context to copy relevant keys from
runtimeConfig.env so embedders can supply base URL and model overrides without
mutating process state.
---
Outside diff comments:
In `@packages/runtime-core/src/agent-handler.ts`:
- Around line 171-205: The MCP client setup can leak processes if
createMCPClient() or client.tools() rejects; ensure partial clients are closed
on failure by wrapping the creation and tools-fetching in a try/catch/finally
and, on any error, iterate the already-created mcpClients and call their
shutdown/close/dispose method (e.g., client.close() or client.shutdown(), using
the actual API) to stop underlying Experimental_StdioMCPTransport processes
before rethrowing; similarly, ensure any created clients are closed in the
existing cleanup/finally block so mergedMcpConfig, createMCPClient, mcpClients,
mcpToolSets and client.tools() failures do not leave stray stdio transports
running.
---
Duplicate comments:
In `@packages/runtime-core/src/execution-engine.ts`:
- Around line 252-286: The addAndExecuteCodeBlock function currently discards
successful execution details and only returns {success: true} while errors
return raw Error objects and failed runs aren't recorded in
blockOutputs/collectedOutputs, so change addAndExecuteCodeBlock to capture the
full, serializable execution result from kernel.execute (including outputs and
executionCount and a serializable error/message on failure), store that same
result into blockOutputs and collectedOutputs for every run (success or
failure), and return that single unified result structure so the tool response
and any host callbacks (e.g., onBlockDone consumers) consume the exact same
serializable execution result.
🪄 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: 41513acb-7b98-4c35-b5d0-ecd19248eb98
📒 Files selected for processing (3)
packages/cli/src/commands/run.tspackages/runtime-core/src/agent-handler.tspackages/runtime-core/src/execution-engine.ts
…te into tk/agent-vscode-abstract
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/runtime-core/src/execution-engine.test.ts (1)
839-843:⚠️ Potential issue | 🟠 MajorUse imported types for agent context and mock results.
Mock
executeAgentBlockstill returns deprecatedaddedBlockIdsandblockOutputsfields (found at lines 841–842, 1030, 1099, 1129, 1157, 1197, 1227, 1255, 1282, 1303, 1321, 1340, 1373). Also, helper contexts usePromise<unknown>instead ofAddAndExecuteCodeBlockResultandAddMarkdownBlockResult(lines 1090, 1095, 1126, 1154, 1194, 1278, 1300, 1318, 1337, 1366–1367). The types are already imported—use them.Example fixes
- mockExecuteAgentBlock.mockResolvedValue({ - finalOutput: 'Analysis complete.', - addedBlockIds: [], - blockOutputs: [], - }) + mockExecuteAgentBlock.mockResolvedValue({ + finalOutput: 'Analysis complete.', + }) - let capturedContext: { addAndExecuteCodeBlock: (args: { code: string }) => Promise<unknown> } | null = null + let capturedContext: Pick<AgentBlockContext, 'addAndExecuteCodeBlock'> | null = null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/execution-engine.test.ts` around lines 839 - 843, Mocks and helper contexts are using deprecated fields and generic Promise<unknown>; update mockExecuteAgentBlock.mockResolvedValue calls to return objects matching the imported AddAndExecuteCodeBlockResult/AddMarkdownBlockResult shapes (remove deprecated addedBlockIds and blockOutputs fields) and annotate helper context variables to use the imported AddAndExecuteCodeBlockResult and AddMarkdownBlockResult types instead of Promise<unknown>; ensure executeAgentBlock-related mocks and any helper functions (search for mockExecuteAgentBlock, executeAgentBlock, and the helper context variables around lines where Promise<unknown> was used) use the correct imported types so the test types align with current interfaces.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/runtime-core/src/execution-engine.test.ts`:
- Around line 923-943: The test currently relies on hard-coded fixture indexes
(analysisNotebook.blocks[3], [4] and toHaveLength(5)) which is brittle; update
the test to capture the initial block count (e.g. const initialCount =
analysisNotebook.blocks.length), assert the count increased by 2
(expect(analysisNotebook.blocks).toHaveLength(initialCount + 2)), and locate the
inserted blocks by their content or ids rather than by fixed indexes (use
Array.prototype.find to locate blocks where content === helperCode and content
=== helperMarkdown or match type + content), then assert those found blocks
equal the expected objects and that mockKernelClient.execute was called with
helperCode. Ensure any references to addedCodeBlock/addedMarkdownBlock are
replaced with the found blocks and keep assertions for addCodeResult and
addMarkdownResult.
- Around line 1165-1341: The test span around the
describe('context.addMarkdownBlock') block is not formatted per Biome and CI
will fail; run the Biome formatter (e.g., biome format from the repo root) to
reformat the changed test block involving mockExecuteAgentBlock, engine.start,
and engine.runProject according to biome.json, then re-stage the resulting edits
so the CI rediff passes.
---
Duplicate comments:
In `@packages/runtime-core/src/execution-engine.test.ts`:
- Around line 839-843: Mocks and helper contexts are using deprecated fields and
generic Promise<unknown>; update mockExecuteAgentBlock.mockResolvedValue calls
to return objects matching the imported
AddAndExecuteCodeBlockResult/AddMarkdownBlockResult shapes (remove deprecated
addedBlockIds and blockOutputs fields) and annotate helper context variables to
use the imported AddAndExecuteCodeBlockResult and AddMarkdownBlockResult types
instead of Promise<unknown>; ensure executeAgentBlock-related mocks and any
helper functions (search for mockExecuteAgentBlock, executeAgentBlock, and the
helper context variables around lines where Promise<unknown> was used) use the
correct imported types so the test types align with current interfaces.
🪄 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: 9f68c9c8-bc40-4ea1-8248-bdebf47ddb80
📒 Files selected for processing (1)
packages/runtime-core/src/execution-engine.test.ts
…ete mock fields Remove the unread `addedBlockIds: string[]` array in `runProject`'s agent-block branch — `AgentBlockResult` no longer carries it and nothing else reads the local. Also drop the matching `addedBlockIds: []` and `blockOutputs: []` from every mock return in `execution-engine.test.ts` so they reflect the current `AgentBlockResult` shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The agent-handler refactor (dca6b0c) was intended as a mechanical extraction to make `addAndExecuteCodeBlock` and `addMarkdownBlock` injectable on `AgentBlockContext`, so the runtime can be reused in a VS Code extension. That extraction also (unintentionally) changed the tool-execute return shape from formatted strings to a structured `{ success, error }` object, which: - Hid the code's stdout/stderr from the LLM. The agent could no longer see its own output and could not iterate, because `Error` instances do not survive `JSON.stringify` and the success branch carried no output text. - Broke the CLI's `startsWith('Execution failed')` status check in run.ts: `event.output` was now JSON like `{"success":false,"error":{}}` instead of the formatted string it was designed to parse, so failures rendered as green `[ok]` with raw JSON in the preview. Drop the invented `AddAndExecuteCodeBlockResult` / `AddMarkdownBlockResult` types and re-type the context callbacks as `Promise<string>`. The helper bodies in `runProject` return the same strings the pre-PR `tool({execute})` callbacks did (`Output:\n…`, `Execution failed:\n…`, `Execution error: …`, `Markdown block added.`). CLI is unchanged from main and works again; the LLM sees its own output again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (5)
packages/runtime-core/src/execution-engine.ts (3)
72-72:⚠️ Potential issue | 🟠 MajorKeep
onAgentEventcompatible with sync handlers.Line 72 narrows to
Promise<void>only, butAgentBlockContextacceptsvoid | Promise<void>. This is an avoidable breaking API mismatch.#!/bin/bash # Verify callback type mismatch between execution options and agent context rg -n "onAgentEvent\\?: \\(event: AgentStreamEvent\\) =>" \ packages/runtime-core/src/execution-engine.ts \ packages/runtime-core/src/agent-handler.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/execution-engine.ts` at line 72, The onAgentEvent type in execution-engine.ts is too narrow (returns Promise<void>) and must match AgentBlockContext's handler type to accept both sync and async handlers; update the onAgentEvent signature to (event: AgentStreamEvent) => void | Promise<void> (or reference the same type alias used by AgentBlockContext) so callers and implementations can return either void or Promise<void>, and adjust any call sites to handle non-Promise returns if necessary.
258-292:⚠️ Potential issue | 🟠 MajorDon’t force added code blocks to success.
The helper drops
success/errorstate, then Line 336 reportssuccess: truefor every inserted code block. Failures become misreported or invisible inonBlockDone.Suggested patch
- const blockOutputs: Array<{ blockId: string; outputs: unknown[]; executionCount: number | null }> = [] + const blockOutputs: Array<{ + blockId: string + success: boolean + outputs: unknown[] + executionCount: number | null + error?: Error + }> = [] ... - blockOutputs.push({ + blockOutputs.push({ blockId: newBlock.id, + success: result.success, outputs: result.outputs, executionCount: result.executionCount, }) ... } catch (error) { const message = error instanceof Error ? error.message : String(error) + blockOutputs.push({ + blockId: newBlock.id, + success: false, + outputs: [createErrorOutput(new Error(message))], + executionCount: null, + error: error instanceof Error ? error : new Error(message), + }) return `Execution error: ${message}` } ... - success: true, + success: bo.success, outputs: bo.outputs as IOutput[], executionCount: bo.executionCount, durationMs: 0, + ...(bo.error ? { error: bo.error } : {}),Also applies to: 324-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/execution-engine.ts` around lines 258 - 292, The addAndExecuteCodeBlock helper currently drops the execution success flag and only returns text, causing inserted code blocks to be marked as successful regardless of kernel result; update addAndExecuteCodeBlock so it propagates result.success into all places that record execution state (specifically include success in the objects pushed to blockOutputs and collectedOutputs and ensure any code that later reads collectedOutputs or blockOutputs (e.g., onBlockDone) uses that success flag), and keep the returned string message separate from the boolean success so failures are not misreported as success.
243-249:⚠️ Potential issue | 🟠 MajorRead agent auth/config from runtime config, not globals.
This path still hard-reads
process.env, which breaks embedders that pass env viaRuntimeConfig.envand avoid mutating process state.Suggested patch
- const apiKey = process.env.OPENAI_API_KEY + const apiKey = this.config.env?.OPENAI_API_KEY ?? process.env.OPENAI_API_KEY if (!apiKey) { throw new Error( 'OPENAI_API_KEY environment variable is required for agent blocks.\n' + 'Set it to your OpenAI API key, or set OPENAI_BASE_URL for compatible providers.' ) }Also applies to: 311-314
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/execution-engine.ts` around lines 243 - 249, The code is directly reading process.env.OPENAI_API_KEY/OPENAI_BASE_URL in the agent auth path; update it to read from the runtime config's env instead (e.g., RuntimeConfig.env or the ExecutionEngine/runtime instance env object) so embedders can supply env without mutating process.env. Replace uses of process.env.OPENAI_API_KEY and process.env.OPENAI_BASE_URL with the runtime-provided env lookup (and keep the same error message/throw behavior), and apply the same change to the other occurrence mentioned (around the 311-314 block).packages/runtime-core/src/agent-handler.ts (1)
150-166:⚠️ Potential issue | 🟠 MajorStop reading model/baseURL overrides from global env here.
OPENAI_BASE_URLandOPENAI_MODELstill come fromprocess.env, so embedders can’t configure this cleanly through runtime context.Suggested patch
export interface AgentBlockContext { openAiToken: string + openAiBaseUrl?: string + openAiModel?: string mcpServers: McpServerConfig[] notebookContext: string addAndExecuteCodeBlock: (args: { code: string }) => Promise<string> addMarkdownBlock: (args: { content: string }) => Promise<string> ... } const openai = createOpenAI({ apiKey: context.openAiToken, - baseURL: process.env.OPENAI_BASE_URL, + baseURL: context.openAiBaseUrl ?? process.env.OPENAI_BASE_URL, }) const modelName = block.metadata.deepnote_agent_model !== 'auto' ? block.metadata.deepnote_agent_model - : (process.env.OPENAI_MODEL ?? 'gpt-5') + : (context.openAiModel ?? process.env.OPENAI_MODEL ?? 'gpt-5')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/agent-handler.ts` around lines 150 - 166, The code currently reads OPENAI_BASE_URL and OPENAI_MODEL from process.env which prevents embedders from configuring these via the runtime context; update the logic in the createOpenAI / model selection block to source baseURL and model name from the runtime context instead of global env: replace process.env.OPENAI_BASE_URL with context.openAiBaseUrl when setting baseURL and use context.openAiModel as the fallback for modelName (keep the block.metadata.deepnote_agent_model override and default 'gpt-5' only if context.openAiModel is undefined), and ensure the branching that chooses between openai.chat(modelName) and openai(modelName) uses the context-derived baseURL.packages/runtime-core/src/execution-engine.test.ts (1)
927-941:⚠️ Potential issue | 🟡 MinorAvoid hard-coded fixture indexes in helper tests.
blocks[3]/blocks[4]and fixed lengths are still fixture-coupled and brittle. Assert via initial count + content/id lookup instead.Suggested patch
- const addedCodeBlock = analysisNotebook.blocks[3] - const addedMarkdownBlock = analysisNotebook.blocks[4] + const initialCount = AGENT_FIXTURE.project.notebooks + .find(notebook => notebook.name === 'Analysis')?.blocks.length ?? 0 + const addedCodeBlock = analysisNotebook.blocks.find( + block => block.type === 'code' && block.content === helperCode + ) + const addedMarkdownBlock = analysisNotebook.blocks.find( + block => block.type === 'markdown' && block.content === helperMarkdown + ) ... - expect(analysisNotebook.blocks).toHaveLength(5) + expect(analysisNotebook.blocks).toHaveLength(initialCount + 2)Also applies to: 998-1007
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/execution-engine.test.ts` around lines 927 - 941, Replace brittle hard-coded fixture indexes and fixed length assertions by recording the initial block count from analysisNotebook.blocks, performing a content-based lookup for the new blocks (match content === helperCode and content === helperMarkdown) and asserting the block count increased by 2 and the found blocks are present; update assertions that reference addedCodeBlock/addedMarkdownBlock to use the located blocks, keep existing checks for summary.failedBlocks, agentContext?.notebookContext, addCodeResult and addMarkdownResult, and retain the mockKernelClient.execute call assertion (adjust its ordinal if needed relative to initial calls) — target symbols: analysisNotebook.blocks, helperCode, helperMarkdown, addCodeResult, addMarkdownResult, summary.failedBlocks, agentContext, and mockKernelClient.execute.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/runtime-core/src/agent-handler.ts`:
- Around line 150-166: The code currently reads OPENAI_BASE_URL and OPENAI_MODEL
from process.env which prevents embedders from configuring these via the runtime
context; update the logic in the createOpenAI / model selection block to source
baseURL and model name from the runtime context instead of global env: replace
process.env.OPENAI_BASE_URL with context.openAiBaseUrl when setting baseURL and
use context.openAiModel as the fallback for modelName (keep the
block.metadata.deepnote_agent_model override and default 'gpt-5' only if
context.openAiModel is undefined), and ensure the branching that chooses between
openai.chat(modelName) and openai(modelName) uses the context-derived baseURL.
In `@packages/runtime-core/src/execution-engine.test.ts`:
- Around line 927-941: Replace brittle hard-coded fixture indexes and fixed
length assertions by recording the initial block count from
analysisNotebook.blocks, performing a content-based lookup for the new blocks
(match content === helperCode and content === helperMarkdown) and asserting the
block count increased by 2 and the found blocks are present; update assertions
that reference addedCodeBlock/addedMarkdownBlock to use the located blocks, keep
existing checks for summary.failedBlocks, agentContext?.notebookContext,
addCodeResult and addMarkdownResult, and retain the mockKernelClient.execute
call assertion (adjust its ordinal if needed relative to initial calls) — target
symbols: analysisNotebook.blocks, helperCode, helperMarkdown, addCodeResult,
addMarkdownResult, summary.failedBlocks, agentContext, and
mockKernelClient.execute.
In `@packages/runtime-core/src/execution-engine.ts`:
- Line 72: The onAgentEvent type in execution-engine.ts is too narrow (returns
Promise<void>) and must match AgentBlockContext's handler type to accept both
sync and async handlers; update the onAgentEvent signature to (event:
AgentStreamEvent) => void | Promise<void> (or reference the same type alias used
by AgentBlockContext) so callers and implementations can return either void or
Promise<void>, and adjust any call sites to handle non-Promise returns if
necessary.
- Around line 258-292: The addAndExecuteCodeBlock helper currently drops the
execution success flag and only returns text, causing inserted code blocks to be
marked as successful regardless of kernel result; update addAndExecuteCodeBlock
so it propagates result.success into all places that record execution state
(specifically include success in the objects pushed to blockOutputs and
collectedOutputs and ensure any code that later reads collectedOutputs or
blockOutputs (e.g., onBlockDone) uses that success flag), and keep the returned
string message separate from the boolean success so failures are not misreported
as success.
- Around line 243-249: The code is directly reading
process.env.OPENAI_API_KEY/OPENAI_BASE_URL in the agent auth path; update it to
read from the runtime config's env instead (e.g., RuntimeConfig.env or the
ExecutionEngine/runtime instance env object) so embedders can supply env without
mutating process.env. Replace uses of process.env.OPENAI_API_KEY and
process.env.OPENAI_BASE_URL with the runtime-provided env lookup (and keep the
same error message/throw behavior), and apply the same change to the other
occurrence mentioned (around the 311-314 block).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8fba9ec3-d8d0-450a-9a67-9c796fbbdbbd
📒 Files selected for processing (3)
packages/runtime-core/src/agent-handler.tspackages/runtime-core/src/execution-engine.test.tspackages/runtime-core/src/execution-engine.ts
… handling Changed the type of `onAgentEvent` in both `run.ts` and `execution-engine.ts` to support synchronous execution, improving the flexibility of event handling in the CLI and runtime core.
…eNotebookContextFromBlocks Updated the type of notebookName in the serializeNotebookContextFromBlocks function to enforce it as a non-nullable string, simplifying the serialization logic by removing conditional checks for null values.
Added new serialization functions to improve handling of block outputs in notebooks. Introduced `serializeNotebookContextFromBlocks` and `createBlocksWithAttachedOutputsFromCollectedOutputs` to manage saved and collected outputs more effectively. Updated tests to cover various scenarios, ensuring only relevant outputs are serialized based on collected data.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/runtime-core/src/execution-engine.ts (2)
254-340:⚠️ Potential issue | 🟠 MajorAdded code block outcomes are reported incorrectly.
Line 336 always emits
success: truefor agent-inserted code blocks. Ifkernel.executereturnssuccess: false, this reports the wrong status; if execute throws, no per-added-block completion is emitted.Suggested diff
- const blockOutputs: Array<{ blockId: string; outputs: unknown[]; executionCount: number | null }> = [] + const blockOutputs: Array<{ + blockId: string + success: boolean + outputs: IOutput[] + executionCount: number | null + error?: Error + }> = [] @@ - blockOutputs.push({ + blockOutputs.push({ blockId: newBlock.id, + success: result.success, - outputs: result.outputs, + outputs: result.outputs as IOutput[], executionCount: result.executionCount, }) @@ } catch (error) { const message = error instanceof Error ? error.message : String(error) + const executionError = error instanceof Error ? error : new Error(message) + blockOutputs.push({ + blockId: newBlock.id, + success: false, + outputs: [createErrorOutput(executionError)], + executionCount: null, + error: executionError, + }) return `Execution error: ${message}` } @@ - success: true, - outputs: bo.outputs as IOutput[], + success: bo.success, + outputs: bo.outputs, executionCount: bo.executionCount, durationMs: 0, + ...(bo.error ? { error: bo.error } : {}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/execution-engine.ts` around lines 254 - 340, The agent-added code blocks are always reported as successful because onBlockDone is called with success: true and thrown executions aren't reported; update addAndExecuteCodeBlock and the post-agent reporting to record the actual execution result and failures: when kernel.execute resolves, push blockOutputs with the real result.success and executionCount (and include outputs), and when kernel.execute throws, push a failure entry to blockOutputs (include a traceback or error message in outputs and executionCount=null); then when emitting onBlockDone for each bo use the stored success flag (not hardcoded true) and ensure thrown errors produce a corresponding onBlockDone call with success: false so every added block gets a completion event.
243-249:⚠️ Potential issue | 🟠 MajorRead agent API key from runtime config env, not only process globals.
Line 243 bypasses
RuntimeConfig.env, which makes embedding/runtime reuse harder and forces host-level env mutation.Suggested diff
- const apiKey = process.env.OPENAI_API_KEY + const apiKey = this.config.env?.OPENAI_API_KEY ?? process.env.OPENAI_API_KEY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/execution-engine.ts` around lines 243 - 249, The code currently reads OPENAI_API_KEY from process.env (apiKey) which bypasses the runtime config; update the lookup to first read from RuntimeConfig.env (e.g., runtimeConfig.env.OPENAI_API_KEY) and fall back to process.env.OPENAI_API_KEY so embedded runtimes can supply keys without host env mutation; apply the same pattern for OPENAI_BASE_URL if used, and update the error/throw logic that references apiKey accordingly (look for the apiKey variable and the error message block to change the source).packages/runtime-core/src/execution-engine.test.ts (1)
927-941: 🧹 Nitpick | 🔵 TrivialAvoid fixture-index assertions in this helper-flow test.
Line 927 and Line 928 (
blocks[3],blocks[4]) plus Line 939 (toHaveLength(5)) make this test fragile to fixture edits. Assert relative growth and find inserted blocks by content/id instead.Suggested diff
- const addedCodeBlock = analysisNotebook.blocks[3] - const addedMarkdownBlock = analysisNotebook.blocks[4] + const initialCount = AGENT_FIXTURE.project.notebooks + .find(notebook => notebook.name === 'Analysis') + ?.blocks.length ?? 0 + const addedCodeBlock = analysisNotebook.blocks.find( + block => block.type === 'code' && block.content === helperCode + ) + const addedMarkdownBlock = analysisNotebook.blocks.find( + block => block.type === 'markdown' && block.content === helperMarkdown + ) @@ - expect(analysisNotebook.blocks).toHaveLength(5) + expect(analysisNotebook.blocks).toHaveLength(initialCount + 2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/execution-engine.test.ts` around lines 927 - 941, The test currently relies on fixture-indexed assertions (blocks[3], blocks[4], and toHaveLength(5)); change it to assert relative growth and locate inserted blocks by content or id: record const beforeCount = analysisNotebook.blocks.length before the helper flow, assert analysisNotebook.blocks.length === beforeCount + 2 after, find the new code block via analysisNotebook.blocks.find(b => b.type === 'code' && b.content === helperCode) and the new markdown block via .find(b => b.type === 'markdown' && b.content === helperMarkdown) (or match by inserted block id if available), then assert those found blocks exist and contain the expected properties (replacing addedCodeBlock/addedMarkdownBlock index usage), keep existing assertions for summary.failedBlocks, agentContext?.notebookContext, addCodeResult/addMarkdownResult, and mockKernelClient.execute expectations but remove the fragile toHaveLength(5) and direct index accesses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/runtime-core/src/agent-handler.ts`:
- Around line 15-23: The handler still reads provider config from process.env;
update AgentBlockContext to accept provider overrides (e.g., add optional fields
like openAiBaseUrl, openAiModel, openAiApiKey or a generic providerConfig
object) and modify the code paths that build provider clients (the logic around
where process.env is read at the block handler — references to openAi
token/URL/model usage) to prefer these AgentBlockContext values before falling
back to process.env; ensure functions that construct clients or call OpenAI
(identify the client creation/usage code that currently reads process.env at
lines around 150–163) pull from ctx.openAiBaseUrl/ctx.openAiModel or
ctx.integrations/providerConfig if present.
---
Duplicate comments:
In `@packages/runtime-core/src/execution-engine.test.ts`:
- Around line 927-941: The test currently relies on fixture-indexed assertions
(blocks[3], blocks[4], and toHaveLength(5)); change it to assert relative growth
and locate inserted blocks by content or id: record const beforeCount =
analysisNotebook.blocks.length before the helper flow, assert
analysisNotebook.blocks.length === beforeCount + 2 after, find the new code
block via analysisNotebook.blocks.find(b => b.type === 'code' && b.content ===
helperCode) and the new markdown block via .find(b => b.type === 'markdown' &&
b.content === helperMarkdown) (or match by inserted block id if available), then
assert those found blocks exist and contain the expected properties (replacing
addedCodeBlock/addedMarkdownBlock index usage), keep existing assertions for
summary.failedBlocks, agentContext?.notebookContext,
addCodeResult/addMarkdownResult, and mockKernelClient.execute expectations but
remove the fragile toHaveLength(5) and direct index accesses.
In `@packages/runtime-core/src/execution-engine.ts`:
- Around line 254-340: The agent-added code blocks are always reported as
successful because onBlockDone is called with success: true and thrown
executions aren't reported; update addAndExecuteCodeBlock and the post-agent
reporting to record the actual execution result and failures: when
kernel.execute resolves, push blockOutputs with the real result.success and
executionCount (and include outputs), and when kernel.execute throws, push a
failure entry to blockOutputs (include a traceback or error message in outputs
and executionCount=null); then when emitting onBlockDone for each bo use the
stored success flag (not hardcoded true) and ensure thrown errors produce a
corresponding onBlockDone call with success: false so every added block gets a
completion event.
- Around line 243-249: The code currently reads OPENAI_API_KEY from process.env
(apiKey) which bypasses the runtime config; update the lookup to first read from
RuntimeConfig.env (e.g., runtimeConfig.env.OPENAI_API_KEY) and fall back to
process.env.OPENAI_API_KEY so embedded runtimes can supply keys without host env
mutation; apply the same pattern for OPENAI_BASE_URL if used, and update the
error/throw logic that references apiKey accordingly (look for the apiKey
variable and the error message block to change the source).
🪄 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: 14bdc844-9080-42f0-9f10-3175fdc3baa3
📒 Files selected for processing (4)
packages/runtime-core/src/agent-handler.test.tspackages/runtime-core/src/agent-handler.tspackages/runtime-core/src/execution-engine.test.tspackages/runtime-core/src/execution-engine.ts
Track success on blockOutputs entries and push on kernel.execute rejection so blocks the agent inserted always get an onBlockDone event matching their actual outcome. Wrap the agent call in try/finally so added blocks are drained even when executeAgentBlock throws. Also tighten add_code_block tool description and drop the redundant post-loop collectedOutputs.set / unknown[] casts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/runtime-core/src/agent-handler.ts`:
- Around line 66-73: The current map over blocks always resets any existing
block.outputs to [] unless there's a collectedOutputs entry; change this so only
blocks with a collectedOutputs entry are modified. In the blocks.map callback
that uses collectedOutputs.get(block.id), keep the branch that returns
{...block, outputs: collected.outputs} when collected is non-null, and otherwise
return the original block unchanged (remove the branch that forces outputs: []).
Update the logic around collectedOutputs and the blocks.map so persisted outputs
are preserved unless an override exists.
In `@packages/runtime-core/src/execution-engine.ts`:
- Around line 279-309: The current addAndExecuteCodeBlock() buffers outputs into
blockOutputs/collectedOutputs and only calls onOutput/onBlockDone after
kernel.execute returns, causing helper-block events to be delayed; change the
call to kernel.execute(...) to pass a per-output callback that forwards each
output immediately to the existing onOutput handler (including mapping
executionCount and blockId), and ensure you call onBlockDone (with success and
executionCount) as soon as that execute promise resolves or in the catch path
for errors (instead of waiting to push to blockOutputs only); update the same
pattern in the similar helper at the other location so both places wire
kernel.execute output streaming through onOutput and emit onBlockDone when the
helper finishes.
🪄 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: 2e32a20f-da8a-4051-b44e-b6bc8d4a2c2c
📒 Files selected for processing (3)
packages/runtime-core/src/agent-handler.tspackages/runtime-core/src/execution-engine.test.tspackages/runtime-core/src/execution-engine.ts
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
3 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 by CodeRabbit
Refactor
Tests
Chores