Skip to content

Correctness overhaul: semantic guard, AST-based RUN formatting, and fixes for #69–#81 - #83

Merged
reteps merged 19 commits into
mainfrom
fix/shell-rewrite
Nov 7, 2026
Merged

reteps merged 19 commits into
mainfrom
fix/shell-rewrite

Conversation

@reteps

@reteps reteps commented Nov 7, 2026

Copy link
Copy Markdown
Owner

Summary

dockerfmt could turn a working Dockerfile into one that no longer builds (or builds something different). This PR fixes every open issue of that kind and changes the design so the class of bug can't silently recur.

1. Semantic guard (lib/semantic.go)

After formatting each instruction, the output is parsed again with BuildKit and compared with BuildKit's reading of the original. The comparison covers:

  • ENV/LABEL/ARG values as BuildKit expands them
  • COPY/ADD sources and destinations
  • flags and exec arrays
  • heredoc contents
  • shell commands, compared in a whitespace-insensitive canonical form

If anything differs, or the formatter panics, the instruction is emitted exactly as written and a warning goes to stderr (Dockerfile:12: left unformatted: ...). The fixture test fails on any fallback, so the guard can't hide regressions.

Cost: the wasm grows from 7.35 to 7.74 MB, or from 2.01 to 2.12 MB gzipped.

2. RUN/CMD shell pipeline rewritten without regexes (lib/shell.go)

  • Old pipeline: comments were disguised as backtick command substitutions for shfmt, regexes moved && around them, more regexes undid it, and two regexes decided when to give up.
  • New pipeline: each physical line is classified as code, comment, blank, or \-only. Only the code goes to mvdan/sh. Comments and separators are put back by walking the source and output syntax trees in parallel, so every line is anchored to the token after it.
  • Placement: a comment the user indented past the next command stays with the previous one (e.g. under an argument list).
  • Unchanged output: every pre-existing fixture formats exactly as before.

3. Decisions taken from BuildKit's parse tree

JSON form, heredocs, SHELL and flags are no longer guessed by re-parsing text. The custom JSON decoder and the google/shlex dependency are gone.

Issues fixed

Other bugs found by the guard and fixed here

  • An instruction with no formatter, such as a misspelled keyword, was deleted. --write removed it from the file.
  • COPY/ADD with a heredoc dropped their flags, so COPY --chmod=755 <<EOF lost --chmod. Found in BuildKit's own Dockerfile.
  • CMD JSON form dropped flags, and COPY dropped a -- end-of-flags marker.
  • Three panics, and five outputs that no longer parsed, on BuildKit's parser test files.

Fixture changes

Test plan

  • go vet ./..., go test ./..., gofmt -l .
  • JS: npm run build && node --test dist/format.test.js, 12/12
  • Ran over 86 Dockerfiles: BuildKit's parser test files, BuildKit's own Dockerfiles, and all fixtures. Result: 0 fallbacks, 0 idempotency failures, and BuildKit reads every output the same as its input. The only differences are RUN heredoc scripts re-indented by shfmt, on purpose.
  • Every fixture converted to CRLF: consistent CRLF output, no fallbacks.

Supersedes #82, #74, #75.

🤖 Generated with Claude Code

RyoKusnadi and others added 19 commits November 6, 2026 00:22
Signed-off-by: Ryo Kusnadi <ryokusnadi@gmail.com>
… to LABEL

Signed-off-by: Ryo Kusnadi <ryokusnadi@gmail.com>
Lines holding only a backslash, used as visual separators inside long RUN
instructions, were dropped wherever mvdan/sh's printer drops them: before
a line starting with "&&" and at the start of the content (right after
the flags). Comments placed before or between RUN flags were dropped too,
because flags are rebuilt from the parsed n.Flags.

- Shell content: turn each empty continuation line into a marker comment
  before shfmt (the comment placeholders already survive every position)
  and back into a "\\" line afterwards, aligned with the next line. A
  "\\" line with no surrounding whitespace joins two words, so it is
  left alone.
- Flags region: scan the original source for comment and empty
  continuation lines among the flags and re-emit them, one item per line.
  Without such lines the existing flag layout is unchanged.
- Drop the redundant continuation the comment handling added after flags
  that already end in " \\", which produced a stray "\\" line.

tests/out/run5.dockerfile now keeps its separator line.

Fixes #59

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LABEL expands variables but MAINTAINER does not, so "MAINTAINER $USER"
became a label holding the value of $USER. Follow-up to #75.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
COPY/ADD in JSON form were always rewritten to the space-separated form,
and each JSON element was additionally shlex-split. BuildKit splits the
space-separated form on whitespace, so ["my file.txt", "/app dir/"]
became sources my, file.txt, /app and destination dir/. Keep the JSON
form whenever an argument contains whitespace or is empty.

Fixes #71

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… meaning

After formatting each instruction, parse the output with BuildKit and
compare it with BuildKit's reading of the original: ENV/LABEL/ARG values
as BuildKit expands them, COPY/ADD sources and destination, exec arrays,
heredoc bodies, flags, and shell-form commands in a whitespace-insensitive
canonical form (mvdan/sh). If they differ, or the formatter panics, emit
the instruction exactly as written and report it through
Config.OnFallback; the CLI prints a warning to stderr.

This turns formatter bugs into "left unformatted" instead of a Dockerfile
that no longer builds (the class of bug in #69, #70, #71, #76-#80). The
fixture test fails on any fallback that isn't expected, so the guard
can't hide regressions. label.dockerfile falls back on its quoted
multi-line ENV value until #78 is fixed.

Also: report shell parse errors and shlex errors as panics (recovered
by the guard) instead of printing to stdout or calling log.Fatalf.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A CRLF Dockerfile came out with mixed line endings: instructions rebuilt
from BuildKit's parsed values lost their "\r", while comments and lines
copied from the original text kept it.

The formatter now normalizes "\r\n" to "\n" before formatting and
restores the input's line ending afterwards, choosing CRLF when most
line breaks are "\r\n" (a tie goes to the first one). This lives in the
library, so the CLI and the JS bindings both get it. BuildKit keeps the
"\r" inside heredoc bodies, and converting back to CRLF puts it back, so
a consistently-CRLF file reads the same to BuildKit before and after
formatting. --check passes on an already-formatted CRLF file.

Fixes #73

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
BuildKit joins continuation lines by appending each line as written,
leading whitespace included. Outside quotes that whitespace only
separates words, but inside a quoted value it is part of the value, so
re-indenting

    ENV MSG="line one \
    line two"

changed MSG from "line one line two" to "line one     line two". The
semantic guard caught this and left such instructions unformatted.

ENV and the instructions formatted by formatBasic (ARG, LABEL, USER,
VOLUME, STOPSIGNAL, HEALTHCHECK) now re-indent through a quote-aware
scanner that follows BuildKit's word splitting: single and double
quotes, and the escape character outside quotes and inside double
quotes. Lines that start inside a quoted string keep their leading
whitespace exactly, as do lines that continue a word with no
whitespace at the join (A=foo\ + bar). Only whitespace BuildKit ignores
is trimmed: after a line continuation, and unescaped whitespace outside
quotes at the end of the instruction. Comment and blank lines inside a
continuation are still re-indented and emptied.

The escape character is the line-continuation character, so it is read
from the end of the instruction's first line, which handles a
"# escape=`" directive without new plumbing.

label.dockerfile no longer needs to be an expected fallback.

Fixes #78

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…einsertion

Shell-form RUN/CMD/ENTRYPOINT bodies used to go through shfmt with
comments disguised as backtick command substitutions, regexes moving
"&&" around them, and more regexes undoing it all afterwards, plus two
regexes deciding when to give up (unescaped semicolons, "{ \\").

The new pipeline (lib/shell.go):
- classifies each physical line: code, comment, blank, or empty
  continuation ("\\" alone). Only code goes to mvdan/sh, which is also
  what the shell actually runs, since BuildKit drops the others.
- prints the code with mvdan/sh, and leaves the body as written if the
  result is not one continued line (this replaces both bail-out regexes).
- re-inserts comments and separators by walking the source and output
  syntax trees in parallel, anchoring each line to the token after it.
  A group the user indented past the next command stays with the
  previous one (e.g. under an argument list); otherwise it takes the
  next line's indentation.

Decisions now come from BuildKit's parse tree instead of re-parsing text:
- JSON form is BuildKit's json attribute, with its decoded elements
  (the lenient custom JSON decoder accepted ["a",]). Fixes #79
- SHELL is tracked per stage; bodies for a non-POSIX shell (pwsh, cmd)
  are left as written. Fixes #72
- Only a heredoc the shell runs as a script ("RUN <<EOF", no non-sh
  shebang) is formatted; heredocs fed to another program are data.
  Fixes #80
- Every heredoc is emitted, not just the first. Fixes #76
- Blank lines inside a continuation are dropped instead of splitting
  the instruction. Fixes #77

Also fixes COPY/ADD with a heredoc dropping its flags
("COPY --chmod=755 <<EOF" lost --chmod), which the semantic guard
caught in BuildKit's own Dockerfile, and stops formatRun from writing
the formatted heredoc back into the parse tree (which would have hidden
heredoc changes from the guard).

Formatting is unchanged for every existing fixture. Over BuildKit's
parser test files, the three panics and five outputs that no longer
parsed are gone.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A UTF-16 Dockerfile (what Windows PowerShell's ">" writes by default)
parsed as a list of unknown instructions, so dockerfmt printed a single
newline and exited 0, and --write replaced the file with it.

Add lib.Format(content, config) (string, error). It rejects UTF-16 and
UTF-32 byte order marks, NUL bytes and invalid UTF-8 with an error that
names the line and says to re-save the file as UTF-8, and returns
BuildKit parse errors (now with a line number when BuildKit gives one)
instead of calling log.Fatalf. A UTF-8 byte order mark, which BuildKit
ignores, is set aside while formatting and kept in the output.
FormatFileLines keeps its signature and behavior, and shares the line
ending and BOM handling.

The CLI uses Format. Errors flow back from processInput to a new run()
that returns the exit status, so only Run calls os.Exit: an input that
can't be formatted is reported as "dockerfmt: <file>: <error>" on
stderr, nothing is printed or written for it, the remaining files are
still processed, and the exit status is 1. Unreadable files and write
errors are reported the same way instead of aborting.

The WASM binding returns a JS Error, which formatDockerfileContents
throws, so the promise rejects instead of the Go runtime exiting.
formatDockerfile decodes the file as strict UTF-8 rather than replacing
invalid bytes with U+FFFD, and keeps a byte order mark (wasm_exec.js
would otherwise drop it on the way back from Go).

Fixes #81

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An instruction with no formatter, such as a misspelled keyword (BuildKit's
parser accepts it and only the build rejects it), was silently dropped,
or only a fragment of a multi-line one survived. With --write that
deleted it from the file. Emit it exactly as written.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mat.wasm

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rker

CMD/ENTRYPOINT in JSON form dropped their flags, and COPY/ADD dropped a
"--" after the flags, which matters when a source starts with "--".
Both were caught by the semantic guard on BuildKit's flags test file.

JSON-form COPY/ADD arguments are now joined as decoded instead of being
shlex-split (BuildKit processes each argument the same way in both
forms), which removes the github.com/google/shlex dependency.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@reteps
reteps merged commit d096f32 into main Nov 7, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants