-
Notifications
You must be signed in to change notification settings - Fork 93
Closed
Description
Description
The script registry uses panic() for validation failures in production code. While comments claim these are "programming errors caught during development," panics in production prevent graceful degradation and crash the entire process.
Problem
func (r *ScriptRegistry) RegisterWithMode(name string, source string, mode RuntimeMode) {
r.mu.Lock()
defer r.mu.Unlock()
if err := validateNoExecSync(name, source, mode); err != nil {
// This is a programming error that should be caught during development
panic(fmt.Sprintf("Script registration validation failed: %v", err))
}
if err := validateNoGitHubScriptGlobals(name, source, mode); err != nil {
panic(fmt.Sprintf("Script registration validation failed: %v", err))
}
// ... rest of function
}Impact
- Severity: High
- Risk: Process crashes, no recovery possible, deadlock risk (panic while holding lock)
- Files Affected:
pkg/workflow/script_registry.go:139-147, 179-185 - Pattern: 4 instances of panic in script_registry.go
Suggested Fix
Return errors instead of panicking:
func (r *ScriptRegistry) RegisterWithMode(name string, source string, mode RuntimeMode) error {
r.mu.Lock()
defer r.mu.Unlock()
if err := validateNoExecSync(name, source, mode); err != nil {
return fmt.Errorf("script registration validation failed for %q: %w", name, err)
}
if err := validateNoGitHubScriptGlobals(name, source, mode); err != nil {
return fmt.Errorf("script registration validation failed for %q: %w", name, err)
}
r.scripts[name] = &scriptEntry{
source: source,
mode: mode,
actionPath: "",
}
return nil
}Files to Update
pkg/workflow/script_registry.go(lines 139-147, 179-185)- All callers of
RegisterWithModeandRegisterWithActionneed error handling
Success Criteria
- Replace all panic() calls with error returns
- Find all callers using
find_referencing_symbols - Update callers to handle returned errors
- Run
pkg/workflow/script_registry_test.go - Verify validation catches same issues without crashing
- Consider whether validation should happen at compile-time instead
Estimated Effort
Medium (1-2 weeks) - Requires updating all registration call sites
Source
Extracted from Sergo Concurrency & Error Flow Analysis discussion #10665
Priority
High - Prevents graceful degradation and could crash production processes
AI generated by Discussion Task Miner - Code Quality Improvement Agent
- expires on Feb 2, 2026, 2:08 PM UTC
Reactions are currently unavailable