Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jan 19, 2026

Script registry validation failures caused process crashes via panic(). While marked as "programming errors caught during development," panics in production prevent graceful degradation and create deadlock risk when holding mutex locks.

Changes

Script Registry (pkg/workflow/script_registry.go)

  • RegisterWithMode() and RegisterWithAction() now return error instead of panicking
  • Register() propagates errors from RegisterWithMode()
  • Error messages include script name context for debugging

Before:

func (r *ScriptRegistry) RegisterWithMode(name string, source string, mode RuntimeMode) {
    if err := validateNoExecSync(name, source, mode); err != nil {
        panic(fmt.Sprintf("Script registration validation failed: %v", err))
    }
    // ...
}

After:

func (r *ScriptRegistry) RegisterWithMode(name string, source string, mode RuntimeMode) error {
    if err := validateNoExecSync(name, source, mode); err != nil {
        return fmt.Errorf("script registration validation failed for %q: %w", name, err)
    }
    // ...
    return nil
}

Test Updates

  • 6 test files updated to handle returned errors
  • Validation tests changed from assert.Panics to require.Error
  • All 17 script registry tests pass

Impact

Validation errors now return instead of crash. No production code currently registers scripts, so migration is complete.

Original prompt

This section details on the original issue you should resolve

<issue_title>[Code Quality] Replace panics with error returns in script registry</issue_title>
<issue_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

  1. pkg/workflow/script_registry.go (lines 139-147, 179-185)
  2. All callers of RegisterWithMode and RegisterWithAction need 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 githubnext/gh-aw#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

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits January 19, 2026 20:07
- Updated RegisterWithMode to return error instead of panic
- Updated RegisterWithAction to return error instead of panic
- Updated Register wrapper to return error
- Updated all test files to check for errors instead of panics
- Updated validation tests to expect errors instead of panics
- All script registry tests passing

Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix panics with error returns in script registry Replace panics with error returns in script registry Jan 19, 2026
Copilot AI requested a review from mnkiefer January 19, 2026 20:15
@pelikhan pelikhan marked this pull request as ready for review January 19, 2026 20:16
@pelikhan pelikhan merged commit 74959ee into main Jan 19, 2026
116 checks passed
@pelikhan pelikhan deleted the copilot/replace-panics-with-error-returns branch January 19, 2026 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Code Quality] Replace panics with error returns in script registry

3 participants