From b2136559bfbddd35dea88f3251d0a53cdbd8debf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=88=98=E7=84=B1?= <1877685089@qq.com> Date: Mon, 8 Jun 2026 19:16:40 +0800 Subject: [PATCH] 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). --- cmd/interactive/e2e_command_test.go | 42 +++++++++++++++ cmd/interactive/executor.go | 59 ++++++++++++++++---- cmd/interactive/executor_runtime_test.go | 69 ++++++++++++++++++++++++ 3 files changed, 159 insertions(+), 11 deletions(-) create mode 100644 cmd/interactive/e2e_command_test.go create mode 100644 cmd/interactive/executor_runtime_test.go diff --git a/cmd/interactive/e2e_command_test.go b/cmd/interactive/e2e_command_test.go new file mode 100644 index 0000000..f3338c1 --- /dev/null +++ b/cmd/interactive/e2e_command_test.go @@ -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 +} diff --git a/cmd/interactive/executor.go b/cmd/interactive/executor.go index 09e995d..381e53d 100644 --- a/cmd/interactive/executor.go +++ b/cmd/interactive/executor.go @@ -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) { diff --git a/cmd/interactive/executor_runtime_test.go b/cmd/interactive/executor_runtime_test.go new file mode 100644 index 0000000..20b7af6 --- /dev/null +++ b/cmd/interactive/executor_runtime_test.go @@ -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") + } +}