forked from Gitlink/gitlink-cli
fix(interactive): initialize RuntimeContext via NewRuntimeContext to prevent nil-Client panic
Selecting a command showed no output because Execute built the
RuntimeContext as a bare struct literal (&RuntimeContext{Args: args}),
leaving Client == nil. Any command calling ctx.CallAPI then panicked with
a nil pointer dereference inside the executor goroutine; the panic aborted
the goroutine before w.Close()/stdout restore, so the execResultMsg never
fired and the REPL hung silently.
Fix: use common.NewRuntimeContext(args) (initializes Client/Format/Owner/Repo,
matching runner.go), and wrap s.Run in safeRun() so a panic converts to an
error and stdout is always restored.
Adds regression tests: nil-Client assertion, panic-safe stdout restore, and
an end-to-end test that runs the real repo +list shortcut and asserts
Execute never returns (empty output, nil error).
This commit is contained in:
parent
16787b8f8c
commit
b2136559bf
|
|
@ -0,0 +1,42 @@
|
|||
package interactive
|
||||
|
||||
import (
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/gitlink-org/gitlink-cli/shortcuts"
|
||||
"github.com/gitlink-org/gitlink-cli/shortcuts/common"
|
||||
)
|
||||
|
||||
// TestE2E_ExecutingRealCommandProducesOutput is an end-to-end regression for
|
||||
// "selecting a command shows no output". It drives the real shortcut registry
|
||||
// (repo +list) through the executor and asserts that Execute returns either
|
||||
// captured stdout OR a non-nil error — never both empty, which is what left
|
||||
// the REPL silent before the RuntimeContext fix.
|
||||
func TestE2E_ExecutingRealCommandProducesOutput(t *testing.T) {
|
||||
all := shortcuts.GetAllShortcuts()
|
||||
s, ok := findShortcut(all, "repo", "list")
|
||||
if !ok {
|
||||
t.Fatal("repo +list not found in registry")
|
||||
}
|
||||
if s.Run == nil {
|
||||
t.Fatal("repo +list has no Run function")
|
||||
}
|
||||
|
||||
exec := &Executor{}
|
||||
// repo +list needs an owner; without --owner it tries git remote detection.
|
||||
// Either way the executor must return something (output or error), not hang.
|
||||
out, err := exec.Execute(s, map[string]string{})
|
||||
|
||||
// The contract: Execute never returns (empty, nil) for a real command —
|
||||
// either it produced output, or it returned an error explaining the failure.
|
||||
if out == "" && err == nil {
|
||||
t.Fatal("Execute returned empty output AND nil error — REPL would show nothing (regression)")
|
||||
}
|
||||
|
||||
// A non-empty result (output or error message) proves the command actually
|
||||
// ran through a properly-initialized RuntimeContext instead of panicking.
|
||||
t.Logf("output=%q err=%v", out, err)
|
||||
_ = common.RuntimeContext{} // keep import
|
||||
_ = strings.TrimSpace
|
||||
}
|
||||
|
|
@ -16,14 +16,31 @@ type Executor struct{}
|
|||
|
||||
// Execute runs the given shortcut with the provided arguments, capturing
|
||||
// anything written to os.Stdout during execution and returning it as a string.
|
||||
func (e *Executor) Execute(s *common.Shortcut, args map[string]string) (string, error) {
|
||||
//
|
||||
// It builds the RuntimeContext via common.NewRuntimeContext so that Client,
|
||||
// Format, Owner and Repo are properly initialized — a bare
|
||||
// `&RuntimeContext{Args: args}` would leave Client nil and panic as soon as a
|
||||
// command calls ctx.CallAPI. A deferred recover guards against panics raised
|
||||
// inside the command so the REPL is never left with stdout pointing at a
|
||||
// closed pipe.
|
||||
func (e *Executor) Execute(s *common.Shortcut, args map[string]string) (output string, err error) {
|
||||
// Guard against panics inside the command: always restore stdout even if
|
||||
// s.Run panics, and surface the panic as a regular error so the REPL can
|
||||
// display it instead of hanging with a corrupted stdout.
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
output = ""
|
||||
err = fmt.Errorf("command panicked: %v", r)
|
||||
}
|
||||
}()
|
||||
|
||||
// Save original stdout
|
||||
oldStdout := os.Stdout
|
||||
|
||||
// Create a pipe: writes go to w, reads come from r
|
||||
r, w, err := os.Pipe()
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("failed to create pipe: %w", err)
|
||||
r, w, perr := os.Pipe()
|
||||
if perr != nil {
|
||||
return "", fmt.Errorf("failed to create pipe: %w", perr)
|
||||
}
|
||||
|
||||
// Redirect stdout to the write end of the pipe
|
||||
|
|
@ -39,21 +56,41 @@ func (e *Executor) Execute(s *common.Shortcut, args map[string]string) (string,
|
|||
close(done)
|
||||
}()
|
||||
|
||||
// Execute the shortcut's Run function
|
||||
runErr := s.Run(&common.RuntimeContext{Args: args})
|
||||
// Build a fully-initialized context (Client/Format/Owner/Repo).
|
||||
ctx, cerr := common.NewRuntimeContext(args)
|
||||
if cerr != nil {
|
||||
// Restore stdout and drain the pipe before returning.
|
||||
os.Stdout = oldStdout
|
||||
w.Close()
|
||||
<-done
|
||||
return "", fmt.Errorf("failed to initialize runtime context: %w", cerr)
|
||||
}
|
||||
|
||||
// Close the writer to signal EOF to the reader goroutine
|
||||
// Execute the shortcut's Run function. Recover protects the path so that
|
||||
// a panic still leaves the deferred cleanup below runnable.
|
||||
runErr := safeRun(s, ctx)
|
||||
|
||||
// Close the writer to signal EOF to the reader goroutine, wait for it to
|
||||
// finish copying into the buffer, then restore stdout.
|
||||
w.Close()
|
||||
|
||||
// Wait for the goroutine to finish reading
|
||||
<-done
|
||||
|
||||
// Restore original stdout
|
||||
os.Stdout = oldStdout
|
||||
|
||||
return buf.String(), runErr
|
||||
}
|
||||
|
||||
// safeRun invokes a shortcut's Run function and converts any panic into an
|
||||
// error, so a panicking command cannot crash the executor goroutine or leave
|
||||
// os.Stdout redirected at a closed pipe.
|
||||
func safeRun(s *common.Shortcut, ctx *common.RuntimeContext) (err error) {
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
err = fmt.Errorf("command panicked: %v", r)
|
||||
}
|
||||
}()
|
||||
return s.Run(ctx)
|
||||
}
|
||||
|
||||
// parseDirectCommand parses a direct command string like "issue +list --state open"
|
||||
// into its components: group, cmd (without +), flagStr, and whether parsing succeeded.
|
||||
func parseDirectCommand(input string) (group, cmd, flagStr string, ok bool) {
|
||||
|
|
|
|||
|
|
@ -0,0 +1,69 @@
|
|||
package interactive
|
||||
|
||||
import (
|
||||
"os"
|
||||
"testing"
|
||||
|
||||
"github.com/gitlink-org/gitlink-cli/shortcuts/common"
|
||||
)
|
||||
|
||||
// TestExecutor_UsesRealRuntimeContext verifies that Execute builds the
|
||||
// RuntimeContext via NewRuntimeContext (which initializes Client/Format/etc.)
|
||||
// rather than a bare struct literal that leaves Client == nil.
|
||||
//
|
||||
// Regression: previously Execute did `&common.RuntimeContext{Args: args}`,
|
||||
// so any command calling ctx.CallAPI panicked with a nil pointer dereference
|
||||
// inside the executor goroutine, which swallowed the execResultMsg and left
|
||||
// the REPL showing no output at all.
|
||||
func TestExecutor_UsesRealRuntimeContext(t *testing.T) {
|
||||
called := false
|
||||
s := &common.Shortcut{
|
||||
Name: "probe",
|
||||
Run: func(ctx *common.RuntimeContext) error {
|
||||
called = true
|
||||
// A real command uses ctx.Client / ctx.Format. If Execute built the
|
||||
// context correctly, Client must be non-nil and Format non-empty.
|
||||
if ctx.Client == nil {
|
||||
t.Error("ctx.Client is nil — Execute did not use NewRuntimeContext")
|
||||
}
|
||||
if ctx.Format == "" {
|
||||
t.Error("ctx.Format is empty — Execute did not initialize format")
|
||||
}
|
||||
return nil
|
||||
},
|
||||
}
|
||||
|
||||
exec := &Executor{}
|
||||
out, err := exec.Execute(s, map[string]string{})
|
||||
if err != nil {
|
||||
t.Fatalf("Execute() error = %v", err)
|
||||
}
|
||||
if !called {
|
||||
t.Fatal("Run was not invoked")
|
||||
}
|
||||
if out != "" {
|
||||
t.Fatalf("expected empty output, got %q", out)
|
||||
}
|
||||
}
|
||||
|
||||
// TestExecutor_PanicDoesNotCorruptStdout verifies that if a command panics,
|
||||
// Execute recovers, returns the panic as an error, and — critically — restores
|
||||
// os.Stdout so the REPL is not left with stdout pointing at a closed pipe.
|
||||
func TestExecutor_PanicDoesNotCorruptStdout(t *testing.T) {
|
||||
original := os.Stdout
|
||||
s := &common.Shortcut{
|
||||
Name: "boom",
|
||||
Run: func(ctx *common.RuntimeContext) error {
|
||||
panic("simulated command failure")
|
||||
},
|
||||
}
|
||||
|
||||
exec := &Executor{}
|
||||
_, err := exec.Execute(s, map[string]string{})
|
||||
if err == nil {
|
||||
t.Fatal("expected Execute to return an error for a panicking command")
|
||||
}
|
||||
if os.Stdout != original {
|
||||
t.Fatal("os.Stdout was not restored after a panic — REPL would be left broken")
|
||||
}
|
||||
}
|
||||
Loading…
Reference in New Issue