Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions pkg/workflow/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/json"
"fmt"
"os"
"slices"
"sort"
"strings"

Expand All @@ -14,6 +15,9 @@ import (

var cacheLog = logger.New("workflow:cache")

// validCacheMemoryScopes defines the allowed values for cache-memory scope
var validCacheMemoryScopes = []string{"workflow", "repo"}

// CacheMemoryConfig holds configuration for cache-memory functionality
type CacheMemoryConfig struct {
Caches []CacheMemoryEntry `yaml:"caches,omitempty"` // cache configurations
Expand Down Expand Up @@ -112,6 +116,10 @@ func parseCacheMemoryEntry(cacheMap map[string]any, defaultID string) (CacheMemo
if entry.Scope == "" {
entry.Scope = "workflow"
}
// Validate scope value
if !slices.Contains(validCacheMemoryScopes, entry.Scope) {
return entry, fmt.Errorf("invalid cache-memory scope %q: must be one of %v", entry.Scope, validCacheMemoryScopes)
}
Comment on lines +119 to +122
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list of valid scopes is duplicated (once in the slices.Contains call and again in the error string). This can drift if a new scope is added. Define a validScopes slice once (optionally at package scope) and use it both for validation and for formatting the error message (e.g., %v), so there’s a single source of truth.

Copilot uses AI. Check for mistakes.

// Parse allowed-extensions field
if allowedExts, exists := cacheMap["allowed-extensions"]; exists {
Expand Down
146 changes: 146 additions & 0 deletions pkg/workflow/cache_scope_validation_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
//go:build !integration

package workflow

import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

// TestCacheMemoryScopeValidationObject tests scope validation with object notation
func TestCacheMemoryScopeValidationObject(t *testing.T) {
tests := []struct {
name string
scope string
wantError bool
errorText string
}{
{
name: "valid workflow scope",
scope: "workflow",
wantError: false,
},
{
name: "valid repo scope",
scope: "repo",
wantError: false,
},
{
name: "invalid organization scope",
scope: "organization",
wantError: true,
errorText: `invalid cache-memory scope "organization": must be one of [workflow repo]`,
},
{
name: "invalid global scope",
scope: "global",
wantError: true,
errorText: `invalid cache-memory scope "global": must be one of [workflow repo]`,
},
{
name: "invalid whitespace scope",
scope: " ",
wantError: true,
errorText: `invalid cache-memory scope " ": must be one of [workflow repo]`,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
toolsMap := map[string]any{
"cache-memory": map[string]any{
"scope": tt.scope,
},
}

toolsConfig, err := ParseToolsConfig(toolsMap)
require.NoError(t, err, "Should parse tools config")

compiler := NewCompiler()
_, err = compiler.extractCacheMemoryConfig(toolsConfig)

if tt.wantError {
require.Error(t, err, "Should return error for invalid scope")
assert.ErrorContains(t, err, tt.errorText, "Error should contain expected message")
} else {
assert.NoError(t, err, "Should not return error for valid scope")
}
})
}
}

// TestCacheMemoryScopeValidationArray tests scope validation with array notation
func TestCacheMemoryScopeValidationArray(t *testing.T) {
tests := []struct {
name string
scope string
wantError bool
errorText string
}{
{
name: "valid workflow scope in array",
scope: "workflow",
wantError: false,
},
{
name: "valid repo scope in array",
scope: "repo",
wantError: false,
},
{
name: "invalid scope in array",
scope: "global",
wantError: true,
errorText: `invalid cache-memory scope "global": must be one of [workflow repo]`,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
toolsMap := map[string]any{
"cache-memory": []any{
map[string]any{
"id": "test-cache",
"scope": tt.scope,
},
},
}

toolsConfig, err := ParseToolsConfig(toolsMap)
require.NoError(t, err, "Should parse tools config")

compiler := NewCompiler()
_, err = compiler.extractCacheMemoryConfig(toolsConfig)

if tt.wantError {
require.Error(t, err, "Should return error for invalid scope")
assert.ErrorContains(t, err, tt.errorText, "Error should contain expected message")
} else {
assert.NoError(t, err, "Should not return error for valid scope")
}
})
}
}

// TestCacheMemoryScopeDefault tests that omitting scope defaults to "workflow"
func TestCacheMemoryScopeDefault(t *testing.T) {
toolsMap := map[string]any{
"cache-memory": map[string]any{
"key": "my-cache-key",
},
}

toolsConfig, err := ParseToolsConfig(toolsMap)
require.NoError(t, err, "Should parse tools config")

compiler := NewCompiler()
config, err := compiler.extractCacheMemoryConfig(toolsConfig)

require.NoError(t, err, "Should not error when scope is omitted")
require.NotNil(t, config, "Config should not be nil")
require.Len(t, config.Caches, 1, "Should have exactly one cache entry")

assert.Equal(t, "workflow", config.Caches[0].Scope, "Default scope should be 'workflow'")
}
Loading