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

docs/adr/27198-consolidate-expression-helpers-and-unify-cli-version-state.md

@@ -0,0 +1,79 @@
+# ADR-27198: Consolidate Expression Helpers and Unify CLI Version State
+
+**Date**: 2026-04-19
+**Status**: Draft
+**Deciders**: pelikhan, Copilot
+
+---
+
+## Part 1 — Narrative (Human-Friendly)
+
+### Context
+
+The `pkg/workflow` package accumulated near-duplicate expression-detection helpers across multiple files (`dispatch_repository_validation.go`, `shell.go`, `safe_outputs_validation.go`, `templatables.go`). Each file defined its own local boolean helpers for checking GitHub Actions `${{ ... }}` expressions, with subtly different semantics (permissive marker checks vs. full containment checks vs. whole-string checks). Separately, `pkg/cli` maintained its own local `version` variable and an `init()` synchronization path that had to be manually kept in sync with the canonical version state in `pkg/workflow`. Both patterns violated the single-source-of-truth principle and created silent divergence risks.
+
+### Decision
+
+We will centralize the three expression-detection predicates (`hasExpressionMarker`, `containsExpression`, `isExpression`) into `pkg/workflow/expression_patterns.go` and remove all per-file duplicates. We will also remove the local version variable and `init()` in `pkg/cli/version.go`, making `cli.GetVersion()` and `cli.SetVersionInfo()` thin delegators to the canonical `workflow.GetVersion()` / `workflow.SetVersion()` functions. Both changes enforce a single source of truth within their respective domains, eliminating the risk of semantic drift between helper copies.
+
+### Alternatives Considered
+
+#### Alternative 1: Keep per-file helpers but add linting rules
+
+Each file could retain its own inline helper, and a custom linter or `grep`-based CI check could be added to flag divergence. This approach avoids touching call sites and keeps the helpers co-located with their consumers. It was rejected because linting checks would detect drift only after it happens rather than preventing it structurally, and the helpers are small enough that centralization adds no meaningful indirection.
+
+#### Alternative 2: Introduce a separate `expressionutil` sub-package
+
+A new package (e.g., `pkg/expressionutil`) could expose the helpers, providing a fully independent import path. This was considered for its explicit package boundary, but rejected because the helpers are only used within `pkg/workflow` call sites, making an external package an overengineered abstraction. Keeping the helpers unexported (`hasExpressionMarker`, `containsExpression`, `isExpression`) in `expression_patterns.go` preserves encapsulation without adding an unnecessary dependency edge.
+
+#### Alternative 3: Retain the dual version state with better documentation
+
+The `pkg/cli` version variable could have been kept but annotated with comments requiring manual sync with `workflow.GetVersion()`. This was rejected because documentation alone cannot enforce invariants. The actual synchronization logic (`init()` and local `version` variable) was already causing confusion; removing the indirection entirely is safer and simpler.
+
+### Consequences
+
+#### Positive
+- Expression-check semantics are defined once; all call sites share the same behavior, eliminating silent divergence.
+- Version state has a single owner (`pkg/workflow`); the CLI layer is a transparent pass-through with no independent state to maintain.
+- Reduced surface area for future contributors who previously had to know which file's helper was the "correct" one.
+- Test coverage can now target the helpers directly in `expression_patterns_test.go` instead of being spread across consumer tests.
+
+#### Negative
+- All call sites in `pkg/workflow` must be updated when helper semantics change; there is no local override capability per file.
+- The `expression_patterns.go` file is already large (regex patterns + helper functions); additional consolidation could make it a maintenance hotspot.
+
+#### Neutral
+- `pkg/cli` becomes a thin delegation layer with no independent business logic for version management; callers of `cli.GetVersion()` and `cli.SetVersionInfo()` are unaffected at the API level.
+- New tests (`expression_patterns_test.go`, `version_test.go`) were added to cover the consolidated logic and the delegation contract.
+
+---
+
+## Part 2 — Normative Specification (RFC 2119)
+
+> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119).
+
+### Expression Detection Helpers
+
+1. All GitHub Actions expression-detection predicates used within `pkg/workflow` **MUST** be defined in `pkg/workflow/expression_patterns.go`.
+2. Individual `pkg/workflow` source files **MUST NOT** define their own local boolean helpers that check for `${{` or `}}` patterns.
+3. The three canonical helpers **MUST** provide the following distinct semantics:
+   - `hasExpressionMarker(s string) bool` — returns `true` if `s` contains the substring `${{` (permissive, partial-expression check).
+   - `containsExpression(s string) bool` — returns `true` if `s` contains a complete non-empty expression (a `${{` followed by at least one character before `}}`).
+   - `isExpression(s string) bool` — returns `true` if the entire string `s` is a single expression (starts with `${{` and ends with `}}`).
+4. Callers **MUST** select the helper whose semantics match their intent; using `hasExpressionMarker` where `containsExpression` or `isExpression` is needed (or vice versa) constitutes non-conformance.
+5. New expression-detection predicates **SHOULD** be added to `expression_patterns.go` rather than introduced inline in consumer files.
+
+### CLI Version State
+
+1. `pkg/cli` **MUST NOT** maintain its own local version variable or an `init()` function that synchronizes version state from `pkg/workflow`.
+2. `cli.GetVersion()` **MUST** delegate directly to `workflow.GetVersion()` with no local caching or transformation.
+3. `cli.SetVersionInfo(v string)` **MUST** delegate directly to `workflow.SetVersion(v)` with no local storage.
+4. All version state **MUST** be owned exclusively by `pkg/workflow`; `pkg/cli` **MAY** expose wrapper functions for backwards-compatible API surface but **MUST NOT** shadow or duplicate the underlying state.
+
+### Conformance
+
+An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement — in particular, defining expression-detection helpers outside `expression_patterns.go` or maintaining independent version state in `pkg/cli` — constitutes non-conformance.
+
+---
+
+*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24632327310) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

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