Re: [PR] refactor(code-mode): remove scaffolding superseded by @ai-sdk/code-mode [maka]

["hqhq1025 (via GitHub)" <gi...@apache.org>]

hqhq1025 commented on code in PR #3225:
URL: https://github.com/apache/maka/pull/3225#discussion_r3819264523


##########
packages/code-mode/src/quickjs.ts:
##########
@@ -75,20 +68,7 @@ export async function executeCodeCellImpl(
       js: input.code,
       tools,
       toolExecutionOptions: { abortSignal: invocationSignal },
-      options: {
-        executionPolicy: {
-          timeoutMs: limits.maxSandboxTimeMs,
-          memoryLimitBytes: limits.maxMemoryBytes,
-          maxStackSizeBytes: limits.maxStackBytes,
-          maxResultBytes: limits.maxOutputBytes,
-          maxConsoleOutputBytes: 1,
-          maxSourceBytes: limits.maxSourceBytes,
-          maxToolInputBytes: limits.maxToolInputBytes,
-          maxToolOutputBytes: limits.maxToolOutputBytes,
-          maxBridgeRequests: limits.maxToolCalls,
-          maxInFlightBridgeRequests: limits.maxToolConcurrency,
-        },
-      },
+      options: { executionPolicy },

Review Comment:
   [P1] Keep admission ownership until host operations finish draining.
   
   Delegating outer-cell admission to `run` does not preserve the guarantee provided by the removed queue. On cancellation or timeout, `runCodeMode` releases its process-global worker slot before this adapter finishes `drainHostToolOperations`. If `callTool` ignores cancellation or performs slow cleanup, another cell is admitted while the previous host operation is still running.
   
   I reproduced this with `setMaxWorkers(1)`: repeatedly starting a tool, aborting its cell, and starting the next cell accumulated eight simultaneously retained host operations even though the worker cap was one. This can create unbounded host-side work, overlapping mutations, and turns that never finish draining.
   
   Please keep a Runtime-owned admission permit across the complete cell lifecycle, including host-operation drainage, and add a multi-wave cancellation regression test. The SDK worker cap can remain the lower-level QuickJS bound.



##########
packages/code-mode/src/quickjs.ts:
##########
@@ -10,25 +11,17 @@ import type {
   CodeModeToolCall,
   ExecuteCodeCellInput,
 } from './index.js';
-import { DEFAULT_CODE_MODE_LIMITS } from './index.js';
+import { DEFAULT_CODE_MODE_EXECUTION_POLICY } from './index.js';
 
-export async function executeCodeCellImpl(
+export async function executeCodeCell(
   input: ExecuteCodeCellInput,
 ): Promise<CodeModeExecutionResult> {
-  const limits = {
-    maxSourceBytes: input.limits?.maxSourceBytes ?? DEFAULT_CODE_MODE_LIMITS.maxSourceBytes,
-    maxSandboxTimeMs: input.limits?.maxSandboxTimeMs ?? DEFAULT_CODE_MODE_LIMITS.maxSandboxTimeMs,
-    maxMemoryBytes: input.limits?.maxMemoryBytes ?? DEFAULT_CODE_MODE_LIMITS.maxMemoryBytes,
-    maxStackBytes: input.limits?.maxStackBytes ?? DEFAULT_CODE_MODE_LIMITS.maxStackBytes,
-    maxToolCalls: input.limits?.maxToolCalls ?? DEFAULT_CODE_MODE_LIMITS.maxToolCalls,
-    maxToolConcurrency:
-      input.limits?.maxToolConcurrency ?? DEFAULT_CODE_MODE_LIMITS.maxToolConcurrency,
-    maxToolInputBytes:
-      input.limits?.maxToolInputBytes ?? DEFAULT_CODE_MODE_LIMITS.maxToolInputBytes,
-    maxToolOutputBytes:
-      input.limits?.maxToolOutputBytes ?? DEFAULT_CODE_MODE_LIMITS.maxToolOutputBytes,
-    maxOutputBytes: input.limits?.maxOutputBytes ?? DEFAULT_CODE_MODE_LIMITS.maxOutputBytes,
-  };
+  // An explicit `undefined` override keeps the product default rather than
+  // falling through to the SDK's looser one.
+  const executionPolicy: CodeModeExecutionPolicy = { ...DEFAULT_CODE_MODE_EXECUTION_POLICY };
+  for (const [key, value] of Object.entries(input.executionPolicy ?? {})) {
+    if (value !== undefined) (executionPolicy as Record<string, unknown>)[key] = value;

Review Comment:
   [P2] Reject invalid overrides instead of falling through to looser SDK defaults.
   
   This loop copies `null` because it excludes only `undefined`. The SDK resolves policy fields with `??`, so a runtime JavaScript or JSON caller can pass `{ maxSourceBytes: null }` and silently restore the SDK default of 256 KiB instead of Maka's 64 KiB default. The same widening affects tool output, bridge requests, in-flight requests, and console output.
   
   I reproduced this with a 70 KiB source: the default and an explicit `undefined` override were rejected at 65,536 bytes, while the `null` override executed successfully.
   
   Please whitelist the policy fields and accept only valid positive integers, treating both `null` and `undefined` as retaining Maka's product default. Add a regression test for runtime-invalid values.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org