feat(template): replace third-party Dockerfile parsers with @e2b/dockerfile-utils / e2b-dockerfile-utils (BuildKit-style parser + .dockerignore matcher) - #1936
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
🦋 Changeset detectedLatest commit: f586297 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Package ArtifactsBuilt from 54b0ae7. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-dockerfile-utils-0.0.1-devin-1790966038-dockerfile-parser.0.tgz ./e2b-2.52.2-devin-1790966038-dockerfile-parser.0.tgzCLI ( npm install ./e2b-cli-2.21.2-devin-1790966038-dockerfile-parser.0.tgzCode Interpreter JS SDK ( npm install ./e2b-code-interpreter-2.8.1-devin-1790966038-dockerfile-parser.0.tgzDesktop JS SDK ( npm install ./e2b-desktop-2.4.1-devin-1790966038-dockerfile-parser.0.tgzPython SDK ( pip install ./e2b_dockerfile_utils-0.0.0+devin.1790966038.dockerfile.parser-py3-none-any.whl ./e2b-2.52.1+devin.1790966038.dockerfile.parser-py3-none-any.whlCode Interpreter Python SDK ( pip install ./e2b_code_interpreter-2.10.1+devin.1790966038.dockerfile.parser-py3-none-any.whlDesktop Python SDK ( pip install ./e2b_desktop-2.6.1+devin.1790966038.dockerfile.parser-py3-none-any.whl |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| const SHELL_SAFE = /^[A-Za-z0-9_@%+=:,./-]+$/ | ||
|
|
||
| /** Quote a word for POSIX shells (same rules as Python's `shlex.quote`). */ | ||
| export function shellQuote(word: string): string { |
There was a problem hiding this comment.
T-43 — shell interpolation goes through the SDK's shellQuote() (JS) / shlex.quote() (Python). The Python side of this PR does use shlex.quote, but the JS side adds a second, exported shellQuote (plus its own SHELL_SAFE regex) next to the existing shellQuote in src/utils.ts that readycmd.ts and template/index.ts already use. Two quoting implementations can drift, and the exported name now shadows the shared helper for anyone importing from this module.
Compliant form — delete SHELL_SAFE and this function (lines 88–100) and reuse the shared one:
import { shellQuote } from '../utils'
| convert(instructions: DockerfileInstruction[]): DockerfileParseResult { | ||
| const fromInstructions = instructions.filter((i) => i.name === 'FROM') | ||
| if (fromInstructions.length > 1) { | ||
| throw new Error('Multi-stage Dockerfiles are not supported') |
There was a problem hiding this comment.
T-42 — builder precondition failures raise BuildError / BuildException, never a bare Error / ValueError. The rewritten converter throws DockerfileSyntaxError for every other rejection (unknown flag, COPY --from, remote ADD, …) but these two still throw a bare Error, so callers need two catch paths for "this Dockerfile isn't supported".
| throw new Error('Multi-stage Dockerfiles are not supported') | |
| throw new DockerfileSyntaxError('Multi-stage Dockerfiles are not supported', fromInstructions[1].startLine) |
Same for line 134 (throw new DockerfileSyntaxError('Dockerfile must contain a FROM instruction')), and the Python mirror in _DockerfileConverter.convert (dockerfile_parser.py lines 137/139, still raise ValueError(...)).
| } from './dockerfile/syntax' | ||
| import { ShellLex } from './dockerfile/lexer' | ||
|
|
||
| export { DockerfileSyntaxError } |
There was a problem hiding this comment.
T-54 — one flat entry point per package: everything public is re-exported from index.ts / listed in e2b/__init__.py's __all__. DockerfileSyntaxError is a new user-facing thrown type (and carries a public line field), but it's only re-exported from this internal module and from e2b/template/dockerfile_parser.py's __all__ — neither src/index.ts nor e2b/__init__.py exposes it, so users can't instanceof / except it without a deep import.
Compliant form: add it next to BuildError in packages/js-sdk/src/index.ts's errors export, and add the Python exception to the from .exceptions import … / __all__ lists in packages/python-sdk/e2b/__init__.py (ideally after moving it to errors.ts / exceptions.py alongside the other BuildError subclasses).
TASTE.md review summaryChecked the changed code against: T-1/T-1a/T-1c (JS ↔ Python parity), T-42 (builder failures → 5 violations, posted as inline comments:
Not tied to a changed line:
Note: I posted these as separate inline comments, not one batched review, because the GitHub CLI credentials weren't available in this run. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
devin-ai-integration
Bot
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
resolve conflicts and check remaining comments |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
there's more to resolve |
|
Merged |
…BuildKit-style parser Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
…ils and e2b-docker-utils packages Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
…d preview artifacts
- COPY heredocs run as root (Docker COPY writes as the builder, not USER)
- heredoc terminators are chosen so they never occur in the content
- heredoc markers are detected on raw tokens, so COPY "<<EOF" is a file name
- --chmod=000 is preserved (builders check mode for undefined/None, not truthiness)
- flag lookup uses Object.hasOwn so --toString etc. are rejected as unknown
- bare CMD/ENTRYPOINT no longer raise IndexError in Python
- Python parse_heredoc uses the same linear marker scan as JS (no regex)
- RecursionError from nested ${…} is reported as a parse error in Python
- pkg_artifacts: pack/build docker-utils before the SDKs and pin the preview
SDKs to the exact preview docker-utils version so both artifacts install together
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
…move the .dockerignore matcher into it The packages become @e2b/dockerfile-utils (packages/dockerfile-utils-js) and e2b-dockerfile-utils / e2b_dockerfile_utils (packages/dockerfile-utils-python). PatternMatcher moves from the SDKs' template/dockerignore modules into the packages. It no longer depends on node:path (Go filepath.Clean is reimplemented on slash paths) and takes a backslashIsSeparator / backslash_is_separator option instead of reading the platform separator; invalid patterns raise a plain Error / ValueError, which the SDKs' getAllFilesInPath wrap into TemplateError / TemplateException as before. Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
6f7f8d7 to
fe1f769
Compare
…tcher move Updates the references, lockfiles and SDK imports for the rename, adds the moved PatternMatcher (plain Error / ValueError, backslashIsSeparator option, no node:path) with its tests, and wraps its errors into TemplateError / TemplateException in the SDKs' getAllFilesInPath. Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
devin-ai-integration
Bot
changed the title
| def _match_heredoc_marker(word: str) -> Optional[Tuple[str, bool, str]]: | ||
| """Split ``[fd]<<[-]name`` into its parts without a backtracking regex.""" | ||
| i = 0 | ||
| while i < len(word) and word[i].isdigit(): |
There was a problem hiding this comment.
🟡 Superscript filenames abort Dockerfile parsing
For a source named ²<<EOF, parse_heredoc treats ² as a file descriptor. Python's int rejects it, so the Dockerfile fails instead of copying that literal filename.
Was this helpful? React with 👍 or 👎 to provide feedback.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ea72fd4b2
ℹ️ 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".
| heredoc.chomp ? chompHeredocContent(heredoc.content) : heredoc.content | ||
|
|
||
| // `RUN <<EOF` on its own: the heredoc body is the script itself. | ||
| if (heredocs.length === 1 && parseHeredoc(line.trim())) { |
There was a problem hiding this comment.
For a valid RUN 3<<EOF ... EOF, this condition treats the heredoc body as the command merely because it is the only heredoc. Docker instead runs a redirection-only command with the content bound to file descriptor 3, so the body is not executed; the converted template executes it on normal stdin (for example, a touch in the body gains an unintended side effect). Restrict the direct-body shortcut to descriptor 0, or retain the original command, and apply the equivalent correction to the Python converters.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
| "scripts": { | ||
| "version": "pnpm changeset version && pnpm run -r postVersion && pnpm run -r lock", | ||
| "publish": "pnpm --dir packages/js-sdk build && pnpm run -r postPublish && pnpm changeset publish", | ||
| "publish": "pnpm --dir packages/dockerfile-utils-js build && pnpm --dir packages/js-sdk build && pnpm run -r postPublish && pnpm changeset publish", |
There was a problem hiding this comment.
🔴 New PyPI users can hit a broken pip install e2b right after a release, because publishing e2b and e2b-dockerfile-utils isn't actually ordered. pnpm run -r postPublish (package.json:6) only sequences packages that declare a workspace dependency in package.json; js-sdk gets this via "@ e2b/dockerfile-utils": "workspace:^", but packages/python-sdk/package.json has no such entry, so pnpm may run the two Python packages' postPublish (uv build && uv publish) concurrently/in arbitrary order. If e2b publishes before e2b-dockerfile-utils (required at >=0.1.0,<0.2, pyproject.toml:23) lands on PyPI, pip install e2b fails to resolve. Fix: give pnpm an explicit ordering signal for this Python-package pair, e.g. a workspace-protocol dependency edge or an explicit sequential publish step.
Why this was flagged
On the release that first publishes both @ e2b/python-sdk and @ e2b/dockerfile-utils-python together, publish_command.cjs selects pnpm run publish, which runs pnpm run -r postPublish (package.json:6). pnpm orders postPublish scripts topologically from package.json dependencies only; packages/python-sdk/package.json and packages/dockerfile-utils-python/package.json have no dependency field linking them, unlike the JS pair linked via workspace:^ in packages/js-sdk/package.json. Without that edge, pnpm can run their uv build && uv publish steps in parallel or arbitrary order, not guaranteed 'dockerfile-utils-python first'. packages/python-sdk/pyproject.toml:23 now requires e2b-dockerfile-utils>=0.1.0,<0.2 for the published e2b wheel to install. If python-sdk's publish completes first, pip install e2b fails with an unresolvable dependency for any user installing during that window — a failure mode that did not exist before this diff. No CI step enforces the ordering the PR description assumes.
Verification: The release publish path at package.json:6 runs pnpm run -r postPublish. Both Python packages are published only via postPublish (uv build && uv publish); packages/python-sdk/package.json and packages/dockerfile-utils-python/package.json both carry only scripts, no dependencies field.
| /** A heredoc delimiter that does not occur in the content it wraps. */ | ||
| function heredocTerminator(name: string, content: string): string { | ||
| let terminator = `E2B_HEREDOC_${name}` | ||
| while (content.includes(terminator)) { | ||
| terminator += '_' | ||
| } | ||
| return terminator | ||
| } |
There was a problem hiding this comment.
🔴 Calling Template.fromDockerfile/from_dockerfile on a heredoc (RUN/COPY <<EOF) whose body contains the right pattern can hang the process; the base parsers never had this step. heredocTerminator (dockerfileParser.ts:102-107) and its Python twin heredoc_terminator (dockerfile_parser.py:111-116) grow the delimiter one underscore at a time and rescan the whole heredoc content on every iteration, so content containing 'E2B_HEREDOC' followed by N underscores makes this O(N^2) with no cap on N or on iterations. Fix: pick the terminator in one pass (e.g. find the longest run of trailing '_' after any occurrence of the base string and append one more) instead of an unbounded retry-and-rescan loop, for both sites. Same pattern at packages/python-sdk/e2b/template/dockerfile_parser.py:111-116.
Why this was flagged
Input: a RUN/COPY heredoc (<<EOF) body containing a line like 'E2B_HEREDOC_main' followed by a long run of underscores, reachable via Template.fromDockerfile -> DockerfileConverter.runWithHeredocs (dockerfileParser.ts:358) or handleCopy's heredoc path (dockerfileParser.ts:461), and the matching Python call sites (dockerfile_parser.py:315, 401). heredocTerminator/_heredoc_terminator loop while content.includes(terminator) appending one char per pass, each pass rescanning the full content: O(content_length^2). No size cap exists on heredoc content and no iteration cap on the loop. The pre-change parser (dockerfile-ast/dockerfile-parse) never synthesizes a heredoc terminator, so this quadratic path is newly introduced and can hang the SDK process on a crafted Dockerfile.
Verification: The quadratic loop is real and unbounded. JS dockerfileParser.ts:102-107 appends one underscore per pass and rescans the full content each pass; Python dockerfile_parser.py:111-116 is identical. Both are reached from user heredoc bodies via runWithHeredocs through Template.fromDockerfile/from_dockerfile, giving O(M^2) with no cap, and the prior dockerfile-ast/dockerfile-parse parsers had no such step.
…shell test suites - dockerignore: validate patterns like Go's filepath.Match (bad bracket expressions, trailing backslash), reject a lone '!', expose cleaned 'patterns', normalize backslash paths in matches()/mayMatchUnder() when backslashIsSeparator is set - syntax: parse HEALTHCHECK arguments instead of ignoring them - tests: moby/patternmatcher tables, BuildKit parser testfiles (+negative, line numbers, heredocs, JSON, directives) and shell wordsTest/envVarTest fixtures for both JS and Python Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
This branch has not been deployed
2 participants
Summary
Template.fromDockerfile/Template.from_dockerfileno longer depend ondockerfile-ast(JS) ordockerfile-parse(Python). Both SDKs now use the same parser design, ported from BuildKit'sfrontend/dockerfile/parser+frontend/dockerfile/shell, shipped as two new dependency-free packages (which also host the.dockerignorematcher):e2bdepends on them ("@e2b/dockerfile-utils": "workspace:^",e2b-dockerfile-utils>=0.1.0,<0.2via atool.uv.sourcespath source — a uv workspace isn't possible because python-sdk is already a member of the code-interpreter/desktop workspaces). The packages start at0.0.0with aminorchangeset so the first release is0.1.0;pnpm -r postPublishis topological so the Python package publishes beforee2b. The Dockerfile→TemplateBuilderconverter stays in the SDKs.Plumbing added per package, mirroring the other sub-packages:
dockerfile_utils_{js,python}_tests.yml(unit-only, no sandboxes), path filters + jobs insdk_tests.yml, release checks inrelease.yml, itinerary labels,pkg_artifacts.ymlbuild/pack, and a "Build Dockerfile Utils JS" step before everyjs-sdkbuild (itstscneeds the built types).syntax: BOM, parser directives (# escape=\/# escape=`), case-insensitive keywords, comments (including inside continuations),\-continuations, builder flags (--chown=… --chmod=…), JSON/exec forms,ENV/ARG/LABELkey=value + legacy forms, heredocs (<<EOF,<<-EOF, quoted delimiters, multiple per instruction), line numbers and warnings.lexer(ShellLex): BuildKit's quote/escape-aware word splitting. Variables ($X,${X:-y}, …) are preserved, not expanded, so the sandbox/backend still evaluates them.dockerignore(PatternMatcher): the.dockerignorematcher (moby/patternmatcher port) moved here from the SDKs'template/dockerignoremodules. It has nonode:pathdependency (Gofilepath.Cleanreimplemented on slash paths), takes{ backslashIsSeparator }/backslash_is_separatorinstead of reading the platform separator, and raises a plainError/ValueError;getAllFilesInPathin the SDKs wraps that intoTemplateError/TemplateExceptionas before.dockerfileParser.ts/dockerfile_parser.pyare now a thin converter from the AST to the existingTemplateBuildercalls; public API and the single-stage / defaultUSER/WORKDIRsemantics are unchanged.Both ports were differential-tested against the real BuildKit Go parser (
moby/buildkit@v0.26.1) on BuildKit's parser testfiles plus a custom corpus (118 files) and a 202-case lexer corpus. Remaining diffs are intentional: unknown instructions are rejected at parse time (BuildKit rejects them one stage later),ONBUILDargs are dropped since the converter ignores them, and${VAR//a/b}keeps its//(BuildKit'sSkipUnsetEnvreproduction loses one).Bugs from the issue/PR history this fixes, with the same behavior in JS and Python:
ENV/ARGquoted values and values with spaces (fix(js-sdk): strip quotes from ENV values in fromDockerfile() #1176, fix(js-sdk): parse Dockerfile ENV/ARG values with spaces #1789):ENV A="hello world" B='x y'→setEnvs({A: 'hello world', B: 'x y'}).FROM img As name/ASin any case (fix(python-sdk): strip uppercase/mixed-case AS stage alias in from_dockerfile #1788).RUN/CMDquoted args is preserved; exec-formRUN ["echo", "a b"]is shell-quoted (echo 'a b') instead of space-joined.COPY/ADDshell form, andCOPY --chmod=755→copy(src, dest, { mode: 0o755 }).ENTRYPOINT+CMDcombined per Docker semantics (exec+exec → concatenated; execENTRYPOINT+ shellCMD→entrypoint /bin/sh -c '…'; shellENTRYPOINTignoresCMD).RUN <<EOFbecomes the heredoc body as the command;RUN cmd <<EOFkeeps the shell heredoc;COPY <<EOF /pathbecomesmkdir -p … && cat <<'E2B_HEREDOC_EOF' >/path …run as root (like Docker'sCOPY, independent ofUSER), with a delimiter that never occurs in the content; heredoc markers are detected on raw tokens, soCOPY "<<EOF" /tmp/is a literal file name.TemplateBuilder.copy/makeDirkeepmode: 0(--chmod=000) instead of dropping it; bareCMD/ENTRYPOINTare no-ops in both SDKs.# escape=directive, comments inside continuations,--platform/--mountflags warned and ignored instead of breaking the parse.Not included:
ARGsubstitution intoFROM(#1624) —ARGbeforeFROMis accepted but not substituted, as before.pkg_artifacts.yml: dockerfile-utils is versioned/packed before the SDKs, and the preview SDKs pin the exact preview dockerfile-utils version (workspace:^→^0.0.1-<branch>.0;uv add --frozen "e2b-dockerfile-utils==<version>"), so both preview artifacts install together.Lockfiles regenerated (
pnpm-lock.yaml,uv.lockin dockerfile-utils-python, python-sdk, code-interpreter-python, desktop-python with uv 0.10.0).Upstream test suites
The packages carry ports of the upstream test suites (
tests/fixtures/buildkit/README.mddocuments provenance/license), identical cases in JS (vitest) and Python (pytest):moby/patternmatcher@v0.6.1patternmatcher_test.go:TestMatches(with the Windows table viabackslashIsSeparator),TestMatchesWithMalformedPatterns,TestMultiplePatterns,TestMatch(Gofilepath.Matchtable),TestCleanPatterns*,TestMatchesWithNoPatterns,TestMatchesWithParentPatternfamily and theMatchesOrParentMatches/MatchesUsingParentResultstables adapted tomatches()/mayMatchUnder(). To pass them,PatternMatchernow rejects whatfilepath.Matchrejects ([,[^,[]a],[-],a-b-c], trailing\), rejects a lone!, exposes the cleanedpatternsand accepts backslash candidate paths whenbackslashIsSeparatoris set.moby/buildkit@v0.26.1frontend/dockerfile/parser: alltestfiles/*(Dockerfile+result, compared through aNode.Dump()-style normalizer of the flat AST),testfiles-negative/*,testfile-lineline numbers,TestParseWords,TestParseJSON,TestParseNameVal/TestParseNameValOldFormat,TestParseDirectives,TestHeredocsFromLine,TestParseHeredoc*,TestChompHeredocContent.HEALTHCHECKis now parsed (CMD/NONE+ command) instead of ignored so thehealthfixture matches. One fixture is skipped on purpose:escapes, because BuildKit splits shell-formADD/COPYon whitespace only, while this parser honors quotes (COPY "a b" /dstworks) which changes the parse of its unbalancedADD \conf\\" /.zncline.frontend/dockerfile/shelllex_test.goword tables (wordsTest,wordsTestwithRawQuotes,envVarTest), as JSON fixtures generated with the Go lexer (SkipUnsetEnv, empty env).ProcessWordWithMatches/EnvsFromSlice/TestShellParser4EnvVars-style tests that need an environment map are not applicable sinceShellLexpreserves variables instead of expanding them. 36envVarTestentries keep the upstreamProcessWordsoutput asbuildkitWordsfor reference: upstream drops the text before a${VAR<modifier>…}(he${XXX:-000}xx→${XXX:-000}xx, a words-buffer reset inprocessStopOn), ours returns the intact word.Linear
Fixes SDK-98, SDK-101, SDK-100. Related: SDK-233 (parser polish), SDK-97 (
ARGsubstitution inFROM, intentionally not addressed here).Link to Devin session: https://app.devin.ai/sessions/846d5c3ece6147a1ac83e90f3b97cbfe
Open in Devin Desktop: https://app.devin.ai/desktop/session/846d5c3ece6147a1ac83e90f3b97cbfe?variant=devin
Requested by: @mishushakov