Просмотр исходного кода

fix: self-contained built bundles + wire-size value cap (bot review)

Two findings from the GitHub review bot on the ready PR:

The tsdown two-entry build emitted the shared bootstrap module as a
lib/bootstrap-*.js chunk imported by both bundles, which the package.json
files whitelist (deliberately exact) omitted — a packed install had
dangling imports. The package now runs two single-entry builds, so each
bundle inlines its own bootstrap copy and every shipped file is
self-contained.

prepareValue admitted any cloneable value whose BOUNDED inspect rendering
fit maxValueBytes, so a huge container with a compact rendering (a
50k-element array renders as '... N more items') crossed the port raw,
bypassing the cap on both sides. The cap now measures the value's real
cross-boundary size — exact bytes for strings, the structured-clone wire
size (v8.serialize) for everything else — and oversized containers cross
as their bounded rendering instead.
Tianyi Cui 2 месяцев назад
Родитель
Сommit
e20ce35ffb

+ 5 - 1
docs/config-catalog.md

@@ -164,7 +164,11 @@ export interface Config {
   maxWallMs?: number
   /** Shared byte budget for captured log text (console + raw stream writes), truncation marked in-band. */
   maxLogBytes?: number
-  /** Byte cap for the rendered completion value; an oversized or non-cloneable value crosses as a capped string rendering. */
+  /**
+   * Byte cap for the completion value, measured by its real cross-boundary
+   * size (string bytes, or structured-clone wire size); an oversized or
+   * non-cloneable value crosses as a capped string rendering.
+   */
   maxValueBytes?: number
   /** The worker's max old-generation heap in MiB (`resourceLimits`); overflow kills the worker, surfacing as kind `'worker-exit'`. */
   maxOldGenerationSizeMb?: number

+ 25 - 17
packages/code-runtime/code-runtime-worker/src/bootstrap.ts

@@ -11,6 +11,7 @@
  */
 
 import { inspect } from 'node:util'
+import { serialize } from 'node:v8'
 import type { CodeLogEntry } from '@deepseek-ai/dsh-code-runtime'
 import { logTruncationMarker } from './protocol.ts'
 import type { DoneMessage, ReplyMessage, WorkerBootData, WorkerToHost } from './protocol.ts'
@@ -119,29 +120,36 @@ export function captureStreamWrites(logs: LogBuffer, stream: PatchableStream, so
 const INSPECT_OPTIONS = { depth: 4, maxArrayLength: 100, maxStringLength: 10_000 } as const
 
 /**
- * Prepare the program's completion value for the done message: a
- * structured-clone-safe value whose rendering fits `maxValueBytes` crosses
- * raw; anything else (non-cloneable, or oversized) is REPLACED by its
- * bounded `util.inspect` rendering, truncated with an in-band marker — the
- * seam contract's "a non-transferable value is replaced by a string
- * rendering", extended to oversized ones so a huge return cannot flood the
- * host.
+ * Prepare the program's completion value for the done message: a value whose
+ * MEASURED cross-boundary size fits `maxValueBytes` crosses raw — exact
+ * bytes for a string, the structured-clone wire size (`v8.serialize`) for
+ * everything else, so a huge container whose BOUNDED inspect rendering
+ * happens to be small cannot smuggle itself past the cap. Anything else
+ * (non-cloneable, or oversized) is REPLACED by its bounded `util.inspect`
+ * rendering, truncated with an in-band marker — the seam contract's "a
+ * non-transferable value is replaced by a string rendering", extended to
+ * oversized ones so a huge return cannot flood the host.
  * @param value - the program's completion value.
- * @param maxValueBytes - the byte cap for the rendered value.
+ * @param maxValueBytes - the byte cap for the value.
  * @returns the done-message fragment: `{}` for `undefined`, else `{ value }`.
  */
 export function prepareValue(value: unknown, maxValueBytes: number): { value?: unknown } {
   if (value === undefined) return {}
-  const rendered = typeof value === 'string' ? value : inspect(value, INSPECT_OPTIONS)
-  let cloneable = true
-  try {
-    structuredClone(value)
-  } catch {
-    // Only the verdict matters: the value has parts structured clone rejects
-    // (functions, classes, …) and must cross as its rendering instead.
-    cloneable = false
+  if (typeof value === 'string') {
+    if (Buffer.byteLength(value, 'utf8') <= maxValueBytes) return { value }
+  } else {
+    let size: number | undefined
+    try {
+      size = serialize(value).byteLength
+    } catch {
+      // Only the verdict matters: the value has parts the structured-clone
+      // algorithm rejects (functions, classes, …) and must cross as its
+      // rendering instead.
+      size = undefined
+    }
+    if (size !== undefined && size <= maxValueBytes) return { value }
   }
-  if (cloneable && Buffer.byteLength(rendered, 'utf8') <= maxValueBytes) return { value }
+  const rendered = typeof value === 'string' ? value : inspect(value, INSPECT_OPTIONS)
   const capped = rendered.length > maxValueBytes ? `${rendered.slice(0, maxValueBytes)}… [truncated]` : rendered
   return { value: capped }
 }

+ 5 - 1
packages/code-runtime/code-runtime-worker/src/index.ts

@@ -45,7 +45,11 @@ export interface Config {
   maxWallMs?: number
   /** Shared byte budget for captured log text (console + raw stream writes), truncation marked in-band. */
   maxLogBytes?: number
-  /** Byte cap for the rendered completion value; an oversized or non-cloneable value crosses as a capped string rendering. */
+  /**
+   * Byte cap for the completion value, measured by its real cross-boundary
+   * size (string bytes, or structured-clone wire size); an oversized or
+   * non-cloneable value crosses as a capped string rendering.
+   */
   maxValueBytes?: number
   /** The worker's max old-generation heap in MiB (`resourceLimits`); overflow kills the worker, surfacing as kind `'worker-exit'`. */
   maxOldGenerationSizeMb?: number

+ 10 - 0
packages/code-runtime/code-runtime-worker/tests/bootstrap.spec.ts

@@ -108,6 +108,16 @@ describe('prepareValue', () => {
     const { value } = prepareValue('x'.repeat(50), 10)
     expect(value).toBe(`${'x'.repeat(10)}… [truncated]`)
   })
+
+  it('measures a container by its structured-clone wire size, not its bounded rendering', () => {
+    // The bounded inspect rendering of a huge array is tiny ("... N more
+    // items"), but its real cross-boundary size is not — the cap must catch
+    // it, replacing the value with that bounded rendering.
+    const huge = new Array(50_000).fill(7)
+    const { value } = prepareValue(huge, 1_000)
+    expect(typeof value).toBe('string')
+    expect(value).toContain('more items')
+  })
 })
 
 describe('makeNamespaces', () => {

+ 8 - 0
packages/code-runtime/code-runtime-worker/tests/runtime.spec.ts

@@ -221,6 +221,14 @@ describe('WorkerCodeRuntime — budgets and containment (real workers)', () => {
     expect(result.value).toBe(`${'y'.repeat(64)}… [truncated]`)
   })
 
+  it('caps a huge container whose bounded rendering is small (wire size, not rendering, is what counts)', async () => {
+    const { runtime } = await setup()
+    const result = await runtime.run({ program: 'return new Array(50_000).fill(7)', bindings: [] })
+    expect(result.error).toBeUndefined()
+    expect(typeof result.value).toBe('string')
+    expect(result.value).toContain('more items')
+  })
+
   it('captures pipe writes that bypass the patched write slot as stray logs, capped by the same budget', async () => {
     const { runtime } = await setup({ maxLogBytes: 4 })
     const result = await runtime.run({

+ 28 - 11
packages/code-runtime/code-runtime-worker/tsdown.config.ts

@@ -4,15 +4,32 @@ import { defineConfig } from 'tsdown'
  * Package-shape override (see the root tsdown.config.ts): besides the
  * default lib/index.js bundle, the worker BOOTSTRAP ships as its own
  * sibling entry — `new Worker(new URL('./worker.js', import.meta.url))`
- * loads it as a file, so it cannot be part of the index bundle.
+ * loads it as a file, so it cannot be part of the index bundle. TWO
+ * single-entry builds, not one two-entry build: a multi-entry build emits
+ * the shared bootstrap module as a `lib/bootstrap-*.js` chunk both bundles
+ * import, which the package.json `files` whitelist (deliberately exact)
+ * would omit from the packed artifact — each single-entry build inlines its
+ * own bootstrap copy instead, keeping every shipped file self-contained.
  */
-export default defineConfig({
-  entry: ['lib/types/index.js', 'lib/types/worker.js'],
-  outDir: 'lib',
-  format: ['esm'],
-  platform: 'node',
-  target: 'es2024',
-  fixedExtension: false,
-  dts: false,
-  clean: false,
-})
+export default defineConfig([
+  {
+    entry: ['lib/types/index.js'],
+    outDir: 'lib',
+    format: ['esm'],
+    platform: 'node',
+    target: 'es2024',
+    fixedExtension: false,
+    dts: false,
+    clean: false,
+  },
+  {
+    entry: ['lib/types/worker.js'],
+    outDir: 'lib',
+    format: ['esm'],
+    platform: 'node',
+    target: 'es2024',
+    fixedExtension: false,
+    dts: false,
+    clean: false,
+  },
+])