Refactor expression helper semantics into a single workflow source and remove duplicate CLI version state

pkg/cli/version.go

@@ -2,23 +2,12 @@ package cli
 
 import "github.com/github/gh-aw/pkg/workflow"
 
-// Package-level version information
-var (
-	version = "dev"
-)
-
-func init() {
-	// Set the version in the workflow package so NewCompiler() auto-detects it
-	workflow.SetVersion(version)
-}
-
-// SetVersionInfo sets the version information for the CLI and workflow package
+// SetVersionInfo sets version information for the workflow package.
 func SetVersionInfo(v string) {
-	version = v
-	workflow.SetVersion(v) // Keep workflow package in sync
+	workflow.SetVersion(v)
 }
 
-// GetVersion returns the current version
+// GetVersion returns the current version.
 func GetVersion() string {
-	return version
+	return workflow.GetVersion()
 }

pkg/cli/version_test.go

@@ -0,0 +1,27 @@
+//go:build !integration
+
+package cli
+
+import (
+	"testing"
+
+	"github.com/github/gh-aw/pkg/workflow"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGetVersionDelegatesToWorkflowVersion(t *testing.T) {
+	originalVersion := workflow.GetVersion()
+	defer workflow.SetVersion(originalVersion)
+
+	workflow.SetVersion("workflow-direct")
+	assert.Equal(t, "workflow-direct", GetVersion(), "cli.GetVersion should read workflow version")
+}
+
+func TestSetVersionInfoUpdatesWorkflowVersion(t *testing.T) {
+	originalVersion := workflow.GetVersion()
+	defer workflow.SetVersion(originalVersion)
+
+	SetVersionInfo("set-version-info")
+	assert.Equal(t, "set-version-info", workflow.GetVersion(), "SetVersionInfo should set workflow version")
+	assert.Equal(t, "set-version-info", GetVersion(), "cli.GetVersion should return workflow version")
+}

pkg/workflow/dispatch_repository_validation.go

@@ -65,7 +65,7 @@ func (c *Compiler) validateDispatchRepository(data *WorkflowData, workflowPath s
 		}
 
 		// Validate single repository format (skip if it looks like a GitHub Actions expression)
-		if hasRepository && !isGitHubActionsExpression(tool.Repository) {
+		if hasRepository && !hasExpressionMarker(tool.Repository) {
 			if !repoSlugPattern.MatchString(tool.Repository) {
 				repoFmtErr := fmt.Errorf("dispatch_repository: tool %q has invalid 'repository' format %q (expected 'owner/repo')", toolKey, tool.Repository)
 				if returnErr := collector.Add(repoFmtErr); returnErr != nil {
@@ -76,7 +76,7 @@ func (c *Compiler) validateDispatchRepository(data *WorkflowData, workflowPath s
 
 		// Validate allowed_repositories format
 		for _, repo := range tool.AllowedRepositories {
-			if isGitHubActionsExpression(repo) {
+			if hasExpressionMarker(repo) {
 				continue
 			}
 			// Allow glob patterns like "org/*"
@@ -99,9 +99,3 @@ func (c *Compiler) validateDispatchRepository(data *WorkflowData, workflowPath s
 
 	return collector.FormattedError("dispatch_repository")
 }
-
-// isGitHubActionsExpression returns true if the string contains a GitHub Actions
-// expression syntax (${{ }}), which should not be validated as a static value.
-func isGitHubActionsExpression(value string) bool {
-	return strings.Contains(value, "${{")
-}

pkg/workflow/expression_patterns.go

@@ -58,10 +58,35 @@ package workflow
 
 import (
 	"regexp"
+	"strings"
 
 	"github.com/github/gh-aw/pkg/logger"
 )
 
+// hasExpressionMarker reports whether s contains a GitHub Actions expression opening marker.
+// This is a permissive check used in scenarios where partial expressions should be treated
+// as dynamic values.
+func hasExpressionMarker(s string) bool {
+	return strings.Contains(s, "${{")
+}
+
+// containsExpression reports whether s contains a complete non-empty GitHub Actions expression.
+// A complete expression has a "${{" marker that appears before a closing "}}" marker
+// with at least one character between them.
+func containsExpression(s string) bool {
+	_, afterOpen, found := strings.Cut(s, "${{")
+	if !found {
+		return false
+	}
+	closeIdx := strings.Index(afterOpen, "}}")
+	return closeIdx > 0
+}
+
+// isExpression reports whether the entire string s is a GitHub Actions expression.
+func isExpression(s string) bool {
+	return strings.HasPrefix(s, "${{") && strings.HasSuffix(s, "}}")
+}
+
 var expressionPatternsLog = logger.New("workflow:expression_patterns")
 
 func init() {

pkg/workflow/expression_patterns_test.go

@@ -456,3 +456,66 @@ func TestRangePattern(t *testing.T) {
 		})
 	}
 }
+
+func TestExpressionHelpers(t *testing.T) {
+	tests := []struct {
+		name              string
+		input             string
+		wantHasMarker     bool
+		wantContainsExpr  bool
+		wantWholeExprOnly bool
+	}{
+		{
+			name:              "full expression",
+			input:             "${{ github.actor }}",
+			wantHasMarker:     true,
+			wantContainsExpr:  true,
+			wantWholeExprOnly: true,
+		},
+		{
+			name:              "embedded expression",
+			input:             "prefix ${{ github.actor }} suffix",
+			wantHasMarker:     true,
+			wantContainsExpr:  true,
+			wantWholeExprOnly: false,
+		},
+		{
+			name:              "partial expression marker only",
+			input:             "${{ github.actor",
+			wantHasMarker:     true,
+			wantContainsExpr:  false,
+			wantWholeExprOnly: false,
+		},
+		{
+			name:  "empty expression body",
+			input: "${{}}",
+			// isExpression is a strict wrapper check, while containsExpression
+			// requires a non-empty expression body between markers.
+			wantHasMarker:     true,
+			wantContainsExpr:  false,
+			wantWholeExprOnly: true,
+		},
+		{
+			name:              "wrong marker order",
+			input:             "}} before ${{",
+			wantHasMarker:     true,
+			wantContainsExpr:  false,
+			wantWholeExprOnly: false,
+		},
+		{
+			name:              "no expression",
+			input:             "plain text",
+			wantHasMarker:     false,
+			wantContainsExpr:  false,
+			wantWholeExprOnly: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.wantHasMarker, hasExpressionMarker(tt.input), "hasExpressionMarker result mismatch")
+			assert.Equal(t, tt.wantContainsExpr, containsExpression(tt.input), "containsExpression result mismatch")
+			assert.Equal(t, tt.wantWholeExprOnly, isExpression(tt.input), "isExpression result mismatch")
+		})
+	}
+}

pkg/workflow/expression_safety_validation.go

@@ -226,7 +226,7 @@ func validateSingleExpression(expression string, opts ExpressionValidationOption
 			rightExpr := strings.TrimSpace(orMatch[2])
 
 			leftErr := validateSingleExpression(leftExpr, opts)
-			leftIsSafe := leftErr == nil && !containsExpression(opts.UnauthorizedExpressions, leftExpr)
+			leftIsSafe := leftErr == nil && !containsExpressionInList(opts.UnauthorizedExpressions, leftExpr)
 
 			if leftIsSafe {
 				// Check if right side is a literal string (single, double, or backtick quotes)
@@ -242,7 +242,7 @@ func validateSingleExpression(expression string, opts ExpressionValidationOption
 				} else {
 					// If right side is also a safe expression, recursively check it
 					rightErr := validateSingleExpression(rightExpr, opts)
-					if rightErr == nil && !containsExpression(opts.UnauthorizedExpressions, rightExpr) {
+					if rightErr == nil && !containsExpressionInList(opts.UnauthorizedExpressions, rightExpr) {
 						allowed = true
 					}
 				}
@@ -296,7 +296,7 @@ func validateSingleExpression(expression string, opts ExpressionValidationOption
 	return nil
 }
 
-// containsExpression checks if an expression is in the list
-func containsExpression(list *[]string, expr string) bool {
+// containsExpressionInList checks if an expression is in the list.
+func containsExpressionInList(list *[]string, expr string) bool {
 	return slices.Contains(*list, expr)
 }

pkg/workflow/frontmatter_extraction_metadata.go

@@ -173,7 +173,7 @@ func (c *Compiler) extractToolsTimeout(tools map[string]any) (string, error) {
 		frontmatterMetadataLog.Printf("Extracting tools.timeout value: type=%T", timeoutValue)
 		// Handle GitHub Actions expression strings
 		if strVal, ok := timeoutValue.(string); ok {
-			if isExpressionString(strVal) {
+			if isExpression(strVal) {
 				frontmatterMetadataLog.Printf("Extracted tools.timeout as expression: %s", strVal)
 				return strVal, nil
 			}
@@ -224,7 +224,7 @@ func (c *Compiler) extractToolsStartupTimeout(tools map[string]any) (string, err
 	if timeoutValue, exists := tools["startup-timeout"]; exists {
 		// Handle GitHub Actions expression strings
 		if strVal, ok := timeoutValue.(string); ok {
-			if isExpressionString(strVal) {
+			if isExpression(strVal) {
 				return strVal, nil
 			}
 			return "", fmt.Errorf("tools.startup-timeout must be an integer or a GitHub Actions expression (e.g. '${{ inputs.startup-timeout }}'), got string %q", strVal)

pkg/workflow/safe_outputs_max_validation.go

@@ -64,7 +64,7 @@ func validateSafeOutputsMax(config *SafeOutputsConfig) error {
 		}
 
 		maxPtr, ok := maxField.Interface().(*string)
-		if !ok || maxPtr == nil || isExpressionString(*maxPtr) {
+		if !ok || maxPtr == nil || isExpression(*maxPtr) {
 			continue
 		}
 
@@ -94,7 +94,7 @@ func validateSafeOutputsMax(config *SafeOutputsConfig) error {
 
 		for _, toolName := range sortedToolNames {
 			tool := config.DispatchRepository.Tools[toolName]
-			if tool == nil || tool.Max == nil || isExpressionString(*tool.Max) {
+			if tool == nil || tool.Max == nil || isExpression(*tool.Max) {
 				continue
 			}
 

pkg/workflow/safe_outputs_target_validation_test.go

@@ -374,7 +374,7 @@ func TestValidateTargetValue(t *testing.T) {
 	}
 }
 
-func TestIsGitHubExpression(t *testing.T) {
+func TestContainsExpressionForTargetValidation(t *testing.T) {
 	tests := []struct {
 		name string
 		s    string
@@ -434,9 +434,9 @@ func TestIsGitHubExpression(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := isGitHubExpression(tt.s)
+			got := containsExpression(tt.s)
 			if got != tt.want {
-				t.Errorf("isGitHubExpression() = %v, want %v", got, tt.want)
+				t.Errorf("containsExpression() = %v, want %v", got, tt.want)
 			}
 		})
 	}

pkg/workflow/safe_outputs_validation.go

@@ -153,7 +153,7 @@ func validateTargetValue(configName, target string) error {
 	}
 
 	// Check if it's a GitHub Actions expression
-	if isGitHubExpression(target) {
+	if containsExpression(target) {
 		safeOutputsTargetValidationLog.Printf("Target for %s is a GitHub Actions expression", configName)
 		return nil
 	}
@@ -179,23 +179,6 @@ func validateTargetValue(configName, target string) error {
 	)
 }
 
-// isGitHubExpression checks if a string is a valid GitHub Actions expression
-// A valid expression must have properly balanced ${{ and }} markers
-func isGitHubExpression(s string) bool {
-	// Must contain both opening and closing markers
-	if !strings.Contains(s, "${{") || !strings.Contains(s, "}}") {
-		return false
-	}
-
-	// Basic validation: opening marker must come before closing marker
-	openIndex := strings.Index(s, "${{")
-	closeIndex := strings.Index(s, "}}")
-
-	// The closing marker must come after the opening marker
-	// and there must be something between them
-	return openIndex >= 0 && closeIndex > openIndex+3
-}
-
 var safeOutputsAllowWorkflowsValidationLog = newValidationLogger("safe_outputs_allow_workflows")
 
 // validateSafeOutputsAllowWorkflows validates that allow-workflows: true requires

pkg/workflow/shell.go

@@ -31,7 +31,7 @@ func shellEscapeArg(arg string) string {
 	// so single-quoting would mangle the expression syntax (e.g., 'staging' inside
 	// ${{ env.X == 'staging' }} becomes '\''staging'\'' which GA cannot parse).
 	// Double-quoting preserves the expression for GA evaluation.
-	if containsGitHubActionsExpression(arg) {
+	if containsExpression(arg) {
 		shellLog.Print("Argument contains GitHub Actions expression, using double-quote wrapping")
 		escaped := strings.ReplaceAll(arg, `"`, `\"`)
 		return `"` + escaped + `"`
@@ -49,16 +49,6 @@ func shellEscapeArg(arg string) string {
 	return arg
 }
 
-// containsGitHubActionsExpression checks if a string contains GitHub Actions
-// expressions (${{ ... }}). It verifies that ${{ appears before }}.
-func containsGitHubActionsExpression(s string) bool {
-	openIdx := strings.Index(s, "${{")
-	if openIdx < 0 {
-		return false
-	}
-	return strings.Contains(s[openIdx:], "}}")
-}
-
 // buildDockerCommandWithExpandableVars builds a properly quoted docker command
 // that allows ${VAR_NAME} variables to be expanded at runtime.
 func buildDockerCommandWithExpandableVars(cmd string) string {