refactor(sandbox): inline tool prefixing into getTools implementations

[gautamsirdeshmukh]

Description

• Move tool prefixing into getTools() implementations

Type of Change

Refactor

Testing

How have you tested the change? Verify that the changes do not break functionality or introduce new warnings.

• I ran hatch run prepare

Checklist

• I have read the CONTRIBUTING document
• I have reviewed and understand every line of code in this PR, including any generated by AI tools, and I can explain why it works
• My change is focused and reasonably small; I have split unrelated work into separate PRs
• I have added any necessary tests that prove my fix is effective or my feature works
• I have updated the documentation accordingly
• I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
• My changes generate no new warnings
• Any dependent changes have been merged and published

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[github-actions]

Assessment: Comment

Clean, well-scoped fix. Converting the module-level singleton into a per-agent factory correctly addresses the shared-state bug, and the ??= memoization in Agent.sandbox gives each unconfigured agent its own stable instance. The new tests cover the important behaviors (fresh-per-call factory, per-agent isolation, memoization).

Review notes

• Correctness: Core change is sound; sandbox: false fall-through behavior is preserved and tested.
• Consistency: One gap — the createMockAgent test fixture drops the memoization the real getter now has, so its sandbox semantics diverge from the production Agent. See inline comment.
• API surface: defaultSandboxFactory is internal (not exported from index.ts), so no API bar-raising needed.
• Tests: Reference-equality assertions are appropriate here; no per-field-assertion concerns.

Nice, focused bug fix — the per-agent memoization reads cleanly.

[github-actions]

Assessment: Request Changes

Re-reviewed after the latest push. The memoization feedback on the mock fixture was fully addressed — thank you. However, this push introduced a new change that needs attention before merge.

Re-review notes

• Resolved: createMockAgent now memoizes its default sandbox, matching the real Agent.sandbox getter.
• New — Critical: The sandbox: false opt-out was removed from the public AgentConfig type, dropping a forward-looking escape hatch and its test. This is a breaking change to an exported contract and is out of scope for a "per-agent default" bug fix. See inline comment for options (revert here, or split + api/needs-review + migration notes).
• Scope: Diff now spans the original singleton→per-agent change and an unrelated public-API removal. CONTRIBUTING/AGENTS guidance asks for one logical change per PR.

The core singleton→per-agent fix remains clean; the blocker is purely the bundled sandbox: false removal.

[agent-of-mkmeral]

Assessment: Request Changes 🔴

Reviewed the works on 9870aa7 — independently cloned, type-checked (src project clean), and ran the full unit-node suite.

✅ The core fix is clean and correct

The singleton→per-agent factory conversion is exactly right and fixes a real shared-state bug:

• defaultSandboxFactory stores () => Sandbox instead of a shared instance, so each unconfigured agent builds its own host default.
• Agent.sandbox memoizes via this._defaultSandbox ??= defaultSandboxFactory.get()() — stable instance per agent across repeated reads (important: sandbox-aware tools read agent.sandbox multiple times and rely on a shared cwd/filesystem).
• The earlier mock-fixture feedback (memoize createMockAgent's getter to mirror the real semantics) was fully addressed — verified it now matches.
• New tests cover the right behaviors: fresh-per-factory-call, per-agent isolation, and per-agent memoization.

Verification results:

• tsc --project src/tsconfig.json → ✅ clean (0 errors)
• Sandbox unit tests → ✅ 106/106 pass (incl. the 6 changed default-sandbox tests)
• Full unit-node → 3493 pass. The only failures (4 in telemetry/__tests__/config.test.ts) and integ type errors (steering.test.node.ts) are pre-existing on main and unrelated to this PR (confirmed by re-running against main).

🔴 Blocker: the bundled sandbox: false removal is out of scope — and it's a breaking change to a released contract

This PR also drops sandbox?: Sandbox | false → sandbox?: Sandbox from the public AgentConfig (inline comment below). I want to add one fact the prior re-review didn't establish: I checked git merge-base --is-ancestor efa837af typescript/v1.5.0 and #2563 (which introduced sandbox: false) is already shipped in typescript/v1.5.0. So this isn't removing an unreleased placeholder — it's removing an option from an exported type that users may already be passing. Anyone with new Agent({ model, sandbox: false }) will get a compile error after this lands.

Yes, false is behaviorally a no-op today (this._sandbox is falsy → falls through to the default, and the tool-registration if (this._sandbox) block is skipped). So removing it is harmless at runtime but type-breaking at the source level.

Per AGENTS.md ("One logical change per PR") and CONTRIBUTING.md (breaking changes → design proposal / API bar-raising), this belongs in its own PR. Two clean options:

1. Revert the sandbox: false removal here (keep this PR a pure singleton→per-agent fix — my preference; smallest, mergeable now), or
2. Split it out into a dedicated PR with api/needs-review, a changeset/migration note, and the treats sandbox: false the same as unconfigured test handled there.

The singleton→per-agent fix is ready to merge the moment the sandbox: false change is unbundled. Nice work on the memoization — it reads cleanly. 👍

[github-actions]

Assessment: Request Changes

Re-reviewed after the force-push (54fe09e). The core singleton→per-agent fix and the mock memoization remain clean and correct — but the blocker is still unaddressed, and the scope grew further.

Re-review notes

• Still open — sandbox: false removal: The agent.ts:305 thread is marked resolved, but the code is unchanged — AgentConfig.sandbox is still narrowed to Sandbox, and the treats sandbox: false the same as unconfigured test is still deleted. As established, sandbox: false shipped in typescript/v1.5.0, so this remains a source-level breaking change. (Marking the thread resolved without a code change doesn't resolve the concern.)
• New — Critical regression: This push also removed ExecuteOptions, StreamType, StreamChunk, FileInfo, OutputFile, ExecutionResult from index.ts, while Sandbox stays exported. Those types are in Sandbox's public method signatures, so consumers can no longer name its return/param types. See inline comment.
• Scope: A "create the default per-agent" bug fix now bundles two separate public-API reductions. Per AGENTS.md (one logical change per PR) and CONTRIBUTING (breaking changes → API bar-raising), both API trims should be unbundled.

To unblock: keep this PR to the singleton→per-agent change (revert both the sandbox: false and index.ts export removals), and handle any intended public-surface trimming in a separate api/needs-review PR.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[github-actions]

Assessment: Request Changes

Re-reviewed after d9f7d1e (title now: "refactor(sandbox): per-agent default, vended-sandboxes folder, export cleanup"). The original bug fix is now buried inside a much larger, multi-concern refactor. The fix itself is still correct, but the PR has grown well past what can be safely reviewed or merged as a unit.

This PR now bundles at least five distinct changes, spanning both TypeScript and Python:

Distinct changes bundled into this PR

1. Singleton → per-agent default sandbox (the original, correct bug fix — TS).
2. sandbox: false removal from the public AgentConfig (still unaddressed; breaking, shipped in typescript/v1.5.0).
3. Root export cleanup — 6 sandbox types + Sandbox*Error classes removed from package-root index.ts. Partially mitigated (now reachable via the ./sandbox subpath) but still a root-import breaking change. See thread.
4. vended-sandboxes/ folder reorganization — Docker/SSH sandboxes physically moved (TS and Python: new strands.vended_sandboxes package), with package.json subpath renames (./sandbox/docker → ./vended-sandboxes/docker) — a breaking change to documented import paths.
5. toolPrefix removal + tool renaming — public Sandbox.toolPrefix dropped; tools now hardcoded as sandbox_file_editor / sandbox_bash. Behavioral + API change. See inline comment.

This directly conflicts with the PR's own checklist ("My change is focused and reasonably small; I have split unrelated work into separate PRs"), with AGENTS.md (one logical change per PR), and with CONTRIBUTING (breaking changes → API bar-raising). It also still carries the original bug type/title mismatch — the description still says only "create the default per-agent."

Strong recommendation: Reset this PR to just change #1 (the singleton→per-agent fix, which was ready to merge three pushes ago). Each of #2–#5 is a legitimate change, but each is an independent, separately-reviewable API/behavioral decision and should be its own PR with api/needs-review + migration notes. Bundling them here means none of them gets the focused review it needs, and a clean, mergeable bug fix is being held hostage to four unrelated refactors.

[github-actions]

Assessment: Approve

Re-reviewed after 53b5780 and the "discussed offline, removing everything except TS tool prefixing move" note. The scope is now tight and clean — this is purely the TS sandbox tool-prefixing refactor (6 files). All the previously-bundled concerns I flagged in earlier rounds (TS root export removal, Python __init__.py changes, sandbox: false) have been removed from this PR. 👍

What the PR now does, and it does it well:

• agent.ts — removes the runtime Object.create prefixing in the registration loop; tool names are now baked in by each sandbox's getTools(). The if (this._sandbox) guard still correctly handles the false/undefined cases, and the user-override skip behavior is preserved.
• base.ts — drops the now-unused toolPrefix field.
• docker.ts / ssh.ts — hardcode sandbox_file_editor / sandbox_bash directly in getTools().
• tests — updated to assert the new names.

Verification:

• ✅ No dangling toolPrefix references anywhere in src/.
• ✅ New tool names are consistent across source and tests.
• ✅ Changed files type-check clean (the only tsc errors in this env are missing @types/node / vite/client packages — an environment dependency gap, not code).

One thing to note for the record: the file-editor tool name moves from sandbox_fileEditor → sandbox_file_editor (snake_case). I'm treating that as intentional given the offline discussion and the PR title. The two newer inline questions from @mkmeral (agent.ts:305, base.ts:15) point at lines that are no longer part of this reduced diff, so they appear to be already resolved by the scope reduction — worth a quick confirm/resolve on those threads.

Nice tightening of the change — much easier to reason about now that it's a single focused refactor.