Skip to content

1680 fix tool calling in playground#1782

Merged
neelneelneel merged 32 commits intodevfrom
1680-fix-tool-calling-in-playground
Sep 2, 2025
Merged

1680 fix tool calling in playground#1782
neelneelneel merged 32 commits intodevfrom
1680-fix-tool-calling-in-playground

Conversation

@neelneelneel
Copy link
Copy Markdown
Contributor

Description

Changes Made

How to Test

  1. Steps to reproduce/test the behavior
  2. Expected outcomes

Notes

neelneelneel and others added 27 commits August 12, 2025 00:36
@neelneelneel neelneelneel requested a review from a team as a code owner August 27, 2025 22:25
@github-actions
Copy link
Copy Markdown

@CodiumAI-Agent /describe

@QodoAI-Agent
Copy link
Copy Markdown

Title

1680 fix tool calling in playground


User description

Description

Changes Made

How to Test

  1. Steps to reproduce/test the behavior
  2. Expected outcomes

Notes


PR Type

Enhancement, Bug fix


Description

  • Improve export modal behavior and ids

  • Fix tag rendering and keys

  • Stabilize permission fetch with conditional hook

  • Correct rewrite and sibling navigation logic


Diagram Walkthrough

flowchart LR
  engineHeader["EngineHeader UI tweaks"] -- "export modal, tags, ids" --> clientUI["Client UI"]
  engineLayout["EngineLayout permissions"] -- "conditional useAPI" --> clientData["Data fetch stability"]
  responseMsg["ResponseMessage logic"] -- "use parent InputMessage, sibling nav" --> playgroundUI["Playground UI"]
  roomStore["Room store actions"] -- "rewrite uses correct parent, tool exec parent id" --> backendCalls["Pixel commands"]
Loading

File Walkthrough

Relevant files
Enhancement
EngineHeader.tsx
Export flow, accessibility, and tag rendering fixes           

packages/client/src/components/engine/EngineHeader.tsx

  • Normalize quotes and formatting in export pixel
  • Gate export modal for H2_DB engines
  • Add stable aria ids and Modal props order
  • Fix tag rendering limit and unique keys
  • Simplify fallback text rendering
+64/-68 
EngineLayout.tsx
Conditional permission fetch and metadata pass-through     

packages/client/src/pages/engine/EngineLayout.tsx

  • Use conditional useAPI key to fetch permissions
  • Preserve metadata and add database_subtype
+7/-6     
Bug fix
ResponseMessage.tsx
Correct rewrite controls and sibling navigation                   

packages/playground/src/components/message/ResponseMessage.tsx

  • Remove unused insight login-derived initials
  • Resolve parent InputMessage for rewrite gating
  • Fix sibling navigation to use input message siblings
  • Guard rewrite when no parent input message
+37/-33 
room.store.ts
Rewrite and tool-calling Pixel arguments corrected             

packages/playground/src/stores/room/room.store.ts

  • Enforce rewrite on responses to user messages only
  • Use original user text and grandparent id in AskPlayground
  • Include parentMessageId in AddPlaygroundToolExecution
  • Minor formatting tweaks
+10/-4   

@github-actions
Copy link
Copy Markdown

@CodiumAI-Agent /review

@QodoAI-Agent
Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 Security concerns

Injection/escaping in dynamic commands:
Multiple places build command strings for Pixel (e.g., ExportEngine, AskPlayground, AddPlaygroundToolExecution) using template literals and user/engine-derived values (active.id, parentMessage.text, toolResponse). Ensure all interpolated values are properly escaped or encoded (you used <encode> for command and context, which is good). Confirm similar encoding for toolResponse, toolName, and IDs to prevent command injection or breaking the query.

⚡ Recommended focus areas for review

Possible Null Access

The UI now relies on inputMessage?.previousSibling/nextSibling and inputMessage?.siblings.length. While you added guards and disabled states, ensure all render paths and event handlers never dereference when inputMessage is null, and that MobX observability updates correctly when siblings change.

<div>
	<Divider />
	<Stack
		direction={"row"}
		alignItems={"center"}
		justifyContent={"space-between"}
	>
		<Stack direction={"row"} alignItems={"center"}>
			{inputMessage && (
				<Button
					variant="text"
					size="small"
					color="inherit"
					onClick={() => rewriteMessage()}
				>
					Rewrite
				</Button>
			)}
			{inputMessage?.siblings.length > 1 && (
				<>
					<IconButton
						size="small"
						disabled={
							!inputMessage.previousSibling
						}
						onClick={() => {
							if (
								!inputMessage.previousSibling
							) {
								return;
							}

							inputMessage.previousSibling.activateMessage();
						}}
					>
						<ChevronLeftOutlined fontSize="small" />
					</IconButton>
					<Typography variant="caption">
						{inputMessage.position + 1}/
						{inputMessage.siblings.length}
					</Typography>
					<IconButton
						size="small"
						disabled={!inputMessage.nextSibling}
						onClick={() => {
							if (!inputMessage.nextSibling) {
								return;
							}

							inputMessage.nextSibling.activateMessage();
						}}
					>
						<ChevronRightOutlined fontSize="small" />
					</IconButton>
API Contract Change

The rewrite flow changed from sending prompt and parentMessageId (parent) to using parentMessage.text as command and passing grandParentMessage.id as parentMessageId. Verify backend AskPlayground expects these fields and that threading of messages (parent/child relationships) remains correct.

	 */
	rewriteMessage = async (message: ResponseMessageStore): Promise<void> => {
		try {
			// turn on the loading screen
			this.setIsLoading(true);

			// get the parent message
			const parentMessage = message.parent;
			if (parentMessage instanceof InputMessageStore === false) {
				throw new Error("Can only rewrite response to user messages");
			}

			// get the grand parent message
			const grandParentMessage = parentMessage.parent;

			// build the context if it is there
			let context = "";
			if (this._store.options?.instructions) {
				context = this._store.options?.instructions;
			}

			// get a list of tool ids
			const tools: string[] = this._store.options.tools.map(
				(t) => t.id,
				[],
			);

			// wait for the pixel to run
			const response = await this.runPixel<
				[
					{
						inputMessage: PixelMessage;
						responseMessage: PixelMessage;
					},
				]
			>(
				`AskPlayground(
engine=["${this._store.modelId}"],
roomId=["${this._store.roomId}"],
command=["<encode>${parentMessage.text}</encode>"],
${context ? `context=["<encode>${context}</encode>"],` : `context=[],`}

${tools.length ? `mcpToolID=${JSON.stringify(tools)},` : "mcpToolID=[],"}
${grandParentMessage.id ? `parentMessageId=["${grandParentMessage.id}"],` : ""}
paramValues=[${JSON.stringify({
					max_new_tokens: this._store.options.tokenLength,
					temperature: this._store.options.temperature,
				})}]
Pixel String Construction

The pixel string uses template literals mixing double quotes and encoded booleans. Confirm escaping is correct for all active.id values and that includeData is sent as expected to avoid malformed pixel or injection issues.

const exportDB = (includeData: boolean) => {
	setExportLoading(true);
	const pixel = `META | ExportEngine(engine=["${
		active.id
	}"], includeData="${includeData ? "true" : "false"}" );`;

	monolithStore.runQuery(pixel).then((response) => {
		const output = response.pixelReturn[0].output,
			insightId = response.insightId;

@github-actions
Copy link
Copy Markdown

@CodiumAI-Agent /improve

@QodoAI-Agent
Copy link
Copy Markdown

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Impact
Possible issue
Encode tool response safely

Ensure toolResponse is safely serialized to prevent breaking the Pixel string when
containing quotes or newlines. Wrap with an encode marker or escape characters to
avoid malformed commands.

packages/playground/src/stores/room/room.store.ts [667-674]

 `AddPlaygroundToolExecution(
  engine=["${this._store.modelId}"],
 roomId = ["${this._store.roomId}"],
 ${message.id ? `parentMessageId=["${message.id}"],` : ""}
 toolId = ["${toolId}"],
 toolName=["${toolName}"],
-tool_execution_response=["${toolResponse}"]
+tool_execution_response=["<encode>${toolResponse}</encode>"]
 );`,
Suggestion importance[1-10]: 7

__

Why: Wrapping toolResponse with <encode> prevents malformed Pixel when responses include quotes/newlines, improving robustness; aligns with existing encode usage elsewhere.

Medium
Null-check parent reference

Guard against grandParentMessage being null/undefined before accessing .id. If
absent, pass an empty parent reference to avoid runtime errors and incorrect
threading.

packages/playground/src/stores/room/room.store.ts [548-558]

 `AskPlayground(
  engine=["${this._store.modelId}"],
  roomId=["${this._store.roomId}"],
 command=["<encode>${parentMessage.text}</encode>"],
 ${context ? `context=["<encode>${context}</encode>"],` : `context=[],`}
 
 ${tools.length ? `mcpToolID=${JSON.stringify(tools)},` : "mcpToolID=[],"}
-${grandParentMessage.id ? `parentMessageId=["${grandParentMessage.id}"],` : ""}
+${grandParentMessage && grandParentMessage.id ? `parentMessageId=["${grandParentMessage.id}"],` : ""}
 paramValues=[${JSON.stringify({
 					max_new_tokens: this._store.options.tokenLength,
 					temperature: this._store.options.temperature,
 				})}]
Suggestion importance[1-10]: 6

__

Why: The PR adds ${grandParentMessage.id ? ...}; adding an explicit null-check is a reasonable safeguard against undefined grandparents and avoids runtime errors, though many template engines treat undefined safely already.

Low
Guard navigation against null

Add an inputMessage null guard before dereferencing nested properties inside the
navigation controls. This prevents runtime errors when the response lacks a valid
parent input message.

packages/playground/src/components/message/ResponseMessage.tsx [165-200]

-{inputMessage?.siblings.length > 1 && (
+{inputMessage && inputMessage.siblings.length > 1 && (
 	<>
 		<IconButton
 			size="small"
-			disabled={
-				!inputMessage.previousSibling
-			}
+			disabled={!inputMessage.previousSibling}
 			onClick={() => {
-				if (
-					!inputMessage.previousSibling
-				) {
-					return;
+				if (!inputMessage || !inputMessage.previousSibling) {
+				 return;
 				}
-
 				inputMessage.previousSibling.activateMessage();
 			}}
 		>
Suggestion importance[1-10]: 5

__

Why: Adding an outer inputMessage null-check is a small defensive improvement; the current code already conditionally renders based on inputMessage?.siblings.length, but explicit guarding reduces edge-case risks.

Low

@neelneelneel neelneelneel merged commit d401cca into dev Sep 2, 2025
3 checks passed
@neelneelneel neelneelneel deleted the 1680-fix-tool-calling-in-playground branch September 2, 2025 21:00
@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 2, 2025

@CodiumAI-Agent /update_changelog

@QodoAI-Agent
Copy link
Copy Markdown

Changelog updates: 🔄

2025-09-02 *

Added

  • New Room configuration panel and sidebar UI; tool artifacts and vertical stepper for tool execution
  • Stepper component in UI library and related story

Changed

  • Updated playground chat layout, inputs, and message styling; improved room header and discover routing

Fixed

  • Corrected tool execution payload fields and rewrite flow handling

to commit the new content to the CHANGELOG.md file, please type:
'/update_changelog --pr_update_changelog.push_changelog_changes=true'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants