From 9386c6bf75216f392ff4b478f8714960560baf38 Mon Sep 17 00:00:00 2001 From: antoinetoussaint-byte <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 25 Oct 2026 19:06:14 -0400 Subject: [PATCH 1/3] fix(companions): proto 0.0.18 for the streaming facade; refuse an unbumped companion change 08716e9 changed the proto companion's Python facade plugin and kept 0.0.17. 0.0.17 had been published with the old plugin, then was republished with the new one, so a machine that cached the first image still renders every streaming RPC as `unary` (obin-ai/module-model- gateway#210's model client). Proto moves to 0.0.18, a tag no machine has cached. internal/companionbump, run by companions-build on every pull request, refuses a change to a companion's image inputs (its Dockerfile and every build-context COPY source: BuildSpec.Inputs) unless the companion's version is above the base's. Comparing with the base's tip also refuses a version another merged change already took. Run against 08716e9 it refuses exactly that change. The COPY parser the buildspec tests used moves into the package (CopySources) so the check and the tests read Dockerfiles one way. Closes #837 Co-Authored-By: Claude Opus 5.5 --- .github/workflows/companions-build.yml | 22 ++++ companions/buildspec.go | 73 ++++++++++++ companions/buildspec_test.go | 73 +++--------- companions/proto/README.md | 12 ++ companions/proto/info.codefly.yaml | 2 +- internal/companionbump/main.go | 151 +++++++++++++++++++++++++ internal/companionbump/main_test.go | 89 +++++++++++++++ 7 files changed, 364 insertions(+), 58 deletions(-) create mode 100644 internal/companionbump/main.go create mode 100644 internal/companionbump/main_test.go diff --git a/.github/workflows/companions-build.yml b/.github/workflows/companions-build.yml index f8369d4c3..f81bbd0ef 100644 --- a/.github/workflows/companions-build.yml +++ b/.github/workflows/companions-build.yml @@ -14,6 +14,7 @@ on: paths: - companions/** - internal/buildspecs/** + - internal/companionbump/** - .dockerignore - .github/workflows/companions-build.yml push: @@ -21,6 +22,7 @@ on: paths: - companions/** - internal/buildspecs/** + - internal/companionbump/** - .dockerignore - .github/workflows/companions-build.yml workflow_dispatch: @@ -29,6 +31,26 @@ permissions: contents: read jobs: + # A change to a companion's image inputs must bump its version: publishing + # skips a tag the registry already has, and a forced republish swaps the + # image under every machine that cached the tag. companions/proto 0.0.17 + # shipped the streaming Python facade that way (core#837). + version: + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: go.mod + cache: false + - name: every changed companion carries a new version + env: + BASE_REF: origin/${{ github.base_ref }} + run: go run ./internal/companionbump "$BASE_REF" + plan: runs-on: ubuntu-latest outputs: diff --git a/companions/buildspec.go b/companions/buildspec.go index a7e4f8c7e..f73e16c72 100644 --- a/companions/buildspec.go +++ b/companions/buildspec.go @@ -2,7 +2,11 @@ package companions import ( "fmt" + "os" + "path" + "path/filepath" "slices" + "strings" ) // BaseImageArg is the Docker build argument a companion Dockerfile declares @@ -106,3 +110,72 @@ func BuildSpecs() ([]BuildSpec, error) { } return out, nil } + +// CopyKind classifies a Dockerfile line for CopySources. +type CopyKind int + +const ( + // NotCopy is any line that is not a COPY instruction. + NotCopy CopyKind = iota + // FromStage is COPY --from=, which reads from another image + // rather than from the build context. + FromStage + // FromContext is a COPY that reads paths out of the build context. + FromContext + // Unparsed is a COPY whose shape this parser does not understand — the + // JSON form, or one with no source and destination. It is reported rather + // than skipped: a COPY nobody reads is an image input nothing checks. + Unparsed +) + +// CopySources returns the build-context sources of a COPY instruction. +// +// Leading flags are stripped rather than abandoning the line. Skipping any +// flagged COPY would leave `COPY --chown=…` and `COPY --link` — both ordinary +// context reads — unread, so their sources would escape every check. +func CopySources(line string) ([]string, CopyKind) { + fields := strings.Fields(line) + if len(fields) == 0 || fields[0] != "COPY" { + return nil, NotCopy + } + operands := fields[1:] + for len(operands) > 0 && strings.HasPrefix(operands[0], "--") { + if strings.HasPrefix(operands[0], "--from=") { + return nil, FromStage + } + operands = operands[1:] + } + if len(operands) < 2 || strings.HasPrefix(operands[0], "[") { + return nil, Unparsed + } + return operands[:len(operands)-1], FromContext +} + +// Inputs returns the repository-root-relative POSIX paths, read from the +// checkout at root, that this companion's image is built from in this +// repository: its Dockerfile and every build-context COPY source, a file or a +// directory. The staged CLI binary is not one: it is built from codefly-dev/cli +// at publish time. A change under any input changes the image, so it needs a +// new Version: publishing skips a tag the registry already has. +func (s BuildSpec) Inputs(root string) ([]string, error) { + content, err := os.ReadFile(filepath.Join(root, filepath.FromSlash(s.Dockerfile))) //nolint:gosec // the spec's own Dockerfile under the checkout root + if err != nil { + return nil, fmt.Errorf("cannot read the <%s> Dockerfile: %w", s.Name, err) + } + inputs := []string{s.Dockerfile} + for number, line := range strings.Split(string(content), "\n") { + sources, kind := CopySources(line) + switch kind { + case Unparsed: + return nil, fmt.Errorf("%s:%d is a COPY whose sources cannot be read: %s", s.Dockerfile, number+1, strings.TrimSpace(line)) + case FromContext: + for _, source := range sources { + if s.CLIBinary != "" && source == s.CLIBinary { + continue + } + inputs = append(inputs, path.Clean(path.Join(s.Context, source))) + } + } + } + return inputs, nil +} diff --git a/companions/buildspec_test.go b/companions/buildspec_test.go index 1d576b17b..13f5297d2 100644 --- a/companions/buildspec_test.go +++ b/companions/buildspec_test.go @@ -216,64 +216,23 @@ func TestBuildSpecsMatchWhatTheCurrentPublisherApplies(t *testing.T) { } } -// copyKind classifies a Dockerfile line for copySources. -type copyKind int - -const ( - // notCopy is any line that is not a COPY instruction. - notCopy copyKind = iota - // fromStage is COPY --from=, which reads from another image - // rather than from the build context. - fromStage - // fromContext is a COPY that reads paths out of the build context. - fromContext - // unparsed is a COPY whose shape this parser does not understand — the - // JSON form, or one with no source and destination. It is reported rather - // than skipped: a COPY nobody checks is exactly the drift these tests - // exist to catch. - unparsed -) - -// copySources returns the build-context sources of a COPY instruction. -// -// Leading flags are stripped rather than abandoning the line. Skipping any -// flagged COPY would leave `COPY --chown=…` and `COPY --link` — both ordinary -// context reads — unguarded, so a bad path in one would reach the builder. -func copySources(line string) ([]string, copyKind) { - fields := strings.Fields(line) - if len(fields) == 0 || fields[0] != "COPY" { - return nil, notCopy - } - operands := fields[1:] - for len(operands) > 0 && strings.HasPrefix(operands[0], "--") { - if strings.HasPrefix(operands[0], "--from=") { - return nil, fromStage - } - operands = operands[1:] - } - if len(operands) < 2 || strings.HasPrefix(operands[0], "[") { - return nil, unparsed - } - return operands[:len(operands)-1], fromContext -} - func TestCopySourcesReadsEveryContextCopyShape(t *testing.T) { for line, expected := range map[string]struct { sources []string - kind copyKind + kind companions.CopyKind }{ - "COPY facades/go /src": {[]string{"facades/go"}, fromContext}, - "COPY a b /dst/": {[]string{"a", "b"}, fromContext}, - "COPY --chown=1000:1000 facades/ts /opt": {[]string{"facades/ts"}, fromContext}, - "COPY --link --chown=root:root bin/x /bin/x": {[]string{"bin/x"}, fromContext}, - " COPY bin/linux/${TARGETARCH}/codefly /bin/c": {[]string{"bin/linux/${TARGETARCH}/codefly"}, fromContext}, - "COPY --from=builder /go/bin/buf /usr/local/": {nil, fromStage}, - "RUN apk add --no-cache bash": {nil, notCopy}, - "": {nil, notCopy}, - `COPY ["a", "b"]`: {nil, unparsed}, - "COPY onlyone": {nil, unparsed}, + "COPY facades/go /src": {[]string{"facades/go"}, companions.FromContext}, + "COPY a b /dst/": {[]string{"a", "b"}, companions.FromContext}, + "COPY --chown=1000:1000 facades/ts /opt": {[]string{"facades/ts"}, companions.FromContext}, + "COPY --link --chown=root:root bin/x /bin/x": {[]string{"bin/x"}, companions.FromContext}, + " COPY bin/linux/${TARGETARCH}/codefly /bin/c": {[]string{"bin/linux/${TARGETARCH}/codefly"}, companions.FromContext}, + "COPY --from=builder /go/bin/buf /usr/local/": {nil, companions.FromStage}, + "RUN apk add --no-cache bash": {nil, companions.NotCopy}, + "": {nil, companions.NotCopy}, + `COPY ["a", "b"]`: {nil, companions.Unparsed}, + "COPY onlyone": {nil, companions.Unparsed}, } { - sources, kind := copySources(line) + sources, kind := companions.CopySources(line) require.Equalf(t, expected.kind, kind, "kind for %q", line) require.Equalf(t, expected.sources, sources, "sources for %q", line) } @@ -287,11 +246,11 @@ func TestDockerfileCopySourcesResolveInTheDeclaredContext(t *testing.T) { for _, spec := range buildSpecs(t) { context := filepath.Join(root, filepath.FromSlash(spec.Context)) for number, line := range strings.Split(dockerfile(t, spec), "\n") { - sources, kind := copySources(line) - require.NotEqualf(t, unparsed, kind, + sources, kind := companions.CopySources(line) + require.NotEqualf(t, companions.Unparsed, kind, "%s:%d is a COPY this test cannot read, so nothing checks its sources: %s", spec.Dockerfile, number+1, strings.TrimSpace(line)) - if kind != fromContext { + if kind != companions.FromContext { continue } for _, source := range sources { @@ -340,7 +299,7 @@ func TestDockerIgnoreExcludesNothingTheBuildsNeed(t *testing.T) { required = append(required, spec.CLIBinary) } for _, line := range strings.Split(dockerfile(t, spec), "\n") { - if sources, kind := copySources(line); kind == fromContext { + if sources, kind := companions.CopySources(line); kind == companions.FromContext { required = append(required, sources...) } } diff --git a/companions/proto/README.md b/companions/proto/README.md index 917bdefd7..6749117c5 100644 --- a/companions/proto/README.md +++ b/companions/proto/README.md @@ -125,6 +125,18 @@ local plugins: it compiles the `*_pb2` messages and `*_pb2_grpc` stubs with the own, and their gencode outran the protobuf runtime a generated library declares. It ships in companion version 0.0.17. +The Python facade plugin (`facades/python/protoc_gen_codefly_facade_python.py`) +calls the gateway seam named for each RPC's shape: `unary`, `unary_stream`, +`stream_unary` or `stream_stream`. It ships in companion version 0.0.18. An +earlier `0.0.17` image renders every RPC as `unary`; 0.0.17 was republished +with the streaming plugin, and a machine that had pulled it before keeps the +old image. + +A change to any of a companion's image inputs (its Dockerfile and every +build-context `COPY` source, `BuildSpec.Inputs`) must bump its +`info.codefly.yaml`: `internal/companionbump` refuses a pull request that does +not. + Rust generation stages output before publishing it. `.codefly-rust-output.json` records the generated files and their hashes; only unchanged, tracked files may be replaced or removed. Unrelated files are preserved. A collision with an diff --git a/companions/proto/info.codefly.yaml b/companions/proto/info.codefly.yaml index f2e5f6d40..8c04ba083 100644 --- a/companions/proto/info.codefly.yaml +++ b/companions/proto/info.codefly.yaml @@ -1 +1 @@ -version: 0.0.17 +version: 0.0.18 diff --git a/internal/companionbump/main.go b/internal/companionbump/main.go new file mode 100644 index 000000000..8a82fbf55 --- /dev/null +++ b/internal/companionbump/main.go @@ -0,0 +1,151 @@ +// Command companionbump refuses a change to a companion's image inputs that +// keeps the companion's version. +// +// go run ./internal/companionbump +// +// A companion's tag is its info.codefly.yaml version, and publishing skips a +// tag the registry already has. A change that keeps the version therefore +// either never ships or, forced, swaps the image under a tag machines have +// already cached, and those machines keep the old one. Run from the +// repository root of a checkout with 's history. A companion +// whose inputs (BuildSpec.Inputs) changed since its merge base with +// must carry a version above the one at . Taking the +// base's tip rather than the merge base also refuses a version that another +// change on the base already took. +package main + +import ( + "errors" + "fmt" + "os" + "os/exec" + "strings" + + "github.com/Masterminds/semver" + "github.com/codefly-dev/core/companions" + "gopkg.in/yaml.v3" +) + +// companion is what the check needs to know about one companion. +type companion struct { + name string + version string // at HEAD + inputs []string // repository-root-relative files and directories +} + +// unbumped returns one refusal for every companion with a changed input whose +// version is not above its version at the base. base returns the version the +// base declares, and false when the companion does not exist there yet. +func unbumped(all []companion, changed []string, base func(name string) (string, bool, error)) ([]string, error) { + var refusals []string + for _, c := range all { + touched := touchedInputs(c.inputs, changed) + if len(touched) == 0 { + continue + } + was, existed, err := base(c.name) + if err != nil { + return nil, err + } + if !existed { + continue + } + now, err := semver.NewVersion(c.version) + if err != nil { + return nil, fmt.Errorf("companion <%s>: %w", c.name, err) + } + before, err := semver.NewVersion(was) + if err != nil { + return nil, fmt.Errorf("companion <%s> at the base: %w", c.name, err) + } + if now.GreaterThan(before) { + continue + } + refusals = append(refusals, fmt.Sprintf( + "companion <%s> changes its image (%s) but companions/%s/info.codefly.yaml says %s, and the base says %s: bump it above %s", + c.name, strings.Join(touched, ", "), c.name, c.version, was, was)) + } + return refusals, nil +} + +// touchedInputs returns the changed files that are an input or lie under one. +func touchedInputs(inputs, changed []string) []string { + var touched []string + for _, file := range changed { + for _, input := range inputs { + if file == input || strings.HasPrefix(file, input+"/") { + touched = append(touched, file) + break + } + } + } + return touched +} + +func git(args ...string) (string, error) { + out, err := exec.Command("git", args...).Output() //nolint:gosec // fixed git subcommands; the one variable is the base ref the workflow passes + if err != nil { + var exit *exec.ExitError + if errors.As(err, &exit) { + return "", fmt.Errorf("git %s: %w: %s", strings.Join(args, " "), err, strings.TrimSpace(string(exit.Stderr))) + } + return "", fmt.Errorf("git %s: %w", strings.Join(args, " "), err) + } + return string(out), nil +} + +func run(baseRef string) ([]string, error) { + specs, err := companions.BuildSpecs() + if err != nil { + return nil, err + } + all := make([]companion, 0, len(specs)) + for _, spec := range specs { + inputs, inputsErr := spec.Inputs(".") + if inputsErr != nil { + return nil, inputsErr + } + all = append(all, companion{name: spec.Name, version: spec.Version, inputs: inputs}) + } + diff, err := git("diff", "--name-only", baseRef+"...HEAD") + if err != nil { + return nil, err + } + base := func(name string) (string, bool, error) { + manifest := "companions/" + name + "/info.codefly.yaml" + if _, err := git("cat-file", "-e", baseRef+":"+manifest); err != nil { + return "", false, nil + } + content, err := git("show", baseRef+":"+manifest) + if err != nil { + return "", false, err + } + var info struct { + Version string `yaml:"version"` + } + if err := yaml.Unmarshal([]byte(content), &info); err != nil { + return "", false, fmt.Errorf("%s at %s: %w", manifest, baseRef, err) + } + return info.Version, true, nil + } + return unbumped(all, strings.Fields(diff), base) +} + +func main() { + if len(os.Args) != 2 { + fmt.Fprintln(os.Stderr, "usage: companionbump ") + os.Exit(2) + } + refusals, err := run(os.Args[1]) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + for _, refusal := range refusals { + fmt.Fprintln(os.Stderr, refusal) + } + if len(refusals) > 0 { + os.Exit(1) + } + fmt.Println("every companion whose image changed carries a new version") +} diff --git a/internal/companionbump/main_test.go b/internal/companionbump/main_test.go new file mode 100644 index 000000000..589745e75 --- /dev/null +++ b/internal/companionbump/main_test.go @@ -0,0 +1,89 @@ +package main + +import ( + "testing" + + "github.com/codefly-dev/core/companions" + "github.com/stretchr/testify/require" +) + +var proto = companion{ + name: "proto", + version: "0.0.17", + inputs: []string{"companions/proto/Dockerfile", "companions/proto/facades/python"}, +} + +func baseAt(versions map[string]string) func(string) (string, bool, error) { + return func(name string) (string, bool, error) { + v, ok := versions[name] + return v, ok, nil + } +} + +// The case that shipped: the Python facade plugin changed and proto stayed +// 0.0.17, so the republished 0.0.17 never reached a machine that had it. +func TestAnInputChangeKeepingTheVersionIsRefused(t *testing.T) { + refusals, err := unbumped([]companion{proto}, + []string{"companions/proto/facades/python/protoc_gen_codefly_facade_python.py"}, + baseAt(map[string]string{"proto": "0.0.17"})) + require.NoError(t, err) + require.Len(t, refusals, 1) + require.Contains(t, refusals[0], "companion ") + require.Contains(t, refusals[0], "companions/proto/facades/python/protoc_gen_codefly_facade_python.py") + require.Contains(t, refusals[0], "bump it above 0.0.17") +} + +func TestABumpedVersionPasses(t *testing.T) { + bumped := proto + bumped.version = "0.0.18" + refusals, err := unbumped([]companion{bumped}, + []string{"companions/proto/Dockerfile"}, baseAt(map[string]string{"proto": "0.0.17"})) + require.NoError(t, err) + require.Empty(t, refusals) +} + +// Two branches taking the same new version would publish two images under one +// tag; the base's tip already holding it refuses the second. +func TestAVersionTheBaseAlreadyTookIsRefused(t *testing.T) { + bumped := proto + bumped.version = "0.0.18" + refusals, err := unbumped([]companion{bumped}, + []string{"companions/proto/Dockerfile"}, baseAt(map[string]string{"proto": "0.0.18"})) + require.NoError(t, err) + require.Len(t, refusals, 1) +} + +func TestAChangeOutsideTheInputsNeedsNoBump(t *testing.T) { + refusals, err := unbumped([]companion{proto}, []string{ + "companions/proto/README.md", + "companions/proto/testdata/facade/test_facade_gen.py", + "companions/proto/facades/pythonic/x.py", // a sibling sharing the prefix, not under the input + }, baseAt(map[string]string{"proto": "0.0.17"})) + require.NoError(t, err) + require.Empty(t, refusals) +} + +func TestANewCompanionNeedsNoBump(t *testing.T) { + refusals, err := unbumped([]companion{proto}, + []string{"companions/proto/Dockerfile"}, baseAt(map[string]string{})) + require.NoError(t, err) + require.Empty(t, refusals) +} + +// The real specs: every companion's Dockerfile is an input, and proto's facade +// plugins (the files 08716e9 changed) are inputs of proto. +func TestTheCheckoutsInputsCoverTheFacadePlugins(t *testing.T) { + specs, err := companions.BuildSpecs() + require.NoError(t, err) + byName := map[string][]string{} + for _, spec := range specs { + inputs, err := spec.Inputs("../..") + require.NoError(t, err) + require.Contains(t, inputs, spec.Dockerfile) + byName[spec.Name] = inputs + } + require.NotEmpty(t, touchedInputs(byName["proto"], + []string{"companions/proto/facades/python/protoc_gen_codefly_facade_python.py"})) + require.Empty(t, touchedInputs(byName["proto"], []string{"companions/proto/README.md"})) + require.NotContains(t, byName["codefly"], "bin/linux/${TARGETARCH}/codefly") +} From ba33ba1f1b3fd30b806d5928f43a785aa520b1d7 Mon Sep 17 00:00:00 2001 From: antoinetoussaint-byte <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 25 Oct 2026 21:04:50 -0400 Subject: [PATCH 2/3] fix(companions): the version check runs on every PR; renames and every copy shape count Review r65 on #838: - The check moves to its own workflow with no path filter, so it reports on every pull request and can be a required check of main's ruleset (a path-filtered required check stays pending on a PR it skips). - git diff runs with --no-renames: moving a file out of an input lists it at its old path too, so it no longer escapes. A test in a scratch repository fails without the flag. - The Dockerfile reader takes logical instructions (continued lines joined, comments inside them dropped) and reads ADD as well as COPY, in any case. An ADD of a URL or git remote and a heredoc are not context inputs. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/companion-version.yml | 34 ++++++++ .github/workflows/companions-build.yml | 22 ------ companions/buildspec.go | 100 ++++++++++++++++++++---- companions/buildspec_test.go | 29 +++++-- companions/proto/README.md | 4 +- internal/companionbump/main.go | 15 +++- internal/companionbump/main_test.go | 34 ++++++++ 7 files changed, 191 insertions(+), 47 deletions(-) create mode 100644 .github/workflows/companion-version.yml diff --git a/.github/workflows/companion-version.yml b/.github/workflows/companion-version.yml new file mode 100644 index 000000000..ac391ce2a --- /dev/null +++ b/.github/workflows/companion-version.yml @@ -0,0 +1,34 @@ +name: companion-version + +# A change to a companion's image inputs must bump its version: publishing +# skips a tag the registry already has, and a forced republish swaps the image +# under every machine that cached the tag. companions/proto 0.0.17 shipped the +# streaming Python facade that way (core#837). +# +# It runs on every pull request, with no path filter, so it can be a required +# check of main's ruleset: a path-filtered workflow that does not run leaves a +# required check pending forever. internal/companionbump reads which files are +# a companion's inputs from the Dockerfiles themselves; a change that touches +# none passes at once. +on: + pull_request: + +permissions: + contents: read + +jobs: + companion-version: + name: Companion version + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: go.mod + - name: every changed companion carries a new version + env: + BASE_REF: origin/${{ github.base_ref }} + run: go run ./internal/companionbump "$BASE_REF" diff --git a/.github/workflows/companions-build.yml b/.github/workflows/companions-build.yml index f81bbd0ef..f8369d4c3 100644 --- a/.github/workflows/companions-build.yml +++ b/.github/workflows/companions-build.yml @@ -14,7 +14,6 @@ on: paths: - companions/** - internal/buildspecs/** - - internal/companionbump/** - .dockerignore - .github/workflows/companions-build.yml push: @@ -22,7 +21,6 @@ on: paths: - companions/** - internal/buildspecs/** - - internal/companionbump/** - .dockerignore - .github/workflows/companions-build.yml workflow_dispatch: @@ -31,26 +29,6 @@ permissions: contents: read jobs: - # A change to a companion's image inputs must bump its version: publishing - # skips a tag the registry already has, and a forced republish swaps the - # image under every machine that cached the tag. companions/proto 0.0.17 - # shipped the streaming Python facade that way (core#837). - version: - if: github.event_name == 'pull_request' - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - fetch-depth: 0 - - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 - with: - go-version-file: go.mod - cache: false - - name: every changed companion carries a new version - env: - BASE_REF: origin/${{ github.base_ref }} - run: go run ./internal/companionbump "$BASE_REF" - plan: runs-on: ubuntu-latest outputs: diff --git a/companions/buildspec.go b/companions/buildspec.go index f73e16c72..c389380ce 100644 --- a/companions/buildspec.go +++ b/companions/buildspec.go @@ -111,31 +111,80 @@ func BuildSpecs() ([]BuildSpec, error) { return out, nil } -// CopyKind classifies a Dockerfile line for CopySources. +// Instruction is one logical Dockerfile instruction: its physical lines +// joined where a line ends in the escape character, comment lines inside it +// dropped, and Line the physical line it starts on. +type Instruction struct { + Line int + Text string +} + +// Instructions splits a Dockerfile into its logical instructions. A COPY +// written over several lines is one instruction, so its sources on the later +// lines are read with it rather than missed. +func Instructions(dockerfile string) []Instruction { + var out []Instruction + var current []string + start := 0 + for number, raw := range strings.Split(dockerfile, "\n") { + line := strings.TrimRight(raw, " \t\r") + trimmed := strings.TrimSpace(line) + if len(current) == 0 && (trimmed == "" || strings.HasPrefix(trimmed, "#")) { + continue + } + if len(current) > 0 && strings.HasPrefix(trimmed, "#") { + continue // a comment inside a continued instruction is not part of it + } + if len(current) == 0 { + start = number + 1 + } + if strings.HasSuffix(line, "\\") { + current = append(current, strings.TrimSuffix(line, "\\")) + continue + } + current = append(current, line) + out = append(out, Instruction{Line: start, Text: strings.Join(current, " ")}) + current = nil + } + if len(current) > 0 { + out = append(out, Instruction{Line: start, Text: strings.Join(current, " ")}) + } + return out +} + +// CopyKind classifies an instruction for CopySources. type CopyKind int const ( - // NotCopy is any line that is not a COPY instruction. + // NotCopy is any instruction that is neither COPY nor ADD. NotCopy CopyKind = iota // FromStage is COPY --from=, which reads from another image // rather than from the build context. FromStage - // FromContext is a COPY that reads paths out of the build context. + // FromContext is a COPY or ADD that reads paths out of the build context. + // An ADD source that is a URL or a git remote is not one, and is left out. FromContext - // Unparsed is a COPY whose shape this parser does not understand — the - // JSON form, or one with no source and destination. It is reported rather - // than skipped: a COPY nobody reads is an image input nothing checks. + // Unparsed is a COPY or ADD whose shape this parser does not understand — + // the JSON form, or one with no source and destination. It is reported + // rather than skipped: a copy nobody reads is an image input nothing + // checks. Unparsed ) -// CopySources returns the build-context sources of a COPY instruction. +// CopySources returns the build-context sources of a COPY or ADD instruction, +// in any case (Dockerfile instructions are case-insensitive). Pass it a +// logical instruction (Instructions), not a physical line. // // Leading flags are stripped rather than abandoning the line. Skipping any // flagged COPY would leave `COPY --chown=…` and `COPY --link` — both ordinary // context reads — unread, so their sources would escape every check. -func CopySources(line string) ([]string, CopyKind) { - fields := strings.Fields(line) - if len(fields) == 0 || fields[0] != "COPY" { +func CopySources(instruction string) ([]string, CopyKind) { + fields := strings.Fields(instruction) + if len(fields) == 0 { + return nil, NotCopy + } + command := strings.ToUpper(fields[0]) + if command != "COPY" && command != "ADD" { return nil, NotCopy } operands := fields[1:] @@ -148,12 +197,33 @@ func CopySources(line string) ([]string, CopyKind) { if len(operands) < 2 || strings.HasPrefix(operands[0], "[") { return nil, Unparsed } - return operands[:len(operands)-1], FromContext + var sources []string + for _, source := range operands[:len(operands)-1] { + if strings.HasPrefix(source, "<<") { + continue // a heredoc: its content is in the Dockerfile itself + } + if command == "ADD" && remoteSource(source) { + continue + } + sources = append(sources, source) + } + return sources, FromContext +} + +// remoteSource reports whether an ADD source is fetched rather than read from +// the build context. +func remoteSource(source string) bool { + for _, prefix := range []string{"http://", "https://", "git@", "git://", "ssh://"} { + if strings.HasPrefix(source, prefix) { + return true + } + } + return false } // Inputs returns the repository-root-relative POSIX paths, read from the // checkout at root, that this companion's image is built from in this -// repository: its Dockerfile and every build-context COPY source, a file or a +// repository: its Dockerfile and every build-context COPY or ADD source, a file or a // directory. The staged CLI binary is not one: it is built from codefly-dev/cli // at publish time. A change under any input changes the image, so it needs a // new Version: publishing skips a tag the registry already has. @@ -163,11 +233,11 @@ func (s BuildSpec) Inputs(root string) ([]string, error) { return nil, fmt.Errorf("cannot read the <%s> Dockerfile: %w", s.Name, err) } inputs := []string{s.Dockerfile} - for number, line := range strings.Split(string(content), "\n") { - sources, kind := CopySources(line) + for _, instruction := range Instructions(string(content)) { + sources, kind := CopySources(instruction.Text) switch kind { case Unparsed: - return nil, fmt.Errorf("%s:%d is a COPY whose sources cannot be read: %s", s.Dockerfile, number+1, strings.TrimSpace(line)) + return nil, fmt.Errorf("%s:%d is a COPY or ADD whose sources cannot be read: %s", s.Dockerfile, instruction.Line, strings.TrimSpace(instruction.Text)) case FromContext: for _, source := range sources { if s.CLIBinary != "" && source == s.CLIBinary { diff --git a/companions/buildspec_test.go b/companions/buildspec_test.go index 13f5297d2..82e28257f 100644 --- a/companions/buildspec_test.go +++ b/companions/buildspec_test.go @@ -216,6 +216,18 @@ func TestBuildSpecsMatchWhatTheCurrentPublisherApplies(t *testing.T) { } } +// A COPY spread over continued lines is one instruction: its later sources +// are inputs too, and a comment inside it is not part of it. +func TestInstructionsJoinContinuedLines(t *testing.T) { + dockerfile := "FROM alpine\n# a comment\nCOPY a \\\n # inside\n b /dst/\nRUN true\n" + instructions := companions.Instructions(dockerfile) + require.Len(t, instructions, 3) + require.Equal(t, 3, instructions[1].Line) + sources, kind := companions.CopySources(instructions[1].Text) + require.Equal(t, companions.FromContext, kind) + require.Equal(t, []string{"a", "b"}, sources) +} + func TestCopySourcesReadsEveryContextCopyShape(t *testing.T) { for line, expected := range map[string]struct { sources []string @@ -229,6 +241,11 @@ func TestCopySourcesReadsEveryContextCopyShape(t *testing.T) { "COPY --from=builder /go/bin/buf /usr/local/": {nil, companions.FromStage}, "RUN apk add --no-cache bash": {nil, companions.NotCopy}, "": {nil, companions.NotCopy}, + "copy facades/py /opt": {[]string{"facades/py"}, companions.FromContext}, + "ADD entrypoint.sh /usr/local/bin/": {[]string{"entrypoint.sh"}, companions.FromContext}, + "add --chown=1 a https://example.com/x.tgz /d/": {[]string{"a"}, companions.FromContext}, + "ADD git@github.com:o/r.git /src": {nil, companions.FromContext}, + "COPY < Date: Sun, 25 Oct 2026 21:15:17 -0400 Subject: [PATCH 3/3] fix(ciguard): Companion version is in the documented set of checks safe to require companion-version.yml runs on every pull request with a fixed job name, so ciguard derives it as safe to require; the pinned set and the branch-protection runbook's apply payload name it too. Co-Authored-By: Claude Opus 5.5 --- docs/runbooks/branch-protection.md | 12 ++++++++---- internal/ciguard/branchprotection_test.go | 1 + 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/docs/runbooks/branch-protection.md b/docs/runbooks/branch-protection.md index 07c9c3151..f7a0e00a6 100644 --- a/docs/runbooks/branch-protection.md +++ b/docs/runbooks/branch-protection.md @@ -36,9 +36,9 @@ Queried, not inferred. Ruleset 24397257, `target: branch`, - **`required_status_checks`** — `Build`, `Proto`, `Registry cache cold (go)`, `Registry cache cold (next)`, `Registry cache clean runner (go)`, - `Registry cache clean runner (next)`. Six, not the seven below: + `Registry cache clean runner (next)`. Six, not the eight below: `pnpm source evidence` is derived as safe to require and is **not** in the - live ruleset. + live ruleset; `Companion version` joins it once #838 merges. - **`strict_required_status_checks_policy: false`** — a pull request that went green against an older `main` can land without being retested. The `"strict": true` recommended below is not what is applied. @@ -79,6 +79,7 @@ if you require them. ``` Build +Companion version Proto pnpm source evidence Registry cache cold (go) @@ -92,10 +93,12 @@ coverage, `govulncheck`, and the version/tag and CGO-free guards. It is the check #447 was red on. `Proto` is the same workflow's `buf breaking` gate on `proto/`, split out so a BSR hiccup and a schema break answer separately. `pnpm source evidence` runs the real lockfile audit and inventory tests with the -pinned pnpm toolchain. The other four are `build-cache.yml`'s registry +pinned pnpm toolchain. `Companion version` (`companion-version.yml`) refuses a +change to a companion's image inputs that keeps its version, so a published tag +never names two images. The other four are `build-cache.yml`'s registry cache-conformance matrix. -All seven qualify on the same three grounds, which are what make a required check +All eight qualify on the same three grounds, which are what make a required check safe rather than merely desirable: - **Their workflow runs on every pull request to `main`** — `on.pull_request` @@ -143,6 +146,7 @@ gh api -X PUT repos/codefly-dev/core/rulesets/24397257 --input - <<'JSON' "do_not_enforce_on_create": false, "required_status_checks": [ {"context": "Build"}, + {"context": "Companion version"}, {"context": "Proto"}, {"context": "pnpm source evidence"}, {"context": "Registry cache cold (go)"}, diff --git a/internal/ciguard/branchprotection_test.go b/internal/ciguard/branchprotection_test.go index fe88a02ba..9cc2eb4f2 100644 --- a/internal/ciguard/branchprotection_test.go +++ b/internal/ciguard/branchprotection_test.go @@ -34,6 +34,7 @@ import ( // docs/runbooks/branch-protection.md tells an operator to apply. var requiredChecks = []string{ "Build", + "Companion version", "Proto", "pnpm source evidence", "Registry cache clean runner (go)",