feat(cli)!: stop reading e2b.toml outside template migrate - #1761
feat(cli)!: stop reading e2b.toml outside template migrate#1761devin-ai-integration[bot] wants to merge 1 commit into
Conversation
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
🦋 Changeset detectedLatest commit: ecadd91 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Package ArtifactsBuilt from 0bb6539. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.45.1-devin-1787401629-cli-drop-e2b-toml.0.tgzCLI ( npm install ./e2b-cli-2.17.2-devin-1787401629-cli-drop-e2b-toml.0.tgzPython SDK ( pip install ./e2b-2.45.1+devin.1787401629.cli.drop.e2b.toml-py3-none-any.whl |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecadd91fa4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@e2b/cli': minor | |||
There was a problem hiding this comment.
Removing the accepted --config and --path flags and changing no-argument template resolution are breaking changes for existing scripts, as the changeset itself describes. Marking this as minor allows consumers using a compatible ^2 range to receive the release automatically and then encounter unknown-option failures or silently create the base template; this changeset should request a major CLI bump.
Useful? React with 👍 / 👎.
| if (!templateID) { | ||
| templateID = 'base' | ||
| } | ||
| const templateID = template ?? 'base' |
There was a problem hiding this comment.
This changes no-argument sandbox create to always use base, while the same patch changes argument and option handling for delete, publish, and unpublish and deletes their only tests without adding replacement coverage. Add tests proving that these commands ignore e2b.toml, reject the removed flags, and preserve argument and interactive selection behavior; the repository explicitly requires affected codepaths to be covered and tested.
AGENTS.md reference: AGENTS.md:L6-L6
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed against TASTE.md. Note that TASTE.md governs the JS/Python SDK surfaces; this PR is CLI-only, so I applied the rules that carry over to a user-facing surface — API shape and naming (T-1a, T-12, T-19, T-23), defaults (T-47), deprecation/compatibility (T-65-T-67), and error/help actionability (T-62-T-63). Parity (T-1/T-2), streaming, template-builder, timeout and error-hierarchy sections have nothing changed here to check.
6 findings, 5 distinct issues (the template_id one appears in two files):
- Removed
--config/--pathwith no deprecated forwarder, so previously valid invocations now die on commander's genericunknown option(T-65/T-66/T-62). configOptiondeleted from the sharedoptions.tsand re-inlined anonymously inmigrate.ts, losing the "use the new build system" pointer (T-65/T-23).- Hand-rolled
{ template_id: string }TS shapes, duplicated inline indelete.tsandpublish.ts(T-1a/T-12/T-19). 'base'as a magic literal default at the call site (T-47).sandbox createsilently ignores a presente2b.tomlinstead of saying so (T-62).
Not tied to a line: the two deleted test files remove the only coverage of these commands, and nothing replaces it — worth a test that template delete with no argument prints the actionable message, and that sandbox create with an e2b.toml present now starts base. On the good side, dropping the trailing optional configLocalPath positional from asFormattedSandboxTemplate moves that helper toward T-3, and confining e2b.toml parsing to migrate is the right home for it (T-31's spirit).
| if (!templateID) { | ||
| templateID = 'base' | ||
| } | ||
| const templateID = template ?? 'base' |
There was a problem hiding this comment.
T-47 — defaults belong in a named module constant, never a bare literal at the call site (DEFAULT_TEMPLATE_NAME in template/init.ts is the local precedent).
| const templateID = template ?? 'base' | |
| const templateID = template ?? DEFAULT_TEMPLATE |
with const DEFAULT_TEMPLATE = 'base' at module scope, and the default documented on the [template] argument description so --help says what you get when you omit it.
Separately, this is where the change is silent rather than actionable (T-62: say what to do, not just what happened): a user in a directory with e2b.toml used to see Found sandbox template …, and now gets a base sandbox with no indication their config was ignored. Since getConfigPath/loadConfig still exist for template migrate, cheap fix: if an e2b.toml is present and no [template] was passed, print one line pointing at e2b template migrate before falling back to the default.
| const templates: (Pick<E2BConfig, 'template_id'> & { | ||
| configPath?: string | ||
| })[] = [] | ||
| const templates: { template_id: string; aliases?: string[] }[] = [] |
There was a problem hiding this comment.
T-1a / T-12 / T-19 — a hand-written TS shape should be camelCase (templateId), not the toml/wire spelling template_id. The snake_case was inherited from Pick<E2BConfig, 'template_id'>; now that E2BConfig is gone from this file there is nothing left to justify it, and the same local type is duplicated inline here and in publish.ts (T-19: name the shape, don't inline it).
Better still, the only consumer of this array is asFormattedSandboxTemplate plus deleteTemplate(e.template_id), so hold the values in exactly the shape the formatter takes and delete the three { templateID: e.template_id, aliases: e.aliases } re-mappings:
const templates: { templateID: string; aliases?: string[] }[] = []
// …
templates.push({ templateID: template, aliases: undefined })
// …
templates.push(...selectedTemplates.map((e) => ({ templateID: e.templateID, aliases: e.aliases })))
// …
templates.forEach((e) => console.log(asFormattedSandboxTemplate(e)))| const templates: (Pick<E2BConfig, 'template_id'> & { | ||
| configPath?: string | ||
| })[] = [] | ||
| const templates: { template_id: string; aliases?: string[] }[] = [] |
There was a problem hiding this comment.
T-1a / T-12 / T-19 — same as delete.ts: template_id is the toml/wire spelling, not how our own TS types are named, and this inline shape is now literally duplicated across two commands. Extract one named type (camelCase templateID, matching what asFormattedSandboxTemplate accepts) and share it, which also removes the { templateID: e.template_id, aliases: e.aliases } re-mapping at both log sites below.
| .addOption(configOption) | ||
| .addOption( | ||
| new commander.Option( | ||
| '--config <e2b-toml>', |
There was a problem hiding this comment.
T-65 / T-23 — the shared, exported configOption was deleted and re-inlined here as an anonymous commander.Option. A deprecated-but-still-live knob stays a named export with a comment; inlining it also silently drops the sentence the old option carried ("We recommend using the new build system (https://e2b.dev/docs/template/defining-template) that does not use config files"), which is exactly the guidance a user reading migrate --help needs.
Keep the option in src/options.ts — annotated as the migrate-only reader — and call .addOption(configOption) here instead of building an inline one. That also makes the new asBold import unnecessary again.
| )} directory` | ||
| ) | ||
|
|
||
| export const configOption = new commander.Option( |
There was a problem hiding this comment.
T-65 / T-66 — hard removal with no forwarder. e2b template delete --config ./e2b.toml and e2b sandbox create --path … now fail with commander's generic error: unknown option '--config', which names nothing the user can act on (T-62). The compliant shape for a breaking rename/removal is to keep the identifier and the flag around, hidden and inert, with the migration path in the message:
// Deprecated: only `template migrate` reads e2b.toml. Removed in the next major.
export const configOption = new commander.Option('--config <e2b-toml>', '…').hideHelp()and have the commands that used to accept it print "--config is no longer read here — pass the template as [template] or select it with -s; e2b template migrate --config still reads e2b.toml" instead of erroring out opaquely.
| .addOption(pathOption) | ||
| .addOption(configOption) | ||
| .description('delete sandbox template') | ||
| .argument('[template]', `specify ${asBold('[template]')} to delete it`) |
There was a problem hiding this comment.
T-62 — the new argument description says what the flag does but not what to do when it is omitted, and omitting it is now the common failure (it used to fall back to e2b.toml). The removed else branch had the actionable message; the argument help should carry it now, e.g. specify [template] to delete it, or select templates interactively with -s.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good — clean, mechanical removal of e2b.toml-based template resolution from sandbox create, template delete, and template publish/unpublish, applied consistently. Checked: the no-template/no--s fallback message in delete.ts/publish.ts still fires correctly now that the else config-loading branch is gone; pathOption/getConfigPath/loadConfig are still used by template migrate/create/init and weren't orphaned; migrate.ts's new inline --config Option text matches the removed shared one; the deleted integration tests (delete.test.ts, publish.test.ts) tested the now-removed --config/--path flags directly, so removal rather than porting is appropriate; changeset accurately describes the breaking behavior and uses "minor" consistent with this repo's existing convention for breaking CLI/SDK removals (e.g. the accessToken removal changeset).
Extended reasoning...
Overview
The PR removes e2b.toml config-file resolution from sandbox create, template delete, and template publish/unpublish, deleting the shared configOption/pathOption usage and deleteConfig() helper, and simplifying asFormattedSandboxTemplate to drop the config-path suffix. template migrate is untouched behaviorally but now owns a local, textually-identical --config Option since it's the only remaining reader of e2b.toml. Two integration tests that specifically exercised the removed --config/--path flags on delete/publish are deleted outright.
Security risks
None. This is CLI argument/config plumbing with no auth, crypto, or data-exposure surface touched.
Level of scrutiny
Low-to-medium. This is a non-SDK, JS-only CLI change, mechanical in nature (deleting code paths and updating call sites), but it is a public/user-facing breaking behavior change, so I checked call sites for orphaned imports/exports and confirmed the removed helpers (getRoot, getConfigPath, loadConfig, pathOption) are still legitimately used by unaffected commands (template migrate, template create, template init). I also verified the fallback "no template specified" messaging in delete.ts/publish.ts, which predates this change, still triggers correctly with the config-loading else branch removed.
Other factors
A changeset is present and accurately describes the breaking removal; per repo convention (checked against a prior similar removal, remove-access-token-option.md), "minor" is the established bump level for breaking-but-intentional CLI/SDK removals in this repo, not a slip. The deleted tests were tightly coupled to the removed --config/--path flags and could not be meaningfully adapted; no new test was added for the "template omitted, no -s" fallback path, but that message is unchanged pre-existing behavior, not new logic introduced by this PR, so its absence is a minor gap rather than a defect.
1 participant
Summary
e2b.tomlis no longer required, so the CLI stops resolving templates from it.sandbox create,template deleteandtemplate publish/unpublishnow take the template only as an argument (or via-s), and their--config/--pathoptions are gone.template migratekeeps parsinge2b.toml— it is now the only reader, and owns the--configoption locally instead of importing the sharedconfigOption.Behavior change (breaking for config-file users):
Also dropped
deleteConfig()(the delete command was its only caller) and theconfigLocalPathargument ofasFormattedSandboxTemplate, so template lines no longer render the<-> ./e2b.tomlsuffix. The--configtests for delete/publish are removed with the feature; migrate's tests are unchanged and still pass.Usage
e2b template delete my-template-id e2b template publish -s e2b sandbox create my-template-id # unchanged e2b template migrate --config ./e2b.tomlLink to Devin session: https://app.devin.ai/sessions/1471bde8f45d4d7e937e69a3bf0a3eae
Requested by: @mishushakov