fix: coerce every expression value kind to a string and drop the format() panic (#1135)

`coerceToString` covered only a few kinds and returned the unconverted `reflect.Value` for the rest, so callers rendered contexts as `<*model.GithubContext Value>` and mangled sized integers and floats the same way. It now returns a string, which makes that placeholder unrepresentable, and is exported as `CoerceToString` so Gitea can drop its own diverging copy. It also takes an already reflected value, so internal callers need no conversion and no guard against the zero `Value`.

`format()` panicked whenever a lone `}` was followed by anything other than another `}`. Only a trailing `}` reached the existing unmatched-brace check, so an expression such as `format('a}b')`, which any workflow can write, took the process down instead. It now returns that same error.

Reviewed-on: https://gitea.com/gitea/runner/pulls/1135
Reviewed-by: Lunny Xiao <[email protected]>
Co-authored-by: silverwind <[email protected]>

.golangci.yml

@@ -37,12 +37,8 @@ linters:
       rules:
         main:
           deny:
-            - pkg: io/ioutil
-              desc: use os or io instead
             - pkg: golang.org/x/exp
               desc: it's experimental and unreliable
-            - pkg: github.com/pkg/errors
-              desc: use builtin errors package instead
     nolintlint:
       allow-unused: false
       require-explanation: true

AGENTS.md

@@ -1,10 +1,18 @@
+- Never assume, verify before claiming
 - Use `make help` to find available development targets
-- Run `make fmt` to format `.go` files, and run `make lint-go` to lint them
-- Run `make tidy` after any `go.mod` changes
-- Run single go unit tests with `go test -run '^TestName$' ./modulepath/`
+- PR descriptions: minimal, only what and why, no task lists or file listings
+- Reference issues and PRs by full URL, not by number
+- Use Conventional Commits for commit messages and PR titles, plus the `enhance` type for user-facing enhancements
+- Add an `Assisted-by: AGENT_NAME:MODEL_VERSION` trailer to commit messages, never `Co-Authored-By` or `Signed-off-by`
+- Attribute agent authorship on one trailing line in issue and pull request comments, never as a PR description section
+- Never force-push, amend, or squash unless asked. Use new commits and normal push for pull request updates
+- Comments: write almost none, short and preferably same-line, explaining why for a future reader. Never narrate code, the change or the prompt. Preserve existing ones that still apply
 - Add the current year into the copyright header of new `.go` files
 - Ensure no trailing whitespace in edited files
-- Never force-push, amend, or squash unless asked. Use new commits and normal push for pull request updates
-- Preserve existing code comments, do not remove or rewrite comments that are still relevant
-- Include authorship attribution in issue and pull request comments
-- Add `Co-Authored-By` lines to all commits, indicating name and model used
+- Run `make fmt` after `.go` edits, `make tidy` after `go.mod` edits, and `make checks` for the non-lint source checks
+- Lint what changed with `make lint-go`, and `make lint-go-windows` for Windows and platform-split files
+- Fix the cause rather than disabling a linter or weakening a test. Where unavoidable, use the narrowest scope with a trailing comment giving the reason
+- Run single go tests with `go test -run '^TestName$' ./modulepath/`. `make test` self-skips the integration tests without docker or network, `make test-dind` runs the daemon-facing tests against the built dind image
+- Write the fewest, fastest tests covering the behavior, extending an existing one where possible. Prefer unit tests where logic is testable in isolation
+- Wait on a deterministic condition rather than `sleep`
+- Update the files under `docs/` when behavior documented there changes

act/container/docker_cli.go

@@ -4,8 +4,10 @@
 
 //go:build !(WITHOUT_DOCKER || !(linux || darwin || windows || netbsd))
 
-// This file is exact copy of https://github.com/docker/cli/blob/9a471180cb7d39c236d090399a9d362c3f5a8ebd/cli/command/container/opts.go
-// appended with license information.
+// This file is exact copy of https://github.com/docker/cli/blob/dfc4efb1e2ab8c06d70d2a1366ad448d2f917e90/cli/command/container/opts.go with:
+// * appended with license information
+// * regexp and loader.ParseVolume in place of the import-restricted internal/lazyregexp and internal/volumespec
+// * invalidParameter from the package's errors.go, and convertPortSet/convertPortMap for the callers in docker_run.go
 //
 // docker/cli is licensed under the Apache License, Version 2.0.
 // See DOCKER_LICENSE for the full license text.
@@ -30,6 +32,7 @@ import (
 	"strings"
 	"time"
 
+	cerrdefs "github.com/containerd/errdefs"
 	"github.com/docker/cli/cli/compose/loader"
 	"github.com/docker/cli/opts"
 	"github.com/docker/go-connections/nat"
@@ -380,7 +383,7 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con
 	var binds []string
 	volumes := copts.volumes.GetMap()
 	// add any bind targets to the list of container volumes
-	for bind := range copts.volumes.GetMap() {
+	for bind := range volumes {
 		parsed, err := loader.ParseVolume(bind)
 		if err != nil {
 			return nil, err
@@ -515,13 +518,13 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con
 	// collect all the environment variables for the container
 	envVariables, err := opts.ReadKVEnvStrings(copts.envFile.GetSlice(), copts.env.GetSlice())
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("--env-file: %w", err)
 	}
 
 	// collect all the labels for the container
 	labels, err := opts.ReadKVStrings(copts.labelsFile.GetSlice(), copts.labels.GetSlice())
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("--label-file: %w", err)
 	}
 
 	pidMode := container.PidMode(copts.pidMode)
@@ -1164,16 +1167,17 @@ func toNetipAddrSlice(ips []string) []netip.Addr {
 }
 
 // invalidParameter wraps an error to indicate it was caused by invalid input.
-// This is a local replacement for docker/docker/errdefs.InvalidParameter.
-type invalidParameterError struct{ error }
+// This is a copy of docker/cli's cli/command/container/errors.go, which is not importable.
+type invalidParameterErr struct{ error }
 
-func (e invalidParameterError) InvalidParameter() {}
+func (invalidParameterErr) InvalidParameter() {}
+func (e invalidParameterErr) Unwrap() error   { return e.error }
 
 func invalidParameter(err error) error {
-	if err == nil {
-		return nil
+	if err == nil || cerrdefs.IsInvalidArgument(err) {
+		return err
 	}
-	return invalidParameterError{err}
+	return invalidParameterErr{err}
 }
 
 func convertPortSet(ports nat.PortSet) (network.PortSet, error) {

act/container/docker_run.go

@@ -27,7 +27,6 @@ import (
 	"gitea.com/gitea/runner/act/filecollector"
 
 	"dario.cat/mergo"
-	"github.com/Masterminds/semver"
 	cerrdefs "github.com/containerd/errdefs"
 	"github.com/docker/cli/cli/compose/loader"
 	"github.com/docker/cli/cli/connhelper"
@@ -42,6 +41,7 @@ import (
 	"github.com/moby/moby/api/types/network"
 	"github.com/moby/moby/api/types/system"
 	"github.com/moby/moby/client"
+	"github.com/moby/moby/client/pkg/versions"
 	specs "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/sirupsen/logrus"
 )
@@ -92,19 +92,11 @@ func (cr *containerReference) connectToNetwork(name string, aliases []string) co
 // supportsContainerImagePlatform returns true if the underlying Docker server
 // API version is 1.41 and beyond
 func supportsContainerImagePlatform(ctx context.Context, cli client.APIClient) bool {
-	logger := common.Logger(ctx)
 	ver, err := cli.ServerVersion(ctx, client.ServerVersionOptions{})
 	if err != nil {
-		logger.Panicf("Failed to get Docker API Version: %s", err)
-		return false
+		common.Logger(ctx).Panicf("Failed to get Docker API Version: %s", err)
 	}
-	sv, err := semver.NewVersion(ver.APIVersion)
-	if err != nil {
-		logger.Panicf("Failed to unmarshal Docker Version: %s", err)
-		return false
-	}
-	constraint, _ := semver.NewConstraint(">= 1.41")
-	return constraint.Check(sv)
+	return versions.GreaterThanOrEqualTo(ver.APIVersion, "1.41")
 }
 
 func (cr *containerReference) Create(capAdd, capDrop []string) common.Executor {
@@ -588,7 +580,7 @@ func (cr *containerReference) create(capAdd, capDrop []string) common.Executor {
 		}
 
 		var platSpecs *specs.Platform
-		if supportsContainerImagePlatform(ctx, cr.cli) && cr.input.Platform != "" {
+		if cr.input.Platform != "" && supportsContainerImagePlatform(ctx, cr.cli) {
 			platSpecs, err = parsePlatform(cr.input.Platform)
 			if err != nil {
 				return err

act/container/docker_stub.go

@@ -8,13 +8,13 @@ package container
 
 import (
 	"context"
+	"errors"
 	"runtime"
 	"time"
 
 	"gitea.com/gitea/runner/act/common"
 
 	"github.com/moby/moby/api/types/system"
-	"github.com/pkg/errors"
 )
 
 // ImageExistsLocally returns a boolean indicating if an image with the

act/exprparser/functions.go

@@ -28,8 +28,8 @@ func (impl *interperterImpl) contains(search, item reflect.Value) (bool, error)
 	switch search.Kind() {
 	case reflect.String, reflect.Int, reflect.Float64, reflect.Bool, reflect.Invalid:
 		return strings.Contains(
-			strings.ToLower(impl.coerceToString(search).String()),
-			strings.ToLower(impl.coerceToString(item).String()),
+			strings.ToLower(CoerceToString(search)),
+			strings.ToLower(CoerceToString(item)),
 		), nil
 
 	case reflect.Slice:
@@ -51,15 +51,15 @@ func (impl *interperterImpl) contains(search, item reflect.Value) (bool, error)
 
 func (impl *interperterImpl) startsWith(searchString, searchValue reflect.Value) (bool, error) { //nolint:unparam // pre-existing issue from nektos/act
 	return strings.HasPrefix(
-		strings.ToLower(impl.coerceToString(searchString).String()),
-		strings.ToLower(impl.coerceToString(searchValue).String()),
+		strings.ToLower(CoerceToString(searchString)),
+		strings.ToLower(CoerceToString(searchValue)),
 	), nil
 }
 
 func (impl *interperterImpl) endsWith(searchString, searchValue reflect.Value) (bool, error) { //nolint:unparam // pre-existing issue from nektos/act
 	return strings.HasSuffix(
-		strings.ToLower(impl.coerceToString(searchString).String()),
-		strings.ToLower(impl.coerceToString(searchValue).String()),
+		strings.ToLower(CoerceToString(searchString)),
+		strings.ToLower(CoerceToString(searchValue)),
 	), nil
 }
 
@@ -70,7 +70,7 @@ const (
 )
 
 func (impl *interperterImpl) format(str reflect.Value, replaceValue ...reflect.Value) (string, error) {
-	input := impl.coerceToString(str).String()
+	input := CoerceToString(str)
 	var output strings.Builder
 	replacementIndex := ""
 
@@ -108,7 +108,7 @@ func (impl *interperterImpl) format(str reflect.Value, replaceValue ...reflect.V
 					return "", fmt.Errorf("The following format string references more arguments than were supplied: '%s'", input)
 				}
 
-				output.WriteString(impl.coerceToString(replaceValue[index]).String())
+				output.WriteString(CoerceToString(replaceValue[index]))
 
 				state = passThrough
 
@@ -124,7 +124,7 @@ func (impl *interperterImpl) format(str reflect.Value, replaceValue ...reflect.V
 				state = passThrough
 
 			default:
-				panic("Invalid format parser state")
+				return "", fmt.Errorf("Closing bracket without opening one. The following format string is invalid: '%s'", input)
 			}
 		}
 	}
@@ -143,17 +143,17 @@ func (impl *interperterImpl) format(str reflect.Value, replaceValue ...reflect.V
 }
 
 func (impl *interperterImpl) join(array, sep reflect.Value) (string, error) { //nolint:unparam // pre-existing issue from nektos/act
-	separator := impl.coerceToString(sep).String()
+	separator := CoerceToString(sep)
 	switch array.Kind() {
 	case reflect.Slice:
 		var items []string
 		for i := 0; i < array.Len(); i++ {
-			items = append(items, impl.coerceToString(array.Index(i).Elem()).String())
+			items = append(items, CoerceToString(array.Index(i)))
 		}
 
 		return strings.Join(items, separator), nil
 	default:
-		return strings.Join([]string{impl.coerceToString(array).String()}, separator), nil
+		return strings.Join([]string{CoerceToString(array)}, separator), nil
 	}
 }
 

act/exprparser/functions_test.go

@@ -121,6 +121,7 @@ func TestFunctionJoin(t *testing.T) {
 		{"join(fromJSON('[\"a\", \"b\", null]'), null)", "ab", "join-number"},
 		{"join(fromJSON('[\"a\", \"b\"]'))", "a,b", "join-number"},
 		{"join(fromJSON('[\"a\", \"b\", null]'), 1)", "a1b1", "join-number"},
+		{"join(fromJSON('[1, true, null]'), '-')", "1-true-", "join-mixed-types"},
 	}
 
 	env := &EvaluationEnvironment{}
@@ -230,8 +231,10 @@ func TestFunctionFormat(t *testing.T) {
 		{`format('Hello "{0}" {1} {2} {3} {4}', null, true, -3.14, NaN, Infinity)`, `Hello "" true -3.14 NaN Infinity`, nil, "format-with-primitives"},
 		{`format('Hello "{0}" {1} {2}', fromJSON('[0, true, "abc"]'), fromJSON('[{"a":1}]'), fromJSON('{"a":{"b":1}}'))`, `Hello "Array" Array Object`, nil, "format-with-complex-types"},
 		{"format(true)", "true", nil, "format-with-primitive-args"},
+		{"format('{0}', github)", "Object", nil, "format-with-context"},
 		{"format('echo Hello {0} ${{Test}}', github.undefined_property)", "echo Hello  ${Test}", nil, "format-with-undefined-value"},
 		{"format('{0}}', '{1}', 'World')", nil, "Closing bracket without opening one. The following format string is invalid: '{0}}'", "format-invalid-format-string"},
+		{"format('a}b')", nil, "Closing bracket without opening one. The following format string is invalid: 'a}b'", "format-unmatched-closing-brace"},
 		{"format('{0', '{1}', 'World')", nil, "Unclosed brackets. The following format string is invalid: '{0'", "format-invalid-format-string"},
 		{"format('{2}', '{1}', 'World')", "", "The following format string references more arguments than were supplied: '{2}'", "format-invalid-replacement-reference"},
 		{"format('{2147483648}')", "", "The following format string is invalid: '{2147483648}'", "format-invalid-replacement-reference"},

act/exprparser/interpreter.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"math"
 	"reflect"
+	"strconv"
 	"strings"
 
 	"gitea.com/gitea/runner/act/model"
@@ -429,41 +430,54 @@ func (impl *interperterImpl) coerceToNumber(value reflect.Value) reflect.Value {
 	return reflect.ValueOf(math.NaN())
 }
 
-func (impl *interperterImpl) coerceToString(value reflect.Value) reflect.Value {
-	switch value.Kind() {
-	case reflect.Invalid:
-		return reflect.ValueOf("")
-
-	case reflect.Bool:
-		switch value.Bool() {
-		case true:
-			return reflect.ValueOf("true")
-		case false:
-			return reflect.ValueOf("false")
-		}
-
-	case reflect.String:
-		return value
-
-	case reflect.Int:
-		return reflect.ValueOf(fmt.Sprint(value))
-
-	case reflect.Float64:
-		if math.IsInf(value.Float(), 1) {
-			return reflect.ValueOf("Infinity")
-		} else if math.IsInf(value.Float(), -1) {
-			return reflect.ValueOf("-Infinity")
-		}
-		return reflect.ValueOf(fmt.Sprintf("%.15G", value.Float()))
-
-	case reflect.Slice:
-		return reflect.ValueOf("Array")
-
-	case reflect.Map:
-		return reflect.ValueOf("Object")
+// CoerceToString converts an evaluated expression value to a string the way GitHub does,
+// see https://docs.github.com/en/actions/reference/workflows-and-actions/expressions#operators
+// An already reflected value is accepted as-is, since Interface() would panic on an invalid one.
+func CoerceToString(v any) string {
+	value, ok := v.(reflect.Value)
+	if !ok {
+		value = reflect.ValueOf(v)
 	}
 
-	return value
+	switch value.Kind() {
+	case reflect.Invalid:
+		return ""
+
+	case reflect.Bool:
+		return strconv.FormatBool(value.Bool())
+
+	case reflect.String:
+		return value.String()
+
+	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
+		return strconv.FormatInt(value.Int(), 10)
+
+	case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
+		return strconv.FormatUint(value.Uint(), 10)
+
+	case reflect.Float32, reflect.Float64:
+		if math.IsInf(value.Float(), 1) {
+			return "Infinity"
+		} else if math.IsInf(value.Float(), -1) {
+			return "-Infinity"
+		}
+		return fmt.Sprintf("%.15G", value.Float())
+
+	case reflect.Slice, reflect.Array:
+		return "Array"
+
+	// contexts such as `github` are pointers to structs, so they stringify as objects too
+	case reflect.Map, reflect.Struct:
+		return "Object"
+
+	case reflect.Interface, reflect.Pointer:
+		if value.IsNil() {
+			return ""
+		}
+		return CoerceToString(value.Elem())
+	}
+
+	return fmt.Sprintf("%v", value)
 }
 
 func (impl *interperterImpl) compareString(left, right string, kind actionlint.CompareOpNodeKind) (bool, error) {

act/exprparser/interpreter_test.go

@@ -6,6 +6,7 @@ package exprparser
 
 import (
 	"math"
+	"reflect"
 	"testing"
 
 	"gitea.com/gitea/runner/act/model"
@@ -633,3 +634,51 @@ func TestContexts(t *testing.T) {
 		})
 	}
 }
+
+func TestCoerceToString(t *testing.T) {
+	type object struct{ Name string }
+	obj := object{Name: "x"}
+	var nilPointer *object
+	var nilMap map[string]any
+	var nilSlice []any
+
+	table := []struct {
+		input    any
+		expected string
+		name     string
+	}{
+		{nil, "", "null"},
+		{true, "true", "true"},
+		{false, "false", "false"},
+		{"foo", "foo", "string"},
+		{"", "", "empty-string"},
+		{123, "123", "int"},
+		{int64(-9), "-9", "int64"},
+		{uint8(7), "7", "uint8"},
+		{1.0, "1", "float-integral"},
+		{-9.7, "-9.7", "float"},
+		{2.99e-2, "0.0299", "float-exponential"},
+		{1e21, "1E+21", "float-large"},
+		{float32(1.5), "1.5", "float32"},
+		{math.NaN(), "NaN", "nan"},
+		{math.Inf(1), "Infinity", "positive-infinity"},
+		{math.Inf(-1), "-Infinity", "negative-infinity"},
+		{[]any{1, 2}, "Array", "slice"},
+		{nilSlice, "Array", "nil-slice"},
+		{[2]int{1, 2}, "Array", "fixed-size-array"},
+		{map[string]any{"a": 1}, "Object", "map"},
+		{nilMap, "Object", "nil-map"},
+		{obj, "Object", "struct"},
+		{&obj, "Object", "pointer-to-struct"},
+		{nilPointer, "", "nil-pointer"},
+		{&model.GithubContext{Action: "push"}, "Object", "github-context"},
+		{reflect.ValueOf(42), "42", "reflected-value"},
+		{reflect.Value{}, "", "invalid-reflected-value"},
+	}
+
+	for _, tt := range table {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, CoerceToString(tt.input))
+		})
+	}
+}

go.mod

@@ -6,7 +6,6 @@ require (
 	connectrpc.com/connect v1.20.0
 	dario.cat/mergo v1.0.2
 	gitea.dev/actions-proto-go v0.6.0
-	github.com/Masterminds/semver v1.5.0
 	github.com/avast/retry-go/v5 v5.0.0
 	github.com/containerd/errdefs v1.0.0
 	github.com/creack/pty v1.1.24
@@ -27,7 +26,6 @@ require (
 	github.com/moby/patternmatcher v0.6.1
 	github.com/opencontainers/image-spec v1.1.1
 	github.com/opencontainers/selinux v1.15.1
-	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.24.0
 	github.com/prometheus/client_model v0.6.2
 	github.com/rhysd/actionlint v1.7.12

go.sum

@@ -8,8 +8,6 @@ gitea.dev/actions-proto-go v0.6.0 h1:gjllYQ5vmwlkqOeofTQu5qKTZpmf7kWsafoHvoPCSzY
 gitea.dev/actions-proto-go v0.6.0/go.mod h1:p4RX+D9oqiEEzzkPMXscw2CmaGuYFPWFc6xIOmDNDqs=
 github.com/AdaLogics/go-fuzz-headers v0.0.0-20240806141605-e8a1dd7889d6 h1:He8afgbRMd7mFxO99hRNu+6tazq8nFF9lIwo9JFroBk=
 github.com/AdaLogics/go-fuzz-headers v0.0.0-20240806141605-e8a1dd7889d6/go.mod h1:8o94RPi1/7XTJvwPpRSzSUedZrtlirdB3r9Z20bi2f8=
-github.com/Masterminds/semver v1.5.0 h1:H65muMkzWKEuNDnfl9d70GUjFniHKHRbFPGBuZ3QEww=
-github.com/Masterminds/semver v1.5.0/go.mod h1:MB6lktGJrhw8PrUyiEoblNEGEQ+RzHPF078ddwwvV3Y=
 github.com/Microsoft/go-winio v0.5.2/go.mod h1:WpS1mjBmmwHBEWmogvA2mj8546UReBk4v8QkMxJ6pZY=
 github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY=
 github.com/Microsoft/go-winio v0.6.2/go.mod h1:yd8OoFMLzJbo9gZq8j5qaps8bJ9aShtEA8Ipt1oGCvU=