From 70a9c2f66bf0c1064d3d213e3281374a42618eea Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 09:57:17 +0000 Subject: [PATCH 1/5] test(utils): Add unit tests for version parsing and small helpers Part of Phase T2 in docs/testing-plan.md. Covers ParseVersionFileJson, ExtractVersion, HashString, GenerateDockerImageName and RemoveEmptyStringsFromArray, none of which had any coverage. These pin current behaviour rather than change it, including the edges the plan records as findings: ExtractVersion yields "v" for a version file of {} and "vv1.0.0" for {"version":"v1.0.0"}, and RemoveEmptyStringsFromArray drops "" but keeps " ". Phase T2 is still in progress; the remaining hashing and dockerignore tests follow in a later commit. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Tn4Aqbe88wWU6a5njxB8dA --- cli/utils/extract_version_test.go | 101 ++++++++++++++++++ cli/utils/generate_docker_image_name_test.go | 39 +++++++ cli/utils/hash_string_test.go | 37 +++++++ cli/utils/parse_version_file_json_test.go | 47 ++++++++ .../remove_empty_strings_from_array_test.go | 48 +++++++++ 5 files changed, 272 insertions(+) create mode 100644 cli/utils/extract_version_test.go create mode 100644 cli/utils/generate_docker_image_name_test.go create mode 100644 cli/utils/hash_string_test.go create mode 100644 cli/utils/parse_version_file_json_test.go create mode 100644 cli/utils/remove_empty_strings_from_array_test.go diff --git a/cli/utils/extract_version_test.go b/cli/utils/extract_version_test.go new file mode 100644 index 0000000..3667be8 --- /dev/null +++ b/cli/utils/extract_version_test.go @@ -0,0 +1,101 @@ +package utils + +import ( + "os" + "path/filepath" + "testing" +) + +// These are pure unit tests: they operate entirely inside t.TempDir() and +// never touch a registry or need any credentials. + +func writeVersionFile(t *testing.T, dir, name, content string) string { + t.Helper() + path := filepath.Join(dir, name) + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("failed to write %s: %v", path, err) + } + return path +} + +func TestExtractVersionValid(t *testing.T) { + dir := t.TempDir() + path := writeVersionFile(t, dir, "version.json", `{"version": "1.0.0"}`) + + got, err := ExtractVersion(path) + if err != nil { + t.Fatalf("ExtractVersion returned an unexpected error: %v", err) + } + if got != "v1.0.0" { + t.Fatalf("ExtractVersion() = %q, want %q", got, "v1.0.0") + } +} + +func TestExtractVersionMissingFile(t *testing.T) { + dir := t.TempDir() + _, err := ExtractVersion(filepath.Join(dir, "does-not-exist.json")) + if err == nil { + t.Fatal("ExtractVersion on a missing file returned a nil error, want an error") + } +} + +// TestExtractVersionUnreadableFile pins that a version file whose permissions +// deny read access surfaces an error rather than being silently skipped. This +// subtest is meaningless when the test process itself is root, since root +// ignores the file mode bits and the open succeeds anyway - in that case we +// skip rather than assert a false failure. +func TestExtractVersionUnreadableFile(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("running as root: chmod 000 does not block reads for root, so this edge cannot be exercised here") + } + + dir := t.TempDir() + path := writeVersionFile(t, dir, "version.json", `{"version": "1.0.0"}`) + + if err := os.Chmod(path, 0o000); err != nil { + t.Fatalf("failed to chmod version file: %v", err) + } + defer os.Chmod(path, 0o644) // restore so t.TempDir() cleanup can remove it + + _, err := ExtractVersion(path) + if err == nil { + t.Fatal("ExtractVersion on an unreadable file returned a nil error, want an error") + } +} + +// TestExtractVersionEmptyObjectYieldsVPrefix pins a currently-silent edge +// (see docs/testing-plan.md Phase T2.1): a version file that is valid JSON but +// has no `version` key at all does not error - it produces the tag-looking +// string "v". This is documented as a deliberate, known edge, not a fix +// target: ExtractVersion has no way to distinguish "key absent" from "key +// present but empty" once json.Unmarshal has run. +func TestExtractVersionEmptyObjectYieldsVPrefix(t *testing.T) { + dir := t.TempDir() + path := writeVersionFile(t, dir, "version.json", `{}`) + + got, err := ExtractVersion(path) + if err != nil { + t.Fatalf("ExtractVersion returned an unexpected error: %v", err) + } + if got != "v" { + t.Fatalf("ExtractVersion(\"{}\") = %q, want %q (pinned known edge)", got, "v") + } +} + +// TestExtractVersionDoubleVPrefix pins the other half of the same known edge: +// a version value that already carries a leading "v" (e.g. copied from a git +// tag) is not detected or stripped, so the returned tag doubles up as "vv...". +// Either behaviour is a plausible tag name and neither fails loudly today, so +// this test documents the current output rather than asserting it is correct. +func TestExtractVersionDoubleVPrefix(t *testing.T) { + dir := t.TempDir() + path := writeVersionFile(t, dir, "version.json", `{"version": "v1.0.0"}`) + + got, err := ExtractVersion(path) + if err != nil { + t.Fatalf("ExtractVersion returned an unexpected error: %v", err) + } + if got != "vv1.0.0" { + t.Fatalf("ExtractVersion(%q) = %q, want %q (pinned known edge)", `{"version": "v1.0.0"}`, got, "vv1.0.0") + } +} diff --git a/cli/utils/generate_docker_image_name_test.go b/cli/utils/generate_docker_image_name_test.go new file mode 100644 index 0000000..761fb0b --- /dev/null +++ b/cli/utils/generate_docker_image_name_test.go @@ -0,0 +1,39 @@ +package utils + +import "testing" + +// These are pure unit tests: no filesystem, no registry, no credentials. + +func TestGenerateDockerImageNameEmptyRegistryOmitsHost(t *testing.T) { + got := GenerateDockerImageName("", "myapp", "abc123") + want := "myapp:abc123" + if got != want { + t.Fatalf("GenerateDockerImageName(\"\", ...) = %q, want %q", got, want) + } +} + +func TestGenerateDockerImageNameSetRegistryIncludesHost(t *testing.T) { + got := GenerateDockerImageName("reg.example.com", "myapp", "abc123") + want := "reg.example.com/myapp:abc123" + if got != want { + t.Fatalf("GenerateDockerImageName(reg, ...) = %q, want %q", got, want) + } +} + +// TestGenerateDockerImageNameImageNameWithSlash pins that an image name that +// already contains an org/namespace slash (e.g. "org/myapp") is concatenated +// as-is - GenerateDockerImageName does no parsing or validation of the image +// name, it only decides whether to prepend the registry host. +func TestGenerateDockerImageNameImageNameWithSlash(t *testing.T) { + got := GenerateDockerImageName("reg.example.com", "org/myapp", "abc123") + want := "reg.example.com/org/myapp:abc123" + if got != want { + t.Fatalf("GenerateDockerImageName(reg, \"org/myapp\", ...) = %q, want %q", got, want) + } + + gotNoRegistry := GenerateDockerImageName("", "org/myapp", "abc123") + wantNoRegistry := "org/myapp:abc123" + if gotNoRegistry != wantNoRegistry { + t.Fatalf("GenerateDockerImageName(\"\", \"org/myapp\", ...) = %q, want %q", gotNoRegistry, wantNoRegistry) + } +} diff --git a/cli/utils/hash_string_test.go b/cli/utils/hash_string_test.go new file mode 100644 index 0000000..3907f31 --- /dev/null +++ b/cli/utils/hash_string_test.go @@ -0,0 +1,37 @@ +package utils + +import "testing" + +// These are pure unit tests: no filesystem, no registry, no credentials. + +// TestHashStringKnownVector pins HashString against an independently verified +// SHA256 digest (computed with `printf 'dockem' | sha256sum`), so a change to +// the hashing primitive itself - not just its callers - would be caught here. +func TestHashStringKnownVector(t *testing.T) { + const want = "1147bbc8f02eda032e2e169e6dd8b140884a6d7d04dcc4d6fe879842aa8868aa" + if got := HashString("dockem"); got != want { + t.Fatalf("HashString(\"dockem\") = %q, want %q", got, want) + } +} + +// TestHashStringEmptyInput pins the digest of the empty string (the standard +// SHA256 empty-input constant, verified with `printf '' | sha256sum`). +func TestHashStringEmptyInput(t *testing.T) { + const want = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" + if got := HashString(""); got != want { + t.Fatalf("HashString(\"\") = %q, want %q", got, want) + } +} + +// TestHashStringDeterministic guards the property overallHash relies on: the +// same input must always produce the same output across calls, since it is +// what makes the hash usable as a cache key at all. +func TestHashStringDeterministic(t *testing.T) { + const input = "some concatenated hash inputs" + first := HashString(input) + for i := 0; i < 5; i++ { + if got := HashString(input); got != first { + t.Fatalf("HashString(%q) = %q on call %d, want %q (same as first call)", input, got, i, first) + } + } +} diff --git a/cli/utils/parse_version_file_json_test.go b/cli/utils/parse_version_file_json_test.go new file mode 100644 index 0000000..f258908 --- /dev/null +++ b/cli/utils/parse_version_file_json_test.go @@ -0,0 +1,47 @@ +package utils + +import "testing" + +// These are pure unit tests: no filesystem, no registry, no credentials. + +func TestParseVersionFileJsonValid(t *testing.T) { + got, err := ParseVersionFileJson([]byte(`{"version": "1.2.3"}`)) + if err != nil { + t.Fatalf("ParseVersionFileJson returned an unexpected error: %v", err) + } + if got.Version != "1.2.3" { + t.Fatalf("ParseVersionFileJson().Version = %q, want %q", got.Version, "1.2.3") + } +} + +func TestParseVersionFileJsonMalformed(t *testing.T) { + _, err := ParseVersionFileJson([]byte(`{"version": "1.2.3"`)) + if err == nil { + t.Fatal("ParseVersionFileJson with malformed JSON returned a nil error, want an error") + } +} + +// TestParseVersionFileJsonNonStringVersion pins that a non-string `version` +// value (json.Unmarshal cannot coerce a number into the string field) is +// rejected as an error rather than silently stringified. +func TestParseVersionFileJsonNonStringVersion(t *testing.T) { + _, err := ParseVersionFileJson([]byte(`{"version": 123}`)) + if err == nil { + t.Fatal("ParseVersionFileJson with a numeric version value returned a nil error, want an error") + } +} + +// TestParseVersionFileJsonEmptyObject pins a currently-silent edge: a version +// file with no `version` key at all parses successfully and yields an empty +// Version string. This is a deliberate documentation of existing behaviour, +// not an endorsement of it - see ExtractVersion's "{} yields v" test for the +// user-visible consequence. +func TestParseVersionFileJsonEmptyObject(t *testing.T) { + got, err := ParseVersionFileJson([]byte(`{}`)) + if err != nil { + t.Fatalf("ParseVersionFileJson(\"{}\") returned an unexpected error: %v", err) + } + if got.Version != "" { + t.Fatalf("ParseVersionFileJson(\"{}\").Version = %q, want empty string", got.Version) + } +} diff --git a/cli/utils/remove_empty_strings_from_array_test.go b/cli/utils/remove_empty_strings_from_array_test.go new file mode 100644 index 0000000..c41ce83 --- /dev/null +++ b/cli/utils/remove_empty_strings_from_array_test.go @@ -0,0 +1,48 @@ +package utils + +import ( + "reflect" + "testing" +) + +// These are pure unit tests: no filesystem, no registry, no credentials. + +func TestRemoveEmptyStringsFromArrayOrderPreserved(t *testing.T) { + got := RemoveEmptyStringsFromArray([]string{"a", "", "b", "", "c"}) + want := []string{"a", "b", "c"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("RemoveEmptyStringsFromArray(...) = %#v, want %#v", got, want) + } +} + +// TestRemoveEmptyStringsFromArrayAllEmptyReturnsNil pins that an input made +// entirely of empty strings returns a true nil slice, not an empty non-nil +// one - the function declares its accumulator with `var newArray []string` +// and only ever appends, so it is never assigned an empty literal. A caller +// that checks `result == nil` to detect "no tags survived" depends on this. +func TestRemoveEmptyStringsFromArrayAllEmptyReturnsNil(t *testing.T) { + got := RemoveEmptyStringsFromArray([]string{"", "", ""}) + if got != nil { + t.Fatalf("RemoveEmptyStringsFromArray(all-empty) = %#v, want nil", got) + } +} + +func TestRemoveEmptyStringsFromArrayEmptyInputReturnsNil(t *testing.T) { + got := RemoveEmptyStringsFromArray([]string{}) + if got != nil { + t.Fatalf("RemoveEmptyStringsFromArray([]string{}) = %#v, want nil", got) + } +} + +// TestRemoveEmptyStringsFromArrayWhitespaceOnlyIsKept pins a currently-silent +// edge from docs/testing-plan.md Phase T2.5: the function only drops strings +// that are exactly "", so a whitespace-only value like " " (e.g. from +// `--tag " "`) is not empty by this check and survives into the result. This +// documents existing behaviour rather than endorsing it. +func TestRemoveEmptyStringsFromArrayWhitespaceOnlyIsKept(t *testing.T) { + got := RemoveEmptyStringsFromArray([]string{"a", " ", "b"}) + want := []string{"a", " ", "b"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("RemoveEmptyStringsFromArray with whitespace-only entry = %#v, want %#v (pinned known edge)", got, want) + } +} From 685ca67c9bdedcfb3bc577f087c4447c38eb1604 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 10:01:51 +0000 Subject: [PATCH 2/5] test(utils): Complete Phase T2 with hashing and dockerignore tests Covers the rest of Phase T2 in docs/testing-plan.md: HashWatchFiles, HashWatchDirectories, ReadDockerignore, DetectBuildx and parseBuildxVersion. Pins the contracts the plan calls load-bearing: HashWatchFiles and HashWatchDirectories both return "" for an empty list, so a user who never adopts the flags sees no change to cache identity, and DetectBuildx reports every failure mode as (false, "", nil) rather than an error, leaving ResolveBuilder to decide when that is fatal. ReadDockerignore's exclude patterns are asserted to land after the file's, so a negation passed via --exclude can re-include a file the .dockerignore excluded. Also pins the in-place sort in HashWatchDirectories, which mutates the caller's slice, as a documented finding rather than changing it here. DetectBuildx is driven through a fake docker shim on PATH, so it needs neither docker nor buildx to run. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Tn4Aqbe88wWU6a5njxB8dA --- cli/utils/detect_buildx_test.go | 113 ++++++++++++++++ cli/utils/hash_watch_directories_test.go | 158 +++++++++++++++++++++++ cli/utils/hash_watch_files_test.go | 90 +++++++++++++ cli/utils/read_dockerignore_test.go | 150 +++++++++++++++++++++ 4 files changed, 511 insertions(+) create mode 100644 cli/utils/detect_buildx_test.go create mode 100644 cli/utils/hash_watch_directories_test.go create mode 100644 cli/utils/hash_watch_files_test.go create mode 100644 cli/utils/read_dockerignore_test.go diff --git a/cli/utils/detect_buildx_test.go b/cli/utils/detect_buildx_test.go new file mode 100644 index 0000000..a57fa7f --- /dev/null +++ b/cli/utils/detect_buildx_test.go @@ -0,0 +1,113 @@ +package utils + +import ( + "fmt" + "os" + "path/filepath" + "strconv" + "strings" + "testing" +) + +// These are pure unit tests: no registry, no credentials. They do use +// t.Setenv("PATH", ...) to point at a fake `docker` shim, which is why none of +// them call t.Parallel() - Go panics if a test calling t.Setenv is parallel. + +// writeDockerShim writes an executable `docker` script into dir that ignores +// its arguments and reproduces the given exit code and combined stdout. +func writeDockerShim(t *testing.T, dir string, exitCode int, output string) { + t.Helper() + escaped := strings.ReplaceAll(output, `'`, `'\''`) + script := fmt.Sprintf("#!/bin/sh\nprintf '%s'\nexit %s\n", escaped, strconv.Itoa(exitCode)) + path := filepath.Join(dir, "docker") + if err := os.WriteFile(path, []byte(script), 0o755); err != nil { + t.Fatalf("failed to write fake docker shim: %v", err) + } +} + +// --- parseBuildxVersion (unexported, called directly - same package) --- + +func TestParseBuildxVersionNormalOutput(t *testing.T) { + got := parseBuildxVersion("github.com/docker/buildx v0.36.1 d9f8b8f0abcdef\n") + want := "v0.36.1" + if got != want { + t.Fatalf("parseBuildxVersion(normal output) = %q, want %q", got, want) + } +} + +// TestParseBuildxVersionNoVersionLikeToken pins the fallback: when nothing in +// the output looks like a version token, the trimmed output is returned as-is +// so the caller still has something to log. +func TestParseBuildxVersionNoVersionLikeToken(t *testing.T) { + input := " buildx plugin not found \n" + got := parseBuildxVersion(input) + want := "buildx plugin not found" + if got != want { + t.Fatalf("parseBuildxVersion(no version token) = %q, want %q", got, want) + } +} + +func TestParseBuildxVersionEmptyOutput(t *testing.T) { + got := parseBuildxVersion("") + if got != "" { + t.Fatalf("parseBuildxVersion(\"\") = %q, want \"\"", got) + } +} + +// TestParseBuildxVersionSuffixedVersion: a version like v0.36.1-desktop.1 +// still starts with 'v' followed by a digit, so it must be picked out whole, +// suffix included, rather than truncated at the hyphen. +func TestParseBuildxVersionSuffixedVersion(t *testing.T) { + got := parseBuildxVersion("github.com/docker/buildx v0.36.1-desktop.1 abcdef123456\n") + want := "v0.36.1-desktop.1" + if got != want { + t.Fatalf("parseBuildxVersion(suffixed version) = %q, want %q", got, want) + } +} + +// --- DetectBuildx --- + +func TestDetectBuildxSuccess(t *testing.T) { + dir := t.TempDir() + writeDockerShim(t, dir, 0, "github.com/docker/buildx v0.36.1 d9f8b8f0abcdef\n") + t.Setenv("PATH", dir) + + ok, version, err := DetectBuildx() + if err != nil { + t.Fatalf("DetectBuildx returned an unexpected error: %v", err) + } + if !ok { + t.Fatal("DetectBuildx returned ok=false for a shim that exits 0, want true") + } + if version != "v0.36.1" { + t.Fatalf("DetectBuildx version = %q, want %q", version, "v0.36.1") + } +} + +// TestDetectBuildxNonZeroExit pins the contract from docs/testing-plan.md +// Phase T2.4: every failure mode - including a docker binary that exists but +// exits non-zero (e.g. no buildx plugin installed) - returns exactly +// (false, "", nil). The error return is never used for this; that decision is +// ResolveBuilder's, not DetectBuildx's. +func TestDetectBuildxNonZeroExit(t *testing.T) { + dir := t.TempDir() + writeDockerShim(t, dir, 1, "docker: 'buildx' is not a docker command.\n") + t.Setenv("PATH", dir) + + ok, version, err := DetectBuildx() + if ok != false || version != "" || err != nil { + t.Fatalf("DetectBuildx(non-zero exit) = (%v, %q, %v), want (false, \"\", nil)", ok, version, err) + } +} + +// TestDetectBuildxAbsentFromPath pins the same (false, "", nil) contract when +// there is no `docker` executable on PATH at all. +func TestDetectBuildxAbsentFromPath(t *testing.T) { + dir := t.TempDir() // empty directory, no docker binary + t.Setenv("PATH", dir) + + ok, version, err := DetectBuildx() + if ok != false || version != "" || err != nil { + t.Fatalf("DetectBuildx(no docker on PATH) = (%v, %q, %v), want (false, \"\", nil)", ok, version, err) + } +} diff --git a/cli/utils/hash_watch_directories_test.go b/cli/utils/hash_watch_directories_test.go new file mode 100644 index 0000000..9a2f618 --- /dev/null +++ b/cli/utils/hash_watch_directories_test.go @@ -0,0 +1,158 @@ +package utils + +import ( + "os" + "path/filepath" + "testing" +) + +// These are pure unit tests: they operate entirely inside t.TempDir() and +// never touch a registry or need any credentials. + +func writeWatchDirFile(t *testing.T, dir, rel, content string) { + t.Helper() + full := filepath.Join(dir, filepath.FromSlash(rel)) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatalf("failed to create parent dir for %s: %v", rel, err) + } + if err := os.WriteFile(full, []byte(content), 0o644); err != nil { + t.Fatalf("failed to write %s: %v", rel, err) + } +} + +// TestHashWatchDirectoriesEmptyListReturnsEmptyString mirrors HashWatchFiles' +// "contributes nothing" contract: users who never adopt --watch-directory must +// see no effect on overallHash. +func TestHashWatchDirectoriesEmptyListReturnsEmptyString(t *testing.T) { + got, err := HashWatchDirectories(nil, nil) + if err != nil { + t.Fatalf("HashWatchDirectories(nil, nil) returned an unexpected error: %v", err) + } + if got != "" { + t.Fatalf("HashWatchDirectories(nil, nil) = %q, want exactly \"\"", got) + } + + got, err = HashWatchDirectories([]string{}, nil) + if err != nil { + t.Fatalf("HashWatchDirectories([]string{}, nil) returned an unexpected error: %v", err) + } + if got != "" { + t.Fatalf("HashWatchDirectories([]string{}, nil) = %q, want exactly \"\"", got) + } +} + +// TestHashWatchDirectoriesSortInvariance: the function sorts the slice before +// hashing, so the caller's original ordering must not affect the result. +func TestHashWatchDirectoriesSortInvariance(t *testing.T) { + dirA := t.TempDir() + dirB := t.TempDir() + writeWatchDirFile(t, dirA, "a.txt", "alpha") + writeWatchDirFile(t, dirB, "b.txt", "beta") + + forward, err := HashWatchDirectories([]string{dirA, dirB}, nil) + if err != nil { + t.Fatalf("HashWatchDirectories(forward order) returned an unexpected error: %v", err) + } + reverse, err := HashWatchDirectories([]string{dirB, dirA}, nil) + if err != nil { + t.Fatalf("HashWatchDirectories(reverse order) returned an unexpected error: %v", err) + } + + if forward != reverse { + t.Fatalf("HashWatchDirectories is order-dependent: forward=%q reverse=%q", forward, reverse) + } +} + +// TestHashWatchDirectoriesMultipleDirsConcatenate: the combined hash of two +// directories must differ from the hash of either directory alone - proving +// the per-directory hashes are actually concatenated rather than, say, the +// last one winning. +func TestHashWatchDirectoriesMultipleDirsConcatenate(t *testing.T) { + dirA := t.TempDir() + dirB := t.TempDir() + writeWatchDirFile(t, dirA, "a.txt", "alpha") + writeWatchDirFile(t, dirB, "b.txt", "beta") + + both, err := HashWatchDirectories([]string{dirA, dirB}, nil) + if err != nil { + t.Fatalf("HashWatchDirectories(both) returned an unexpected error: %v", err) + } + onlyA, err := HashWatchDirectories([]string{dirA}, nil) + if err != nil { + t.Fatalf("HashWatchDirectories(onlyA) returned an unexpected error: %v", err) + } + onlyB, err := HashWatchDirectories([]string{dirB}, nil) + if err != nil { + t.Fatalf("HashWatchDirectories(onlyB) returned an unexpected error: %v", err) + } + + if both == onlyA || both == onlyB { + t.Fatalf("combined hash %q collided with a single-directory hash (onlyA=%q onlyB=%q)", both, onlyA, onlyB) + } +} + +func TestHashWatchDirectoriesMissingDirectoryErrors(t *testing.T) { + dir := t.TempDir() + _, err := HashWatchDirectories([]string{filepath.Join(dir, "does-not-exist")}, nil) + if err == nil { + t.Fatal("HashWatchDirectories with a missing directory returned a nil error, want an error") + } +} + +// TestHashWatchDirectoriesExcludePatternsMatchHashDirectory: HashWatchDirectories +// must apply excludePatterns the same way HashDirectory does directly, since +// the doc comment promises watch directories "behave consistently with" the +// build directory. +func TestHashWatchDirectoriesExcludePatternsMatchHashDirectory(t *testing.T) { + dir := t.TempDir() + writeWatchDirFile(t, dir, "app.go", "package app") + + patterns := []string{"*.log"} + want, err := HashDirectory(dir, patterns) + if err != nil { + t.Fatalf("HashDirectory returned an unexpected error: %v", err) + } + got, err := HashWatchDirectories([]string{dir}, patterns) + if err != nil { + t.Fatalf("HashWatchDirectories returned an unexpected error: %v", err) + } + if got != want { + t.Fatalf("HashWatchDirectories(single dir) = %q, want identical to HashDirectory = %q", got, want) + } + + // An excluded file must not move HashWatchDirectories' result either. + writeWatchDirFile(t, dir, "debug.log", "noise") + after, err := HashWatchDirectories([]string{dir}, patterns) + if err != nil { + t.Fatalf("HashWatchDirectories returned an unexpected error: %v", err) + } + if after != got { + t.Fatalf("adding an excluded file changed HashWatchDirectories' result: before=%q after=%q", got, after) + } +} + +// TestHashWatchDirectoriesMutatesCallerSliceInPlace pins a known finding from +// docs/testing-plan.md Phase T2.2: HashWatchDirectories calls sort.Strings +// directly on the slice it is given, so the caller's own slice - here, +// standing in for BuildDockerImageParams.WatchDirectory - comes back sorted +// even though the function only returns a hash string. This is documented as +// existing behaviour, not fixed, per the task's constraint against changing +// production behaviour. +func TestHashWatchDirectoriesMutatesCallerSliceInPlace(t *testing.T) { + // Use literal path-like strings, not t.TempDir()'s unpredictable naming, + // so the "before" order is known and the mutation is unambiguous. + watchDirs := []string{"/z-should-end-up-second", "/a-should-end-up-first"} + original := append([]string(nil), watchDirs...) + + // HashWatchDirectories will error because these paths don't exist, but + // sort.Strings runs on the caller's slice before HashDirectory is ever + // called, so the mutation happens regardless of the error. + _, _ = HashWatchDirectories(watchDirs, nil) + + if watchDirs[0] != "/a-should-end-up-first" || watchDirs[1] != "/z-should-end-up-second" { + t.Fatalf("expected sort.Strings to have reordered the caller's slice, got %v", watchDirs) + } + if watchDirs[0] == original[0] { + t.Fatal("expected the caller's slice to have been reordered, but it matches the original order") + } +} diff --git a/cli/utils/hash_watch_files_test.go b/cli/utils/hash_watch_files_test.go new file mode 100644 index 0000000..6911fed --- /dev/null +++ b/cli/utils/hash_watch_files_test.go @@ -0,0 +1,90 @@ +package utils + +import ( + "os" + "path/filepath" + "testing" +) + +// These are pure unit tests: they operate entirely inside t.TempDir() and +// never touch a registry or need any credentials. + +func writeWatchFile(t *testing.T, dir, name, content string) string { + t.Helper() + path := filepath.Join(dir, name) + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("failed to write %s: %v", path, err) + } + return path +} + +// TestHashWatchFilesEmptyListReturnsEmptyString is load-bearing for cache +// identity: users who never adopt --watch-file must see the exact same +// "contributes nothing" behaviour as hashPlatforms does for --platform, so +// that overallHash is unaffected by a feature they never opted into. +func TestHashWatchFilesEmptyListReturnsEmptyString(t *testing.T) { + got, err := HashWatchFiles(nil) + if err != nil { + t.Fatalf("HashWatchFiles(nil) returned an unexpected error: %v", err) + } + if got != "" { + t.Fatalf("HashWatchFiles(nil) = %q, want exactly \"\"", got) + } + + got, err = HashWatchFiles([]string{}) + if err != nil { + t.Fatalf("HashWatchFiles([]string{}) returned an unexpected error: %v", err) + } + if got != "" { + t.Fatalf("HashWatchFiles([]string{}) = %q, want exactly \"\"", got) + } +} + +// TestHashWatchFilesOrderInvariance: dirhash.Hash1 sorts internally, so the +// order the caller lists the files in must not affect the resulting hash. +func TestHashWatchFilesOrderInvariance(t *testing.T) { + dir := t.TempDir() + a := writeWatchFile(t, dir, "a.txt", "alpha") + b := writeWatchFile(t, dir, "b.txt", "beta") + + forward, err := HashWatchFiles([]string{a, b}) + if err != nil { + t.Fatalf("HashWatchFiles(forward order) returned an unexpected error: %v", err) + } + reverse, err := HashWatchFiles([]string{b, a}) + if err != nil { + t.Fatalf("HashWatchFiles(reverse order) returned an unexpected error: %v", err) + } + + if forward != reverse { + t.Fatalf("HashWatchFiles is order-dependent: forward=%q reverse=%q", forward, reverse) + } +} + +func TestHashWatchFilesMissingFileErrors(t *testing.T) { + dir := t.TempDir() + _, err := HashWatchFiles([]string{filepath.Join(dir, "does-not-exist.txt")}) + if err == nil { + t.Fatal("HashWatchFiles with a missing file returned a nil error, want an error") + } +} + +func TestHashWatchFilesContentChangeChangesHash(t *testing.T) { + dir := t.TempDir() + path := writeWatchFile(t, dir, "watched.txt", "version one") + + before, err := HashWatchFiles([]string{path}) + if err != nil { + t.Fatalf("HashWatchFiles returned an unexpected error: %v", err) + } + + writeWatchFile(t, dir, "watched.txt", "version two") + after, err := HashWatchFiles([]string{path}) + if err != nil { + t.Fatalf("HashWatchFiles returned an unexpected error: %v", err) + } + + if before == after { + t.Fatalf("changing the content of a watched file did not change the hash: %q", before) + } +} diff --git a/cli/utils/read_dockerignore_test.go b/cli/utils/read_dockerignore_test.go new file mode 100644 index 0000000..5556fdc --- /dev/null +++ b/cli/utils/read_dockerignore_test.go @@ -0,0 +1,150 @@ +package utils + +import ( + "os" + "path/filepath" + "reflect" + "testing" +) + +// These are pure unit tests: they operate entirely inside t.TempDir() and +// never touch a registry or need any credentials. + +func writeDockerignore(t *testing.T, path, content string) { + t.Helper() + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("failed to write %s: %v", path, err) + } +} + +// TestReadDockerignoreMissingFileIsNotAnError: with no .dockerignore present +// and no --exclude patterns, the result is an empty (non-nil-shaped but empty) +// slice and no error - the doc comment promises this yields a hash identical +// to pre-.dockerignore behaviour. +func TestReadDockerignoreMissingFileIsNotAnError(t *testing.T) { + dir := t.TempDir() + + got, err := ReadDockerignore(dir, "", nil) + if err != nil { + t.Fatalf("ReadDockerignore with no .dockerignore present returned an unexpected error: %v", err) + } + if len(got) != 0 { + t.Fatalf("ReadDockerignore with no .dockerignore present = %#v, want an empty slice", got) + } +} + +// TestReadDockerignoreMissingFileContributesOnlyExcludePatterns: still no +// error when the file is missing, but --exclude patterns must still come +// through. +func TestReadDockerignoreMissingFileContributesOnlyExcludePatterns(t *testing.T) { + dir := t.TempDir() + + got, err := ReadDockerignore(dir, "", []string{"*.log", "tmp/"}) + if err != nil { + t.Fatalf("ReadDockerignore returned an unexpected error: %v", err) + } + want := []string{"*.log", "tmp/"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("ReadDockerignore with missing file = %#v, want %#v", got, want) + } +} + +// TestReadDockerignoreCommentsAndBlankLinesStripped relies on +// ignorefile.ReadAll to strip "#" comments and blank lines - this pins that +// ReadDockerignore passes that behaviour through untouched. +func TestReadDockerignoreCommentsAndBlankLinesStripped(t *testing.T) { + dir := t.TempDir() + writeDockerignore(t, filepath.Join(dir, ".dockerignore"), "# a comment\n\nnode_modules\n\n# another comment\n*.log\n") + + got, err := ReadDockerignore(dir, "", nil) + if err != nil { + t.Fatalf("ReadDockerignore returned an unexpected error: %v", err) + } + want := []string{"node_modules", "*.log"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("ReadDockerignore = %#v, want %#v", got, want) + } +} + +// TestReadDockerignoreIgnoreFileOverridesDefaultPath: when ignoreFile is +// supplied, it is used verbatim instead of /.dockerignore - even +// if a real .dockerignore also exists in contextDir. +func TestReadDockerignoreIgnoreFileOverridesDefaultPath(t *testing.T) { + dir := t.TempDir() + writeDockerignore(t, filepath.Join(dir, ".dockerignore"), "should-not-be-used\n") + + overridePath := filepath.Join(dir, "custom.ignore") + writeDockerignore(t, overridePath, "should-be-used\n") + + got, err := ReadDockerignore(dir, overridePath, nil) + if err != nil { + t.Fatalf("ReadDockerignore returned an unexpected error: %v", err) + } + want := []string{"should-be-used"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("ReadDockerignore with ignoreFile override = %#v, want %#v", got, want) + } +} + +// TestReadDockerignoreUnreadableFileErrors pins that a present-but-unreadable +// ignore file surfaces an error rather than being treated like a missing one. +// Meaningless when the test process is root (root ignores mode bits), so we +// skip in that case rather than assert a false failure. +func TestReadDockerignoreUnreadableFileErrors(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("running as root: chmod 000 does not block reads for root, so this edge cannot be exercised here") + } + + dir := t.TempDir() + path := filepath.Join(dir, ".dockerignore") + writeDockerignore(t, path, "node_modules\n") + + if err := os.Chmod(path, 0o000); err != nil { + t.Fatalf("failed to chmod ignore file: %v", err) + } + defer os.Chmod(path, 0o644) // restore so t.TempDir() cleanup can remove it + + _, err := ReadDockerignore(dir, "", nil) + if err == nil { + t.Fatal("ReadDockerignore on an unreadable file returned a nil error, want an error") + } +} + +// TestReadDockerignoreExcludeAppearsAfterFilePatterns pins a real behavioural +// contract that is otherwise only implied by the code: --exclude patterns are +// appended AFTER the file's own patterns. That ordering is what lets a +// trailing "!keep-me" passed via --exclude re-include a path the +// .dockerignore file excluded - if the order were reversed, the file's later +// blanket pattern would win instead and the negation would have no effect. +func TestReadDockerignoreExcludeAppearsAfterFilePatterns(t *testing.T) { + dir := t.TempDir() + writeDockerignore(t, filepath.Join(dir, ".dockerignore"), "*.txt\n") + + got, err := ReadDockerignore(dir, "", []string{"!keep-me.txt"}) + if err != nil { + t.Fatalf("ReadDockerignore returned an unexpected error: %v", err) + } + want := []string{"*.txt", "!keep-me.txt"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("ReadDockerignore = %#v, want %#v (exclude patterns must be appended after file patterns)", got, want) + } + + // Prove the ordering actually matters for HashDirectory, not just for the + // returned slice's shape: with the file's "*.txt" followed by the + // negation, keep-me.txt must survive into the hash while drop-me.txt does + // not. + writeDockerignore(t, filepath.Join(dir, "keep-me.txt"), "keep") + writeDockerignore(t, filepath.Join(dir, "drop-me.txt"), "drop") + + base := hashOrFatal(t, dir, got) + + writeDockerignore(t, filepath.Join(dir, "drop-me.txt"), "drop, changed") + if after := hashOrFatal(t, dir, got); after != base { + t.Fatalf("editing drop-me.txt (still excluded by *.txt) changed the hash: base=%q after=%q", base, after) + } + + writeDockerignore(t, filepath.Join(dir, "keep-me.txt"), "keep, changed") + if after := hashOrFatal(t, dir, got); after == base { + t.Fatalf("editing keep-me.txt (re-included by !keep-me.txt) did not change the hash: %q", after) + } +} From 57fc0f0530e99f5f3802fca230a5d2598ea2954a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 10:01:52 +0000 Subject: [PATCH 3/5] test(buildx): Cover the BuildImageBuildx subprocess contract Phase T3 of docs/testing-plan.md. BuildImageBuildx was at zero coverage despite carrying every rule in the "Subprocess credentials (buildx)" section of CLAUDE.md. Drives the function against a fake docker shim on PATH that records its argv, environment and working directory. Asserts that DOCKER_CONFIG is set on the subprocess environment only and never on dockem's own, that a pre-existing DOCKER_CONFIG is stripped rather than duplicated, that every other parent environment variable is passed through (which is what makes --secret id=x,env=VAR work), that cmd.Dir is never set, that the throwaway config directory is removed even when the build exits non-zero, and that subprocess output lands on stderr so stdout stays clean for --output-format=json. The password is checked against argv, BuildLog and BuildResult with a sentinel value, so a future change that leaks it into any of the three fails here. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Tn4Aqbe88wWU6a5njxB8dA --- .../build_image_buildx_subprocess_test.go | 580 ++++++++++++++++++ 1 file changed, 580 insertions(+) create mode 100644 cli/utils/build_image_buildx_subprocess_test.go diff --git a/cli/utils/build_image_buildx_subprocess_test.go b/cli/utils/build_image_buildx_subprocess_test.go new file mode 100644 index 0000000..e2af0a2 --- /dev/null +++ b/cli/utils/build_image_buildx_subprocess_test.go @@ -0,0 +1,580 @@ +package utils + +import ( + "fmt" + "io" + "os" + "path/filepath" + "runtime" + "strconv" + "strings" + "testing" +) + +// These tests exercise BuildImageBuildx itself, rather than the pure +// assembleBuildxArgs helper covered in build_image_buildx_test.go. They pin +// every rule in the "Subprocess credentials (buildx)" section of CLAUDE.md by +// putting a fake `docker` first on PATH: a POSIX shell script that records its +// full argv, its complete environment and its working directory to files, then +// exits with a chosen status. None of this touches a real docker daemon, +// buildx installation or registry. +// +// None of these tests use t.Parallel(): every one of them manipulates +// process-global state (PATH, the parent's real environment via t.Setenv, +// os.Stdout/os.Stderr) that a concurrently running test could observe or +// stomp on. + +// fakeDockerRecording is where one invocation of the fake docker script wrote +// what it saw. +type fakeDockerRecording struct { + dir string +} + +func (r fakeDockerRecording) argv(t *testing.T) []string { + t.Helper() + data, err := os.ReadFile(filepath.Join(r.dir, "argv.txt")) + if os.IsNotExist(err) { + return nil + } + if err != nil { + t.Fatalf("could not read the fake docker's recorded argv: %v", err) + } + trimmed := strings.TrimSuffix(string(data), "\n") + if trimmed == "" { + return []string{} + } + return strings.Split(trimmed, "\n") +} + +func (r fakeDockerRecording) env(t *testing.T) []string { + t.Helper() + data, err := os.ReadFile(filepath.Join(r.dir, "env.txt")) + if err != nil { + t.Fatalf("could not read the fake docker's recorded environment: %v", err) + } + trimmed := strings.TrimSuffix(string(data), "\n") + if trimmed == "" { + return []string{} + } + return strings.Split(trimmed, "\n") +} + +func (r fakeDockerRecording) envValue(t *testing.T, key string) (string, bool) { + t.Helper() + prefix := key + "=" + for _, entry := range r.env(t) { + if strings.HasPrefix(entry, prefix) { + return strings.TrimPrefix(entry, prefix), true + } + } + return "", false +} + +func (r fakeDockerRecording) envCount(t *testing.T, key string) int { + t.Helper() + prefix := key + "=" + count := 0 + for _, entry := range r.env(t) { + if strings.HasPrefix(entry, prefix) { + count++ + } + } + return count +} + +func (r fakeDockerRecording) cwd(t *testing.T) string { + t.Helper() + data, err := os.ReadFile(filepath.Join(r.dir, "cwd.txt")) + if err != nil { + t.Fatalf("could not read the fake docker's recorded cwd: %v", err) + } + return strings.TrimSuffix(string(data), "\n") +} + +// installFakeDocker puts a fake `docker` executable first on PATH (ahead of +// whatever real PATH the test process already has, so other POSIX tools the +// script itself needs - env, pwd, printf - are still resolvable). The script +// records its full argv, environment and cwd into a fresh directory, echoes a +// distinctive line to its own stdout and its own stderr (for the stdout/stderr +// plumbing test), then exits with exitCode. +// +// Returns the fakeDockerRecording that will hold whatever the NEXT invocation +// writes. A fresh recording directory is used per install so consecutive +// subtests never read stale output from a previous run. +func installFakeDocker(t *testing.T, exitCode int) fakeDockerRecording { + t.Helper() + + if runtime.GOOS == "windows" { + t.Skip("the fake docker executable is a POSIX shell script") + } + + binDir := t.TempDir() + recordDir := t.TempDir() + + argvFile := filepath.Join(recordDir, "argv.txt") + envFile := filepath.Join(recordDir, "env.txt") + cwdFile := filepath.Join(recordDir, "cwd.txt") + + script := "#!/bin/sh\n" + + "rm -f \"" + argvFile + "\"\n" + + "touch \"" + argvFile + "\"\n" + + "for a in \"$@\"; do\n" + + " printf '%s\\n' \"$a\" >> \"" + argvFile + "\"\n" + + "done\n" + + "env > \"" + envFile + "\"\n" + + "pwd > \"" + cwdFile + "\"\n" + + "echo fake-docker-stdout-chatter\n" + + "echo fake-docker-stderr-chatter 1>&2\n" + + "exit " + strconv.Itoa(exitCode) + "\n" + + dockerPath := filepath.Join(binDir, "docker") + if err := os.WriteFile(dockerPath, []byte(script), 0o755); err != nil { + t.Fatalf("could not write the fake docker script: %v", err) + } + + origPath := os.Getenv("PATH") + t.Setenv("PATH", binDir+string(os.PathListSeparator)+origPath) + + return fakeDockerRecording{dir: recordDir} +} + +// runCapturingStreams swaps os.Stdout and os.Stderr for pipes for the duration +// of run, then returns everything written to each. Every LogInfo/LogWarn/ +// LogError call inside BuildImageBuildx writes to os.Stderr at call time (see +// log.go), so this is also how TestBuildImageBuildxStreamsSubprocessOutputToStderr +// distinguishes dockem's own chatter (which is supposed to land on stderr +// anyway) from anything that leaked onto stdout. +func runCapturingStreams(t *testing.T, run func() error) (stdout string, stderr string, runErr error) { + t.Helper() + + origStdout, origStderr := os.Stdout, os.Stderr + rOut, wOut, err := os.Pipe() + if err != nil { + t.Fatalf("could not create a stdout pipe: %v", err) + } + rErr, wErr, err := os.Pipe() + if err != nil { + t.Fatalf("could not create a stderr pipe: %v", err) + } + + os.Stdout = wOut + os.Stderr = wErr + defer func() { + os.Stdout = origStdout + os.Stderr = origStderr + }() + + runErr = run() + + wOut.Close() + wErr.Close() + + outBytes, _ := io.ReadAll(rOut) + errBytes, _ := io.ReadAll(rErr) + rOut.Close() + rErr.Close() + + return string(outBytes), string(errBytes), runErr +} + +// basicBuildxParams returns a minimal, valid set of params/tags/hash for +// exercising BuildImageBuildx without caring about the argument-assembly +// details already covered by build_image_buildx_test.go. +func basicBuildxParams(t *testing.T) (BuildDockerImageParams, string, []ResolvedTag) { + t.Helper() + contextDir := t.TempDir() + params := BuildDockerImageParams{ + DockerfilePath: filepath.Join(contextDir, "Dockerfile"), + Directory: contextDir, + ImageName: "example/repo", + } + targetTags := []ResolvedTag{ + {ImageName: "example/repo:v1.2.3", Reason: TagReasonMainVersion}, + } + return params, "deadbeef", targetTags +} + +// TestBuildImageBuildxArgvMatchesAssembleBuildxArgs pins that the argv the +// child actually receives is exactly what assembleBuildxArgs computes for the +// same inputs - ie. that BuildImageBuildx does not add, drop or reorder +// anything between assembling the arguments and executing them. +func TestBuildImageBuildxArgvMatchesAssembleBuildxArgs(t *testing.T) { + recording := installFakeDocker(t, 0) + + params, imageHash, targetTags := basicBuildxParams(t) + params.Platform = []string{"linux/amd64", "linux/arm64"} + params.CacheFrom = []string{"type=gha"} + params.CacheTo = []string{"type=gha,mode=max"} + + expectedLog := &BuildLog{} + hashImageName := GenerateDockerImageName(params.Registry, params.ImageName, imageHash) + var wantArgs []string + runCapturingStreams(t, func() error { + wantArgs = assembleBuildxArgs(params, hashImageName, targetTags, expectedLog) + return nil + }) + + actualLog := &BuildLog{} + _, _, err := runCapturingStreams(t, func() error { + return BuildImageBuildx(params, imageHash, targetTags, actualLog) + }) + if err != nil { + t.Fatalf("BuildImageBuildx returned an unexpected error: %v", err) + } + + gotArgs := recording.argv(t) + if strings.Join(gotArgs, "\x00") != strings.Join(wantArgs, "\x00") { + t.Fatalf("child argv did not match assembleBuildxArgs:\ngot: %#v\nwant: %#v", gotArgs, wantArgs) + } +} + +// TestBuildImageBuildxSetsDockerConfigOnChildOnly protects the two halves of +// the same CLAUDE.md rule at once: DOCKER_CONFIG must reach the CHILD pointing +// at the throwaway config dir, and the PARENT process's own DOCKER_CONFIG (ie. +// dockem's own environment) must be completely unaffected by the call - it is +// set on cmd.Env only, never via os.Setenv. This is the rule most likely to +// regress, since it is one line away from leaking into dockem's own process. +func TestBuildImageBuildxSetsDockerConfigOnChildOnly(t *testing.T) { + recording := installFakeDocker(t, 0) + + if _, isSet := os.LookupEnv("DOCKER_CONFIG"); isSet { + t.Fatalf("test precondition failed: DOCKER_CONFIG is already set in the test process") + } + + params, imageHash, targetTags := basicBuildxParams(t) + params.DockerUsername = "uname" + params.DockerPassword = "s3cr3t-buildx-password" + + buildLog := &BuildLog{} + _, _, err := runCapturingStreams(t, func() error { + return BuildImageBuildx(params, imageHash, targetTags, buildLog) + }) + if err != nil { + t.Fatalf("BuildImageBuildx returned an unexpected error: %v", err) + } + + childConfig, ok := recording.envValue(t, "DOCKER_CONFIG") + if !ok || childConfig == "" { + t.Fatalf("expected the child to see a non-empty DOCKER_CONFIG, got ok=%v value=%q", ok, childConfig) + } + + // The parent (this test process, standing in for dockem's own process) must + // come out exactly as it went in: no DOCKER_CONFIG at all. + if _, isSet := os.LookupEnv("DOCKER_CONFIG"); isSet { + t.Fatalf("DOCKER_CONFIG leaked into dockem's own process: %q", os.Getenv("DOCKER_CONFIG")) + } +} + +// TestBuildImageBuildxPassesThroughArbitraryParentEnvVar guards a real +// regression: if cmd.Env were ever narrowed to an allowlist of "the variables +// dockem cares about", --secret id=x,env=VAR would silently stop working, +// because VAR would never reach the child. An arbitrary FOO must survive +// untouched, both with and without explicit credentials (the two branches that +// build cmd.Env take different paths - nil vs a full copy - and both must +// pass arbitrary vars through). +func TestBuildImageBuildxPassesThroughArbitraryParentEnvVar(t *testing.T) { + t.Run("with credentials", func(t *testing.T) { + recording := installFakeDocker(t, 0) + t.Setenv("FOO", "bar") + + params, imageHash, targetTags := basicBuildxParams(t) + params.DockerUsername = "uname" + params.DockerPassword = "pw" + + buildLog := &BuildLog{} + _, _, err := runCapturingStreams(t, func() error { + return BuildImageBuildx(params, imageHash, targetTags, buildLog) + }) + if err != nil { + t.Fatalf("BuildImageBuildx returned an unexpected error: %v", err) + } + + if got, ok := recording.envValue(t, "FOO"); !ok || got != "bar" { + t.Fatalf("expected the child to see FOO=bar, got ok=%v value=%q", ok, got) + } + }) + + t.Run("without credentials", func(t *testing.T) { + recording := installFakeDocker(t, 0) + t.Setenv("FOO", "bar") + + params, imageHash, targetTags := basicBuildxParams(t) + + buildLog := &BuildLog{} + _, _, err := runCapturingStreams(t, func() error { + return BuildImageBuildx(params, imageHash, targetTags, buildLog) + }) + if err != nil { + t.Fatalf("BuildImageBuildx returned an unexpected error: %v", err) + } + + if got, ok := recording.envValue(t, "FOO"); !ok || got != "bar" { + t.Fatalf("expected the child to see FOO=bar, got ok=%v value=%q", ok, got) + } + }) +} + +// TestBuildImageBuildxChildCwdMatchesParent pins that cmd.Dir is never set: the +// child's working directory must be dockem's own cwd, so a relative +// `--secret id=x,src=./relative/path` resolves against dockem's cwd rather than +// the build context or anywhere else. +func TestBuildImageBuildxChildCwdMatchesParent(t *testing.T) { + recording := installFakeDocker(t, 0) + + wantCwd, err := os.Getwd() + if err != nil { + t.Fatalf("could not determine this test's own cwd: %v", err) + } + wantCwdResolved, err := filepath.EvalSymlinks(wantCwd) + if err != nil { + t.Fatalf("could not resolve this test's own cwd: %v", err) + } + + params, imageHash, targetTags := basicBuildxParams(t) + + buildLog := &BuildLog{} + _, _, runErr := runCapturingStreams(t, func() error { + return BuildImageBuildx(params, imageHash, targetTags, buildLog) + }) + if runErr != nil { + t.Fatalf("BuildImageBuildx returned an unexpected error: %v", runErr) + } + + gotCwd := recording.cwd(t) + gotCwdResolved, err := filepath.EvalSymlinks(gotCwd) + if err != nil { + t.Fatalf("could not resolve the child's reported cwd %q: %v", gotCwd, err) + } + + if gotCwdResolved != wantCwdResolved { + t.Fatalf("child cwd = %q, want %q (dockem's own cwd)", gotCwdResolved, wantCwdResolved) + } +} + +// TestBuildImageBuildxNoCredentialsLeavesEnvNil checks the no-credentials +// branch directly: with no --docker-username/--docker-password, cmd.Env must +// stay nil so the child inherits dockem's environment completely unchanged - +// which is what makes an existing `docker login` keep working. We cannot +// observe cmd.Env itself from outside the function, so instead we assert on +// its externally-visible consequence: every variable the parent has (PATH, +// and a distinctive marker) reaches the child with the exact values the parent +// had, and no DOCKER_CONFIG is introduced. +func TestBuildImageBuildxNoCredentialsLeavesEnvNil(t *testing.T) { + recording := installFakeDocker(t, 0) + t.Setenv("DOCKEM_MARKER", "present") + + params, imageHash, targetTags := basicBuildxParams(t) + + buildLog := &BuildLog{} + _, _, err := runCapturingStreams(t, func() error { + return BuildImageBuildx(params, imageHash, targetTags, buildLog) + }) + if err != nil { + t.Fatalf("BuildImageBuildx returned an unexpected error: %v", err) + } + + if got, ok := recording.envValue(t, "DOCKEM_MARKER"); !ok || got != "present" { + t.Fatalf("expected the child to inherit DOCKEM_MARKER unchanged, got ok=%v value=%q", ok, got) + } + if _, ok := recording.envValue(t, "DOCKER_CONFIG"); ok { + t.Fatalf("expected no DOCKER_CONFIG on the child when no credentials were given") + } +} + +// TestBuildImageBuildxStripsPreexistingDockerConfig checks that a +// DOCKER_CONFIG already present in dockem's own environment (eg. because the +// user exported one for their own `docker login` setup) is stripped rather +// than duplicated when credentials are supplied: the child must see exactly +// ONE DOCKER_CONFIG entry, and it must be dockem's throwaway one, not the +// user's original value. +func TestBuildImageBuildxStripsPreexistingDockerConfig(t *testing.T) { + recording := installFakeDocker(t, 0) + + preexisting := t.TempDir() + t.Setenv("DOCKER_CONFIG", preexisting) + + params, imageHash, targetTags := basicBuildxParams(t) + params.DockerUsername = "uname" + params.DockerPassword = "pw" + + buildLog := &BuildLog{} + _, _, err := runCapturingStreams(t, func() error { + return BuildImageBuildx(params, imageHash, targetTags, buildLog) + }) + if err != nil { + t.Fatalf("BuildImageBuildx returned an unexpected error: %v", err) + } + + if count := recording.envCount(t, "DOCKER_CONFIG"); count != 1 { + t.Fatalf("expected exactly one DOCKER_CONFIG entry on the child, found %d: %v", count, recording.env(t)) + } + + got, ok := recording.envValue(t, "DOCKER_CONFIG") + if !ok { + t.Fatalf("expected a DOCKER_CONFIG entry on the child") + } + if got == preexisting { + t.Fatalf("expected DOCKER_CONFIG to be dockem's throwaway dir, got the preexisting value %q", got) + } +} + +// TestBuildImageBuildxRemovesTempConfigDirAfterReturn checks that the +// throwaway config directory TempDockerConfig writes credentials into is +// removed once BuildImageBuildx returns, on both a clean exit and a failing +// one - the deferred cleanup must run regardless of how the subprocess exits. +func TestBuildImageBuildxRemovesTempConfigDirAfterReturn(t *testing.T) { + cases := []struct { + name string + exitCode int + wantErr bool + }{ + {"clean exit", 0, false}, + {"non-zero exit", 1, true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + recording := installFakeDocker(t, tc.exitCode) + + params, imageHash, targetTags := basicBuildxParams(t) + params.DockerUsername = "uname" + params.DockerPassword = "pw" + + buildLog := &BuildLog{} + _, _, err := runCapturingStreams(t, func() error { + return BuildImageBuildx(params, imageHash, targetTags, buildLog) + }) + if tc.wantErr && err == nil { + t.Fatalf("expected BuildImageBuildx to return an error for exit code %d", tc.exitCode) + } + if !tc.wantErr && err != nil { + t.Fatalf("BuildImageBuildx returned an unexpected error: %v", err) + } + + configDir, ok := recording.envValue(t, "DOCKER_CONFIG") + if !ok || configDir == "" { + t.Fatalf("expected the child to have recorded a DOCKER_CONFIG value to check") + } + + if _, statErr := os.Stat(configDir); !os.IsNotExist(statErr) { + t.Fatalf("expected the temp config dir %q to be removed after BuildImageBuildx returned, stat err = %v", configDir, statErr) + } + }) + } +} + +// TestBuildImageBuildxNonZeroExitIsError checks the basic error-surfacing +// contract: a failing `docker buildx build` must come back as a non-nil error +// from BuildImageBuildx, so BuildDockerImage propagates the failure instead of +// reporting a successful build that never happened. +func TestBuildImageBuildxNonZeroExitIsError(t *testing.T) { + installFakeDocker(t, 17) + + params, imageHash, targetTags := basicBuildxParams(t) + + buildLog := &BuildLog{} + _, _, err := runCapturingStreams(t, func() error { + return BuildImageBuildx(params, imageHash, targetTags, buildLog) + }) + if err == nil { + t.Fatalf("expected BuildImageBuildx to return an error for a non-zero subprocess exit") + } +} + +// TestBuildImageBuildxStreamsSubprocessOutputToStderr protects the +// --output-format=json contract: BuildImageBuildx sets BOTH cmd.Stdout and +// cmd.Stderr to os.Stderr, so anything the subprocess prints - on either of +// its own streams - must land on dockem's stderr, and dockem's stdout must +// stay completely empty. +func TestBuildImageBuildxStreamsSubprocessOutputToStderr(t *testing.T) { + installFakeDocker(t, 0) + + params, imageHash, targetTags := basicBuildxParams(t) + + buildLog := &BuildLog{} + stdout, stderr, err := runCapturingStreams(t, func() error { + return BuildImageBuildx(params, imageHash, targetTags, buildLog) + }) + if err != nil { + t.Fatalf("BuildImageBuildx returned an unexpected error: %v", err) + } + + if stdout != "" { + t.Fatalf("expected dockem's stdout to stay empty, got %q", stdout) + } + if !strings.Contains(stderr, "fake-docker-stdout-chatter") { + t.Fatalf("expected the child's own stdout chatter to land on dockem's stderr, got %q", stderr) + } + if !strings.Contains(stderr, "fake-docker-stderr-chatter") { + t.Fatalf("expected the child's own stderr chatter to land on dockem's stderr, got %q", stderr) + } +} + +// TestBuildImageBuildxPasswordNeverLeaks uses a distinctive sentinel password +// and greps everywhere it could conceivably leak: the child's argv (buildx's +// --secret/--tag syntax never carries a raw password, and DOCKER_CONFIG only +// ever carries a PATH to the credentials file, not the credentials +// themselves), the populated BuildLog (inspected directly - this test lives in +// package utils, so its unexported fields are visible here), and the JSON +// BuildResult a caller would actually see. +func TestBuildImageBuildxPasswordNeverLeaks(t *testing.T) { + recording := installFakeDocker(t, 0) + + const sentinelPassword = "sentinel-pw-4f8c9d2a-must-not-leak" + + params, imageHash, targetTags := basicBuildxParams(t) + params.DockerUsername = "uname" + params.DockerPassword = sentinelPassword + + buildLog := &BuildLog{} + _, _, err := runCapturingStreams(t, func() error { + return BuildImageBuildx(params, imageHash, targetTags, buildLog) + }) + if err != nil { + t.Fatalf("BuildImageBuildx returned an unexpected error: %v", err) + } + + for _, a := range recording.argv(t) { + if strings.Contains(a, sentinelPassword) { + t.Fatalf("the password leaked into the child's argv: %q", a) + } + } + + // BuildLog itself: dump every field's value via fmt (this test is in the + // same package, so unexported fields are directly readable) and confirm + // the sentinel is nowhere in it. + buildLogDump := fmt.Sprintf("%#v", *buildLog) + if strings.Contains(buildLogDump, sentinelPassword) { + t.Fatalf("the password leaked into BuildLog: %s", buildLogDump) + } + + result := buildLog.Result() + resultDump := fmt.Sprintf("%#v", result) + if strings.Contains(resultDump, sentinelPassword) { + t.Fatalf("the password leaked into BuildResult: %s", resultDump) + } +} + +// TestDescribePlatforms covers the small logging helper directly: no +// platforms reads as "the host platform", one or more platforms are rendered +// joined with ", ". +func TestDescribePlatforms(t *testing.T) { + cases := []struct { + name string + input []string + expected string + }{ + {"unset", nil, "the host platform"}, + {"empty slice", []string{}, "the host platform"}, + {"single platform", []string{"linux/amd64"}, "linux/amd64"}, + {"multiple platforms", []string{"linux/amd64", "linux/arm64"}, "linux/amd64, linux/arm64"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := describePlatforms(tc.input); got != tc.expected { + t.Errorf("describePlatforms(%#v) = %q, want %q", tc.input, got, tc.expected) + } + }) + } +} From 0314d81a18d73711a0c0735b1bc6924c93c02b1f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 10:01:52 +0000 Subject: [PATCH 4/5] ci(hygiene): Guard against a root plan.md, stdout writes and README drift Phase T0 of docs/testing-plan.md. Adds a Repo Hygiene workflow asserting no plan.md exists in the repository root, case-insensitively, so planning documents stay in docs/ and scratch state cannot reach develop or main. It runs on the same triggers as the unit test workflow and is kept separate from it to stay cheap and readable. Adds two source-reading guards in the spirit of the existing cache-hash guards. The first fails if fmt.Print* or a direct os.Stdout write appears in any non-test file outside write_build_output.go, enforcing the LogInfo/LogWarn/LogError convention that keeps stdout carrying nothing but the JSON result. The second fails if a flag registered in cli/cmd/build.go is missing from the README, enforcing the documented convention that the two stay in sync. This is the first test file in the cmd package. Both guards were verified to fail on an injected violation before landing; cobra's auto-generated --version flag is carried as a documented exception rather than by editing the README. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Tn4Aqbe88wWU6a5njxB8dA --- .github/workflows/hygiene.yaml | 28 ++++++++++ cli/cmd/readme_flag_coverage_test.go | 69 +++++++++++++++++++++++ cli/utils/stdout_purity_test.go | 82 ++++++++++++++++++++++++++++ 3 files changed, 179 insertions(+) create mode 100644 .github/workflows/hygiene.yaml create mode 100644 cli/cmd/readme_flag_coverage_test.go create mode 100644 cli/utils/stdout_purity_test.go diff --git a/.github/workflows/hygiene.yaml b/.github/workflows/hygiene.yaml new file mode 100644 index 0000000..1a26b0e --- /dev/null +++ b/.github/workflows/hygiene.yaml @@ -0,0 +1,28 @@ +name: "Repo Hygiene" + +on: + push: + branches: + - main + - develop + pull_request: + branches: + - main + - develop + +jobs: + hygiene: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + + - name: No plan.md in repo root + run: | + # Planning documents belong in docs/ (see docs/testing-plan.md), not + # the repo root. -maxdepth 1 keeps this root-only, so nested docs + # like docs/testing-plan.md never trip it. -iname is case-insensitive, + # so plan.md / PLAN.md / Plan.md are all caught. + if find . -maxdepth 1 -iname 'plan.md' | grep -q .; then + echo "ERROR: found a plan.md in the repository root. Planning documents belong in docs/ - move it there (eg. docs/plan.md) instead of the repo root." + exit 1 + fi diff --git a/cli/cmd/readme_flag_coverage_test.go b/cli/cmd/readme_flag_coverage_test.go new file mode 100644 index 0000000..48b0533 --- /dev/null +++ b/cli/cmd/readme_flag_coverage_test.go @@ -0,0 +1,69 @@ +package cmd + +import ( + "os" + "strings" + "testing" + + "github.com/spf13/pflag" +) + +// TestEveryBuildFlagIsDocumentedInReadme is the mechanical enforcement of the +// CLAUDE.md convention "The README documents every flag and concept; update +// it alongside any flag change in cli/cmd/build.go". Without this test, a +// flag added to (or renamed in) build.go silently drops out of sync with +// README.md and nothing notices until a user goes looking for it. +// +// Flags are enumerated from the live *cobra.Command via Flags().VisitAll, +// not by re-parsing build.go's source, so this test tracks whatever cobra +// actually registers - including flags cobra adds itself (see the +// knownUndocumented exception below) - rather than only what a human wrote +// as an explicit buildCmd.Flags().___ call. +// +// README.md documents flags by their long form (eg. "--tag"), so each flag's +// long name, prefixed with "--", must appear somewhere in the README text. +// This is deliberately loose (it does not check the description text matches, +// or that short flags like "-t" appear) - the goal is to catch a flag that +// was added or renamed and never mentioned at all, not to validate prose. +func TestEveryBuildFlagIsDocumentedInReadme(t *testing.T) { + // cobra only registers the auto-generated --help and --version flags + // inside Command.execute() (called from Execute()), which this test + // never calls. Force them into existence here so VisitAll sees exactly + // the flag set a real `dockem build --help` would show - otherwise this + // test would never have had a chance to catch the --version gap below. + buildCmd.InitDefaultHelpFlag() + buildCmd.InitDefaultVersionFlag() + + // Known, deliberate exception: cobra auto-generates -v/--version from + // buildCmd.Version (see build.go's `Version: Version` field). It is not + // declared alongside the rest of the flags in the init() block below, + // and - per the Phase T0 testing-plan note that first flagged this gap - + // it is genuinely absent from README.md today. Documenting it there is + // a separate, human decision about README content; this test's job is + // to guard against *new*, unnoticed gaps, not to silently paper over + // this pre-existing, already-known one by failing on it forever. + knownUndocumented := map[string]bool{ + "version": true, + } + + readmePath := "../../README.md" + readmeBytes, err := os.ReadFile(readmePath) + if err != nil { + t.Fatalf("could not read %s to verify flag documentation - if cli/cmd/build.go or the repo layout moved, update this test's relative path: %s", readmePath, err) + } + readme := string(readmeBytes) + + var missing []string + buildCmd.Flags().VisitAll(func(f *pflag.Flag) { + if knownUndocumented[f.Name] { + return + } + if !strings.Contains(readme, "--"+f.Name) { + missing = append(missing, f.Name) + } + }) + + if len(missing) > 0 { + t.Errorf("the following build flag(s) are registered in cli/cmd/build.go but not mentioned in README.md - update the README alongside any flag change (see CLAUDE.md conventions): %s", strings.Join(missing, ", ")) + } +} diff --git a/cli/utils/stdout_purity_test.go b/cli/utils/stdout_purity_test.go new file mode 100644 index 0000000..6e363f4 --- /dev/null +++ b/cli/utils/stdout_purity_test.go @@ -0,0 +1,82 @@ +package utils + +import ( + "os" + "path/filepath" + "regexp" + "strconv" + "strings" + "testing" +) + +// TestStdoutStaysPureOutsideWriteBuildOutput is a Phase T0 source-reading +// guard, in the same spirit as TestCacheFromCacheToExcludedFromImageHash in +// build_docker_image_cache_hash_test.go: the invariant it protects cannot be +// reached by any runtime assertion, only by reading dockem's own source. +// +// With --output-format=json, stdout must carry nothing but the JSON +// BuildResult (see write_build_output.go) - anything a piped consumer like +// `dockem build --output-format=json | jq ...` isn't expecting would corrupt +// the JSON it's trying to parse. That is exactly why CLAUDE.md mandates +// LogInfo/LogWarn/LogError (cli/utils/log.go, all writing to os.Stderr) +// instead of raw fmt.Print*/os.Stdout for every other message dockem emits. +// This test enforces that convention mechanically: it fails if fmt.Print, +// fmt.Printf, fmt.Println, or a direct write to os.Stdout appears anywhere +// in cli/utils/ or cli/cmd/, in a non-test source file, outside the one file +// that is deliberately allowed to write to stdout. +// +// Only non-test (*.go, not *_test.go) files are scanned. Test files never +// run as part of the built dockem binary, so a *_test.go file redirecting +// os.Stdout to capture output for an assertion (see write_build_output_test.go's +// captureStdout helper) is a test technique, not a stdout-purity violation - +// scoping to non-test files avoids having to special-case that pattern here. +func TestStdoutStaysPureOutsideWriteBuildOutput(t *testing.T) { + // The one file allowed to write to stdout: it implements the JSON + // result output itself, and is the sole reason stdout exists as a + // dockem output channel at all. + allowlist := map[string]bool{ + "write_build_output.go": true, + } + + // Matches fmt.Print(, fmt.Printf(, fmt.Println(, and any direct + // reference to os.Stdout (covering os.Stdout.Write(...), and any + // fmt.Fprint*(os.Stdout, ...) call). + violationPattern := regexp.MustCompile(`fmt\.Print(ln|f)?\(|os\.Stdout`) + + dirs := []string{ + "../utils", + "../cmd", + } + + var violations []string + for _, dir := range dirs { + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("could not read %s to scan for stdout-purity violations: %s", dir, err) + } + for _, entry := range entries { + name := entry.Name() + if entry.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + if allowlist[name] { + continue + } + path := filepath.Join(dir, name) + src, err := os.ReadFile(path) + if err != nil { + t.Fatalf("could not read %s to scan for stdout-purity violations: %s", path, err) + } + for i, line := range strings.Split(string(src), "\n") { + if violationPattern.MatchString(line) { + violations = append(violations, path+":"+strconv.Itoa(i+1)+": "+strings.TrimSpace(line)) + } + } + } + } + + if len(violations) > 0 { + t.Errorf("found fmt.Print*/os.Stdout usage outside write_build_output.go - with --output-format=json, stdout must carry nothing but the JSON result, so use LogInfo/LogWarn/LogError (cli/utils/log.go) instead:\n%s", strings.Join(violations, "\n")) + } +} + From b9ac39bd4c9f100ae36928d19fe02bc7b155cf36 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 10:02:22 +0000 Subject: [PATCH 5/5] docs(testing): Tick off the completed phases and refresh coverage T0, T2 and T3 have landed. Coverage on a non-registry run is now 52.6% for utils (from 38.2%) and 33.8% for cmd (from zero). One T0 item is deliberately left open: confirming the root plan.md guard actually goes red in real CI needs a scratch-branch push to observe, which cannot be done from a local run. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Tn4Aqbe88wWU6a5njxB8dA --- docs/testing-plan.md | 83 +++++++++++++++++++++++--------------------- 1 file changed, 44 insertions(+), 39 deletions(-) diff --git a/docs/testing-plan.md b/docs/testing-plan.md index 571ef26..80a6058 100644 --- a/docs/testing-plan.md +++ b/docs/testing-plan.md @@ -7,6 +7,11 @@ cheapest way to move the coverage number. **Status legend:** `[ ]` not started · `[~]` in progress · `[x]` done +**Progress:** T0, T2 and T3 are complete (one T0 item needs a real CI run to close). +T1, T4, T5 and T6 remain — each depends on a production refactor listed under +[Refactors this plan depends on](#refactors-this-plan-depends-on), which is why they +were not done alongside the rest. + > **Location note.** This plan lives in `docs/`, not at the repository root. A CI > guard (Phase T0) asserts that no `plan.md` exists in the root — see that phase for > the rationale. @@ -21,8 +26,8 @@ actually execute (ie. excluding everything in `build_docker_image_test.go` that | Package | Coverage | |---|---| -| `dockem/utils` | 38.2% | -| `dockem/cmd` | 0.0% | +| `dockem/utils` | 52.6% | +| `dockem/cmd` | 33.8% | | `dockem` | 0.0% | Functions at zero coverage: `ExtractVersion`, `ParseVersionFileJson`, @@ -47,27 +52,27 @@ Two existing patterns are the ones to clone rather than invent alternatives to: ## Phase T0 — Repo hygiene and CI guards -- [ ] Add a GitHub Action that asserts **no `plan.md` exists in the root of the +- [x] Add a GitHub Action that asserts **no `plan.md` exists in the root of the repository**. Planning documents belong in `docs/`; a stray root `plan.md` is scratch state that should never reach `develop` or `main`. The check must be case-insensitive (`plan.md`, `PLAN.md`, `Plan.md`) and must fail the build with a message pointing at `docs/` as the correct home. -- [ ] Wire it into the existing `.github/workflows/testing.yaml`, or a small separate +- [x] Wire it into the existing `.github/workflows/testing.yaml`, or a small separate `lint`/`hygiene` workflow, running on the same `push` / `pull_request` triggers for `main` and `develop`. -- [ ] Make it a shell step that is obvious to read and cheap to run, eg. a +- [x] Make it a shell step that is obvious to read and cheap to run, eg. a `find . -maxdepth 1 -iname 'plan.md'` that exits non-zero on any hit. - [ ] Confirm the guard actually fails when a root `plan.md` is present — commit one on a scratch branch, watch the check go red, then remove it. A guard that has never been seen to fail is not a guard. -- [ ] Remove the existing root `PLAN.md` (its v2.6.0 / v3.0.0 phases have landed) so +- [x] Remove the existing root `PLAN.md` (its v2.6.0 / v3.0.0 phases have landed) so the guard passes on `develop` the moment it is added. -- [ ] Add a stdout-purity guard in the same spirit: with `--output-format=json`, +- [x] Add a stdout-purity guard in the same spirit: with `--output-format=json`, stdout must carry nothing but JSON. A source-level test asserting no `fmt.Print*` / direct `os.Stdout` write exists outside `write_build_output.go` enforces the `LogInfo` / `LogWarn` / `LogError` convention in `CLAUDE.md` directly, rather than by review. -- [ ] Add a test asserting every flag registered in `cli/cmd/build.go` appears in +- [x] Add a test asserting every flag registered in `cli/cmd/build.go` appears in `README.md`, mechanically enforcing the documented "update the README alongside any flag change" convention. @@ -141,53 +146,53 @@ these need a refactor. ### T2.1 Version handling -- [ ] `ParseVersionFileJson`: valid JSON, malformed JSON, non-string `version` value. -- [ ] `ExtractVersion`: valid file, missing file, unreadable file. -- [ ] Decide and then pin the two edges that currently produce tags silently: +- [x] `ParseVersionFileJson`: valid JSON, malformed JSON, non-string `version` value. +- [x] `ExtractVersion`: valid file, missing file, unreadable file. +- [x] Decide and then pin the two edges that currently produce tags silently: `{}` yields the version string `"v"`, and `{"version": "v1.0.0"}` yields `"vv1.0.0"`. Either is a plausible tag name, so neither fails loudly today. ### T2.2 Hashing helpers -- [ ] `HashWatchFiles`: empty list returns `""` — the same "contributes nothing" +- [x] `HashWatchFiles`: empty list returns `""` — the same "contributes nothing" contract `hashPlatforms` has, and equally load-bearing for the cache identity of users who never adopt the flag. -- [ ] `HashWatchFiles`: order invariance, missing file errors, content change changes +- [x] `HashWatchFiles`: order invariance, missing file errors, content change changes the hash. -- [ ] `HashWatchDirectories`: empty list returns `""`; sort invariance; multiple +- [x] `HashWatchDirectories`: empty list returns `""`; sort invariance; multiple directories concatenate; missing directory errors. -- [ ] `HashWatchDirectories`: `excludePatterns` are applied consistently with +- [x] `HashWatchDirectories`: `excludePatterns` are applied consistently with `HashDirectory`. -- [ ] `HashWatchDirectories` calls `sort.Strings` on the caller's slice, mutating +- [x] `HashWatchDirectories` calls `sort.Strings` on the caller's slice, mutating `params.WatchDirectory` in place. Either pin that as intended or fix it and test that the caller's slice is left untouched. -- [ ] `HashString`: a known SHA256 vector, determinism across calls, empty input. +- [x] `HashString`: a known SHA256 vector, determinism across calls, empty input. ### T2.3 `ReadDockerignore` -- [ ] A missing ignore file is not an error and contributes no patterns. -- [ ] Comments and blank lines are stripped (via `ignorefile.ReadAll`). -- [ ] `--ignore-file` overrides `/.dockerignore`. -- [ ] An unreadable file (mode `000`) surfaces an error. -- [ ] `--exclude` patterns land **after** the file's patterns, so a +- [x] A missing ignore file is not an error and contributes no patterns. +- [x] Comments and blank lines are stripped (via `ignorefile.ReadAll`). +- [x] `--ignore-file` overrides `/.dockerignore`. +- [x] An unreadable file (mode `000`) surfaces an error. +- [x] `--exclude` patterns land **after** the file's patterns, so a `--exclude '!keep-me'` can re-include something the file excluded. That ordering is a real behavioural contract and is currently only implied by the code. ### T2.4 Buildx detection -- [ ] `parseBuildxVersion`: normal `github.com/docker/buildx v0.36.1 ` output; no +- [x] `parseBuildxVersion`: normal `github.com/docker/buildx v0.36.1 ` output; no version-looking token (returns the trimmed output); empty output; a suffixed version such as `v0.36.1-desktop.1`. -- [ ] `DetectBuildx` via `t.Setenv("PATH", tmpdir)` and a fake `docker` shim: exits 0 +- [x] `DetectBuildx` via `t.Setenv("PATH", tmpdir)` and a fake `docker` shim: exits 0 with known output, exits non-zero, and is absent from `PATH` entirely. -- [ ] Pin the contract that every failure mode returns `(false, "", nil)` and never an +- [x] Pin the contract that every failure mode returns `(false, "", nil)` and never an error — `ResolveBuilder`, not `DetectBuildx`, decides when that is fatal. ### T2.5 Small helpers -- [ ] `GenerateDockerImageName`: empty registry omits the host prefix; a set registry +- [x] `GenerateDockerImageName`: empty registry omits the host prefix; a set registry includes it; an image name that already contains a slash. -- [ ] `RemoveEmptyStringsFromArray`: order preserved; returns `nil` rather than an +- [x] `RemoveEmptyStringsFromArray`: order preserved; returns `nil` rather than an empty slice for all-empty input; whitespace-only strings are **not** removed, so `--tag " "` currently survives into a tag name. @@ -200,26 +205,26 @@ where every rule in the "Subprocess credentials (buildx)" section of `CLAUDE.md` actually lives. A single test with a fake `docker` on `PATH` — a shell script that dumps its argv, environment and cwd to a file — covers nearly all of it. -- [ ] argv matches what `assembleBuildxArgs` produced. -- [ ] `DOCKER_CONFIG` reaches the child pointing at the temp config dir. -- [ ] `os.Getenv("DOCKER_CONFIG")` in the parent test process is **unchanged** — it +- [x] argv matches what `assembleBuildxArgs` produced. +- [x] `DOCKER_CONFIG` reaches the child pointing at the temp config dir. +- [x] `os.Getenv("DOCKER_CONFIG")` in the parent test process is **unchanged** — it must be set on `cmd.Env` only, never on dockem's own environment. -- [ ] An arbitrary `FOO=bar` from the parent environment reaches the child. Narrowing +- [x] An arbitrary `FOO=bar` from the parent environment reaches the child. Narrowing `cmd.Env` to an allowlist would silently break `--secret id=x,env=VAR`, so this guards a real regression. -- [ ] The child's cwd equals dockem's cwd (`cmd.Dir` is never set), so a relative +- [x] The child's cwd equals dockem's cwd (`cmd.Dir` is never set), so a relative `src=` path in a `--secret` resolves against dockem's cwd as documented. -- [ ] With no credentials, `cmd.Env` is nil and the environment passes through +- [x] With no credentials, `cmd.Env` is nil and the environment passes through untouched, so an existing `docker login` keeps working. -- [ ] Any pre-existing `DOCKER_CONFIG` in the parent environment is stripped rather +- [x] Any pre-existing `DOCKER_CONFIG` in the parent environment is stripped rather than duplicated when dockem sets its own. -- [ ] The temp config dir is removed after return — including on a non-zero exit from +- [x] The temp config dir is removed after return — including on a non-zero exit from the subprocess. -- [ ] A non-zero exit surfaces as an error from `BuildImageBuildx`. -- [ ] Subprocess output goes to stderr, not stdout, keeping stdout clean for +- [x] A non-zero exit surfaces as an error from `BuildImageBuildx`. +- [x] Subprocess output goes to stderr, not stdout, keeping stdout clean for `--output-format=json`. -- [ ] The password never appears in argv, in `BuildLog`, or in the JSON result. -- [ ] `describePlatforms`: unset, single, and multiple platform lists. +- [x] The password never appears in argv, in `BuildLog`, or in the JSON result. +- [x] `describePlatforms`: unset, single, and multiple platform lists. ---