Skip to content

refactor: extract shared registerNodeDefaults() to prevent test/prod drift in sandbox registration (follow-up to #2563) #2736

@agent-of-mkmeral

Description

@agent-of-mkmeral

TL;DR: Follow-up from the blocking review thread on #2563 (now merged) — deferred by @gautamsirdeshmukh to a follow-up PR; filing so the context doesn't get lost. Latent drift risk: the vitest fixture src/__fixtures__/register-node-defaults.ts duplicates the production default-sandbox registration from src/index.node.ts. Both are identical one-liners today (nothing broken), but a future change updating one site and not the other would make unit tests silently diverge from production. Verified fix below (3 files, 61/61 tests pass, bundle check green). ⚠️ Do NOT use the naive import '../index.node.js' fix — it breaks 32 tracer tests (details collapsed).

✅ The verified patch (3 files, copy-paste ready)

Extract a shared registerNodeDefaults() called by both the prod entry point and the test fixture, so they can't drift:

// src/sandbox/register-node-defaults.ts  (new)
import { defaultSandbox } from './default.js'
import { NotASandboxLocalEnvironment } from './not-a-sandbox-local-environment.js'

/**
 * Registers the Node-specific default sandbox. Called by the production Node
 * entry point (index.node.ts) and by the unit-node vitest setup file, so the
 * two can never drift.
 */
export function registerNodeDefaults(): void {
  defaultSandbox.set(new NotASandboxLocalEnvironment())
}
// src/index.node.ts
// (keep the existing side-effect warning comment)
import { registerNodeDefaults } from './sandbox/register-node-defaults.js'

registerNodeDefaults()

export * from './index.js'
// src/__fixtures__/register-node-defaults.ts
// In production, index.node.ts registers this on import. Tests don't go through that
// entry point (importing it here would load the full SDK module graph before vi.mock
// can intercept, breaking module mocks), so this setup file calls the same shared
// registration function instead.
import { registerNodeDefaults } from '../sandbox/register-node-defaults.js'

registerNodeDefaults()
⚠️ Why the naive import fix breaks 32 tracer tests

Making the fixture import '../index.node.js' directly looks right but breaks 32 of 36 tracer tests: vitest setup files run before test files, so the import pulls the entire SDK module graph into the module cache before vi.mock in any test file can intercept — module mocks silently stop applying. The shared-function approach keeps the fixture's import graph tiny (just the sandbox modules), so mocks keep working.

🔬 Verification receipts (run on #2563 head de6085a8 pre-merge)
  • agent.tracer.test.node.ts + default.test.node.ts + default-slot.test.ts + not-a-sandbox-local-environment.test.node.ts61/61 pass, no type errors (the tracer suite is the canary for the vi.mock pitfall)
  • check:browser-bundle → passes, 412.5kb unchanged (index.node.ts stays on the sideEffects allowlist)
  • Browser behavior unchanged: nothing imports the new module from index.ts, so the browser graph never registers a default

cc @gautamsirdeshmukh @mkmeral

Metadata

Metadata

Assignees

No one assigned

    Labels

    area-devxDeveloper experience improvementsarea-serverRelated to strands Server, sandbox, runtime containerchoreMaintenance tasks, dependency updates, CI changes, refactoring with no user-facing impacttypescriptPull requests that update typescript code

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions