Skip to content

Commit 74959ee

Browse files
CopilotCopilot
authored
Replace panics with error returns in script registry (#10744)
1 parent e063996 commit 74959ee

6 files changed

Lines changed: 93 additions & 62 deletions

‎pkg/workflow/bundler_script_validation_test.go‎

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -197,48 +197,46 @@ const repo = github.context.repo;
197197
}
198198

199199
func TestScriptRegistry_RegisterWithMode_Validation(t *testing.T) {
200-
t.Run("GitHub Script mode with execSync should panic", func(t *testing.T) {
200+
t.Run("GitHub Script mode with execSync should return error", func(t *testing.T) {
201201
registry := NewScriptRegistry()
202202
invalidScript := `
203203
const { execSync } = require("child_process");
204204
execSync("ls -la");
205205
`
206-
assert.Panics(t, func() {
207-
registry.RegisterWithMode("invalid_script", invalidScript, RuntimeModeGitHubScript)
208-
}, "Should panic when registering GitHub Script with execSync")
206+
err := registry.RegisterWithMode("invalid_script", invalidScript, RuntimeModeGitHubScript)
207+
require.Error(t, err, "Should return error when registering GitHub Script with execSync")
208+
assert.Contains(t, err.Error(), "execSync", "Error should mention execSync")
209209
})
210210

211-
t.Run("Node.js mode with GitHub Actions globals should panic", func(t *testing.T) {
211+
t.Run("Node.js mode with GitHub Actions globals should return error", func(t *testing.T) {
212212
registry := NewScriptRegistry()
213213
invalidScript := `
214214
const fs = require("fs");
215215
core.info("This should not be here");
216216
`
217-
assert.Panics(t, func() {
218-
registry.RegisterWithMode("invalid_script", invalidScript, RuntimeModeNodeJS)
219-
}, "Should panic when registering Node.js script with GitHub Actions globals")
217+
err := registry.RegisterWithMode("invalid_script", invalidScript, RuntimeModeNodeJS)
218+
require.Error(t, err, "Should return error when registering Node.js script with GitHub Actions globals")
219+
assert.Contains(t, err.Error(), "GitHub Actions global", "Error should mention GitHub Actions globals")
220220
})
221221

222-
t.Run("Valid GitHub Script mode should not panic", func(t *testing.T) {
222+
t.Run("Valid GitHub Script mode should not return error", func(t *testing.T) {
223223
registry := NewScriptRegistry()
224224
validScript := `
225225
const { exec } = require("@actions/exec");
226226
core.info("This is valid for GitHub Script mode");
227227
`
228-
assert.NotPanics(t, func() {
229-
registry.RegisterWithMode("valid_script", validScript, RuntimeModeGitHubScript)
230-
}, "Should not panic with valid GitHub Script")
228+
err := registry.RegisterWithMode("valid_script", validScript, RuntimeModeGitHubScript)
229+
assert.NoError(t, err, "Should not return error with valid GitHub Script")
231230
})
232231

233-
t.Run("Valid Node.js mode should not panic", func(t *testing.T) {
232+
t.Run("Valid Node.js mode should not return error", func(t *testing.T) {
234233
registry := NewScriptRegistry()
235234
validScript := `
236235
const fs = require("fs");
237236
const { execSync } = require("child_process");
238237
console.log("This is valid for Node.js mode");
239238
`
240-
assert.NotPanics(t, func() {
241-
registry.RegisterWithMode("valid_script", validScript, RuntimeModeNodeJS)
242-
}, "Should not panic with valid Node.js script")
239+
err := registry.RegisterWithMode("valid_script", validScript, RuntimeModeNodeJS)
240+
assert.NoError(t, err, "Should not return error with valid Node.js script")
243241
})
244242
}

‎pkg/workflow/compiler_action_mode_test.go‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"testing"
77

88
"github.com/githubnext/gh-aw/pkg/stringutil"
9+
"github.com/stretchr/testify/require"
910
)
1011

1112
// TestActionModeDetection tests the DetectActionMode function
@@ -321,19 +322,20 @@ Test workflow with release mode.
321322

322323
// Register test script with action path
323324
testScript := `const { core } = require('@actions/core'); core.info('test');`
324-
DefaultScriptRegistry.RegisterWithAction(
325+
err := DefaultScriptRegistry.RegisterWithAction(
325326
"create_issue",
326327
testScript,
327328
RuntimeModeGitHubScript,
328329
"./actions/create-issue",
329330
)
331+
require.NoError(t, err)
330332

331333
// Restore after test
332334
defer func() {
333335
if origActionPath != "" {
334-
DefaultScriptRegistry.RegisterWithAction("create_issue", origScript, RuntimeModeGitHubScript, origActionPath)
336+
_ = DefaultScriptRegistry.RegisterWithAction("create_issue", origScript, RuntimeModeGitHubScript, origActionPath)
335337
} else {
336-
DefaultScriptRegistry.RegisterWithMode("create_issue", origScript, RuntimeModeGitHubScript)
338+
_ = DefaultScriptRegistry.RegisterWithMode("create_issue", origScript, RuntimeModeGitHubScript)
337339
}
338340
}()
339341

@@ -408,13 +410,14 @@ Test
408410
origActionPath := DefaultScriptRegistry.GetActionPath("create_issue")
409411

410412
testScript := `const { core } = require('@actions/core'); core.info('test');`
411-
DefaultScriptRegistry.RegisterWithAction("create_issue", testScript, RuntimeModeGitHubScript, "./actions/create-issue")
413+
err := DefaultScriptRegistry.RegisterWithAction("create_issue", testScript, RuntimeModeGitHubScript, "./actions/create-issue")
414+
require.NoError(t, err)
412415

413416
defer func() {
414417
if origActionPath != "" {
415-
DefaultScriptRegistry.RegisterWithAction("create_issue", origScript, RuntimeModeGitHubScript, origActionPath)
418+
_ = DefaultScriptRegistry.RegisterWithAction("create_issue", origScript, RuntimeModeGitHubScript, origActionPath)
416419
} else {
417-
DefaultScriptRegistry.RegisterWithMode("create_issue", origScript, RuntimeModeGitHubScript)
420+
_ = DefaultScriptRegistry.RegisterWithMode("create_issue", origScript, RuntimeModeGitHubScript)
418421
}
419422
}()
420423

‎pkg/workflow/compiler_custom_actions_test.go‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"testing"
77

88
"github.com/githubnext/gh-aw/pkg/stringutil"
9+
"github.com/stretchr/testify/require"
910
)
1011

1112
// TestActionModeValidation tests the ActionMode type validation
@@ -105,7 +106,8 @@ func TestScriptRegistryWithAction(t *testing.T) {
105106
testScript := `console.log('test');`
106107
actionPath := "./actions/test-action"
107108

108-
registry.RegisterWithAction("test_script", testScript, RuntimeModeGitHubScript, actionPath)
109+
err := registry.RegisterWithAction("test_script", testScript, RuntimeModeGitHubScript, actionPath)
110+
require.NoError(t, err)
109111

110112
if !registry.Has("test_script") {
111113
t.Error("Script should be registered")
@@ -163,20 +165,21 @@ Test workflow with safe-outputs.
163165
const { core } = require('@actions/core');
164166
core.info('Creating issue');
165167
`
166-
DefaultScriptRegistry.RegisterWithAction(
168+
err := DefaultScriptRegistry.RegisterWithAction(
167169
"create_issue",
168170
testScript,
169171
RuntimeModeGitHubScript,
170172
"./actions/create-issue",
171173
)
174+
require.NoError(t, err)
172175

173176
// Restore after test
174177
defer func() {
175178
if origSource != "" {
176179
if origActionPath != "" {
177-
DefaultScriptRegistry.RegisterWithAction("create_issue", origSource, RuntimeModeGitHubScript, origActionPath)
180+
_ = DefaultScriptRegistry.RegisterWithAction("create_issue", origSource, RuntimeModeGitHubScript, origActionPath)
178181
} else {
179-
DefaultScriptRegistry.RegisterWithMode("create_issue", origSource, RuntimeModeGitHubScript)
182+
_ = DefaultScriptRegistry.RegisterWithMode("create_issue", origSource, RuntimeModeGitHubScript)
180183
}
181184
}
182185
}()
@@ -305,15 +308,16 @@ Test fallback to inline mode.
305308
origActionPath := DefaultScriptRegistry.GetActionPath("create_issue")
306309

307310
testScript := `console.log('test');`
308-
DefaultScriptRegistry.RegisterWithMode("create_issue", testScript, RuntimeModeGitHubScript)
311+
err := DefaultScriptRegistry.RegisterWithMode("create_issue", testScript, RuntimeModeGitHubScript)
312+
require.NoError(t, err)
309313

310314
// Restore after test
311315
defer func() {
312316
if origSource != "" {
313317
if origActionPath != "" {
314-
DefaultScriptRegistry.RegisterWithAction("create_issue", origSource, RuntimeModeGitHubScript, origActionPath)
318+
_ = DefaultScriptRegistry.RegisterWithAction("create_issue", origSource, RuntimeModeGitHubScript, origActionPath)
315319
} else {
316-
DefaultScriptRegistry.RegisterWithMode("create_issue", origSource, RuntimeModeGitHubScript)
320+
_ = DefaultScriptRegistry.RegisterWithMode("create_issue", origSource, RuntimeModeGitHubScript)
317321
}
318322
}
319323
}()

‎pkg/workflow/custom_action_copilot_token_test.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"testing"
66

77
"github.com/stretchr/testify/assert"
8+
"github.com/stretchr/testify/require"
89
)
910

1011
// TestCustomActionCopilotTokenFallback tests that custom actions use the correct
@@ -15,7 +16,8 @@ func TestCustomActionCopilotTokenFallback(t *testing.T) {
1516
// Register a test custom action
1617
testScript := `console.log('test');`
1718
actionPath := "./actions/test-action"
18-
DefaultScriptRegistry.RegisterWithAction("test_handler", testScript, RuntimeModeGitHubScript, actionPath)
19+
err := DefaultScriptRegistry.RegisterWithAction("test_handler", testScript, RuntimeModeGitHubScript, actionPath)
20+
require.NoError(t, err)
1921

2022
workflowData := &WorkflowData{
2123
Name: "Test Workflow",

‎pkg/workflow/script_registry.go‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,10 @@ func NewScriptRegistry() *ScriptRegistry {
105105
//
106106
// If a script with the same name already exists, it will be overwritten.
107107
// This is useful for testing but should be avoided in production.
108-
func (r *ScriptRegistry) Register(name string, source string) {
109-
r.RegisterWithMode(name, source, RuntimeModeGitHubScript)
108+
//
109+
// Returns an error if validation fails.
110+
func (r *ScriptRegistry) Register(name string, source string) error {
111+
return r.RegisterWithMode(name, source, RuntimeModeGitHubScript)
110112
}
111113

112114
// RegisterWithMode adds a script source to the registry with a specific runtime mode.
@@ -125,9 +127,9 @@ func (r *ScriptRegistry) Register(name string, source string) {
125127
// - GitHub Script mode: validates no execSync usage (should use exec instead)
126128
// - Node.js mode: validates no GitHub Actions globals (core.*, exec.*, github.*)
127129
//
128-
// Panics if validation fails, as this indicates a programming error that should be
129-
// caught during development and testing.
130-
func (r *ScriptRegistry) RegisterWithMode(name string, source string, mode RuntimeMode) {
130+
// Returns an error if validation fails, allowing the caller to handle gracefully
131+
// instead of crashing the process.
132+
func (r *ScriptRegistry) RegisterWithMode(name string, source string, mode RuntimeMode) error {
131133
r.mu.Lock()
132134
defer r.mu.Unlock()
133135

@@ -137,20 +139,20 @@ func (r *ScriptRegistry) RegisterWithMode(name string, source string, mode Runti
137139

138140
// Perform compile-time validation based on runtime mode
139141
if err := validateNoExecSync(name, source, mode); err != nil {
140-
// This is a programming error that should be caught during development
141-
panic(fmt.Sprintf("Script registration validation failed: %v", err))
142+
return fmt.Errorf("script registration validation failed for %q: %w", name, err)
142143
}
143144

144145
if err := validateNoGitHubScriptGlobals(name, source, mode); err != nil {
145-
// This is a programming error that should be caught during development
146-
panic(fmt.Sprintf("Script registration validation failed: %v", err))
146+
return fmt.Errorf("script registration validation failed for %q: %w", name, err)
147147
}
148148

149149
r.scripts[name] = &scriptEntry{
150150
source: source,
151151
mode: mode,
152152
actionPath: "", // No custom action by default
153153
}
154+
155+
return nil
154156
}
155157

156158
// RegisterWithAction registers a script with both inline code and a custom action path.
@@ -166,7 +168,10 @@ func (r *ScriptRegistry) RegisterWithMode(name string, source string, mode Runti
166168
// The actionPath should be a relative path from the repository root for development mode.
167169
// In the future, this can be extended to support versioned references like
168170
// "githubnext/gh-aw/.github/actions/create-issue@SHA" for release mode.
169-
func (r *ScriptRegistry) RegisterWithAction(name string, source string, mode RuntimeMode, actionPath string) {
171+
//
172+
// Returns an error if validation fails, allowing the caller to handle gracefully
173+
// instead of crashing the process.
174+
func (r *ScriptRegistry) RegisterWithAction(name string, source string, mode RuntimeMode, actionPath string) error {
170175
r.mu.Lock()
171176
defer r.mu.Unlock()
172177

@@ -177,18 +182,20 @@ func (r *ScriptRegistry) RegisterWithAction(name string, source string, mode Run
177182

178183
// Perform compile-time validation based on runtime mode
179184
if err := validateNoExecSync(name, source, mode); err != nil {
180-
panic(fmt.Sprintf("Script registration validation failed: %v", err))
185+
return fmt.Errorf("script registration validation failed for %q: %w", name, err)
181186
}
182187

183188
if err := validateNoGitHubScriptGlobals(name, source, mode); err != nil {
184-
panic(fmt.Sprintf("Script registration validation failed: %v", err))
189+
return fmt.Errorf("script registration validation failed for %q: %w", name, err)
185190
}
186191

187192
r.scripts[name] = &scriptEntry{
188193
source: source,
189194
mode: mode,
190195
actionPath: actionPath,
191196
}
197+
198+
return nil
192199
}
193200

194201
// GetActionPath retrieves the custom action path for a script, if registered.

0 commit comments

Comments
 (0)