fix(extension): address review findings 2-6

- Replace brittle Cobra error-string matching and hand-maintained
  globalFlagsWithValues allowlist with rootCmd.Find() + Flags().Args(),
  so extension dispatch is robust to new flags and Cobra version changes
- Return ErrExited from Run instead of calling os.Exit directly, so
  deferred cleanup runs and Run is testable
- Rename injected env vars CIRCLE_TOKEN/CIRCLE_URL → CIRCLECI_TOKEN/CIRCLECI_HOST
  (and VCS/project vars) to match the project's documented CIRCLECI_ prefix
- Extract skipUnlessShell helper in acceptance tests to consolidate the
  Windows skip guard in one place

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: webster

acceptance/extension_test.go

@@ -37,15 +37,22 @@ import (
 	testenv "github.com/CircleCI-Public/circleci-cli-v2/internal/testing/env"
 )
 
+// skipUnlessShell skips the test on platforms where shell scripts cannot be
+// executed as extension binaries (i.e. Windows).
+func skipUnlessShell(t *testing.T) {
+	t.Helper()
+	skip.If(t, runtime.GOOS == "windows", "shell-script extensions are not supported on Windows")
+}
+
 // buildExtension writes a shell script as a fake circleci-<name> extension
 // into extDir and returns the full path. The script prints its positional args
-// and the CIRCLE_TOKEN / CIRCLE_URL env vars, then exits 0.
+// and the CIRCLECI_TOKEN / CIRCLECI_HOST env vars, then exits 0.
 func buildExtension(t *testing.T, extDir, name string) string {
 	t.Helper()
-	skip.If(t, runtime.GOOS == "windows", "shell-script extensions are not supported on Windows")
+	skipUnlessShell(t)
 
 	path := filepath.Join(extDir, "circleci-"+name)
-	script := "#!/bin/sh\nprintf 'args:%s\\n' \"$*\"\nprintf 'token:%s\\n' \"$CIRCLE_TOKEN\"\nprintf 'url:%s\\n' \"$CIRCLE_URL\"\n"
+	script := "#!/bin/sh\nprintf 'args:%s\\n' \"$*\"\nprintf 'token:%s\\n' \"$CIRCLECI_TOKEN\"\nprintf 'url:%s\\n' \"$CIRCLECI_HOST\"\n"
 	err := os.WriteFile(path, []byte(script), 0o755)
 	assert.NilError(t, err)
 	return path
@@ -85,7 +92,7 @@ func TestExtensionDispatch(t *testing.T) {
 		"extension did not receive correct args; stdout: %q", result.Stdout)
 }
 
-// TestExtensionEnvInjection verifies that CIRCLE_TOKEN and CIRCLE_URL are
+// TestExtensionEnvInjection verifies that CIRCLECI_TOKEN and CIRCLECI_HOST are
 // injected into the extension's environment from the CLI's resolved config.
 func TestExtensionEnvInjection(t *testing.T) {
 	extDir := t.TempDir()
@@ -103,13 +110,15 @@ func TestExtensionEnvInjection(t *testing.T) {
 
 	assert.Equal(t, result.ExitCode, 0, "stderr: %s", result.Stderr)
 	assert.Check(t, cmp.Contains(result.Stdout, "token:"+testToken),
-		"CIRCLE_TOKEN not injected; stdout: %q", result.Stdout)
+		"CIRCLECI_TOKEN not injected; stdout: %q", result.Stdout)
+	assert.Check(t, cmp.Contains(result.Stdout, "url:https://circleci.com"),
+		"CIRCLECI_HOST not injected; stdout: %q", result.Stdout)
 }
 
 // TestExtensionExitCodePropagated verifies that the extension's exit code is
 // propagated back to the caller unchanged.
 func TestExtensionExitCodePropagated(t *testing.T) {
-	skip.If(t, runtime.GOOS == "windows", "shell-script extensions are not supported on Windows")
+	skipUnlessShell(t)
 
 	extDir := t.TempDir()
 	path := filepath.Join(extDir, "circleci-fail")
@@ -149,7 +158,7 @@ func TestExtensionNotFoundShowsOriginalError(t *testing.T) {
 // within a known group (e.g. "circleci pipeline foo") are not dispatched to
 // any extension — only top-level unknown commands are intercepted.
 func TestExtensionNestedUnknownNotIntercepted(t *testing.T) {
-	skip.If(t, runtime.GOOS == "windows", "shell-script extensions are not supported on Windows")
+	skipUnlessShell(t)
 
 	// Put a circleci-foo extension on PATH to confirm it is NOT invoked.
 	extDir := t.TempDir()

internal/extension/extension.go

@@ -24,7 +24,7 @@
 //
 // Any executable named "circleci-<name>" found in PATH is treated as an
 // extension and can be invoked transparently as "circleci <name>". The
-// extension receives CIRCLE_TOKEN, CIRCLE_URL, and best-effort project
+// extension receives CIRCLECI_TOKEN, CIRCLECI_HOST, and best-effort project
 // metadata via environment variables so it can call the CircleCI API without
 // reimplementing authentication.
 package extension
@@ -50,6 +50,17 @@ func (e *ErrNotFound) Error() string {
 	return fmt.Sprintf("unknown command %q — and no extension %q found in PATH", e.Name, "circleci-"+e.Name)
 }
 
+// ErrExited is returned by Run when the extension process exits with a
+// non-zero status code. The caller should exit with Code rather than printing
+// an error message — the extension is responsible for its own error output.
+type ErrExited struct {
+	Code int
+}
+
+func (e *ErrExited) Error() string {
+	return fmt.Sprintf("extension exited with code %d", e.Code)
+}
+
 // Run looks up circleci-<name> in PATH and execs it with args, injecting
 // CircleCI environment variables. configPath is the --config flag value
 // (empty means use the default XDG path). The current process is replaced
@@ -58,14 +69,14 @@ func (e *ErrNotFound) Error() string {
 //
 // If no matching binary is found, ErrNotFound is returned and the caller
 // should show the original "unknown command" error instead.
-func Run(ctx context.Context, name string, args []string, configPath string) error {
+func Run(ctx context.Context, name string, args []string, configPath string, secureStorage bool) error {
 	binary := "circleci-" + name
 	path, err := exec.LookPath(binary)
 	if err != nil {
 		return &ErrNotFound{Name: name}
 	}
 
-	env := buildEnv(ctx, configPath)
+	env := buildEnv(ctx, configPath, secureStorage)
 
 	cmd := exec.CommandContext(ctx, path, args...) //#nosec:G204 // path comes from LookPath, args are user-supplied CLI args for the extension
 	cmd.Stdin = os.Stdin
@@ -75,8 +86,7 @@ func Run(ctx context.Context, name string, args []string, configPath string) err
 
 	if err := cmd.Run(); err != nil {
 		if cmd.ProcessState != nil {
-			// Exit with the extension's exit code, not our own error message.
-			os.Exit(cmd.ProcessState.ExitCode())
+			return &ErrExited{Code: cmd.ProcessState.ExitCode()}
 		}
 		return clierrors.New(
 			"extension.exec_failed",
@@ -88,29 +98,29 @@ func Run(ctx context.Context, name string, args []string, configPath string) err
 }
 
 // buildEnv constructs the environment for the extension process. It starts
-// from the current process environment and overlays CIRCLE_* variables so
+// from the current process environment and overlays CIRCLECI_* variables so
 // extensions can call the CircleCI API without reimplementing auth.
-func buildEnv(ctx context.Context, configPath string) []string {
+func buildEnv(ctx context.Context, configPath string, secureStorage bool) []string {
 	env := os.Environ()
 
-	cfg, err := config.LoadFrom(ctx, configPath, false)
+	cfg, err := config.LoadFrom(ctx, configPath, secureStorage)
 	if err != nil {
 		cfg = &config.Config{}
 	}
 
 	overlays := map[string]string{
-		"CIRCLE_TOKEN": cfg.EffectiveToken(),
-		"CIRCLE_URL":   cfg.EffectiveHost(),
+		"CIRCLECI_TOKEN": cfg.EffectiveToken(),
+		"CIRCLECI_HOST":  cfg.EffectiveHost(),
 	}
 
 	// Best-effort: inject project metadata from git remote. Failures are
 	// silently ignored — the extension is responsible for handling missing vars.
 	if info, err := gitremote.Detect(); err == nil {
 		parts := strings.SplitN(info.Slug, "/", 3)
 		if len(parts) == 3 {
-			overlays["CIRCLE_VCS_TYPE"] = vcsLong(parts[0])
-			overlays["CIRCLE_PROJECT_USERNAME"] = parts[1]
-			overlays["CIRCLE_PROJECT_REPONAME"] = parts[2]
+			overlays["CIRCLECI_VCS_TYPE"] = vcsLong(parts[0])
+			overlays["CIRCLECI_PROJECT_USERNAME"] = parts[1]
+			overlays["CIRCLECI_PROJECT_REPONAME"] = parts[2]
 		}
 	}
 

main.go

@@ -28,7 +28,6 @@ import (
 	"fmt"
 	"os"
 	"os/signal"
-	"strings"
 	"syscall"
 
 	"github.com/CircleCI-Public/circleci-cli-v2/internal/cmd/root"
@@ -53,29 +52,37 @@ func run() int {
 	rootCmd := root.NewRootCmd(version)
 	rootCmd.SetContext(ctx)
 	if err := rootCmd.Execute(); err != nil {
-		// Before reporting "unknown command", try the implicit extension mechanism:
-		// any executable named "circleci-<name>" in PATH is a valid extension.
-		// Only intercept top-level unknown commands (error ends with `for "circleci"`).
-		if name, ok := rootUnknownCommand(err, rootCmd.Name()); ok {
-			configPath, _ := rootCmd.Flags().GetString("config")
-			extArgs := extensionArgs(name)
-			extErr := extension.Run(ctx, name, extArgs, configPath)
-			if extErr == nil {
-				return clierrors.ExitSuccess
-			}
-			// Extension not found — fall through and show the original cobra error.
-			var notFound *extension.ErrNotFound
-			if !errors.As(extErr, &notFound) {
-				if cliErr, ok := errors.AsType[*clierrors.CLIError](extErr); ok {
-					if jsonFlagPresent() {
-						_, _ = fmt.Fprint(os.Stderr, cliErr.FormatJSON())
-					} else {
-						_, _ = fmt.Fprint(os.Stderr, cliErr.Format())
+		// Try the implicit extension mechanism before reporting "unknown command".
+		// rootCmd.Flags().Args() gives the positional args cobra parsed; Find tells
+		// us whether the first one matches a registered subcommand or not.
+		if posArgs := rootCmd.Flags().Args(); len(posArgs) > 0 {
+			name := posArgs[0]
+			if found, _, _ := rootCmd.Find([]string{name}); found == rootCmd {
+				configPath, _ := rootCmd.Flags().GetString("config")
+				insecureStorage, _ := rootCmd.Flags().GetBool("insecure-storage")
+				extErr := extension.Run(ctx, name, posArgs[1:], configPath, !insecureStorage)
+				if extErr == nil {
+					return clierrors.ExitSuccess
+				}
+				if exited, ok := errors.AsType[*extension.ErrExited](extErr); ok {
+					if exited.Code > 0 {
+						return exited.Code
 					}
-					return cliErr.ExitCode
+					return clierrors.ExitGeneralError
+				}
+				// Extension not found — fall through and show the original cobra error.
+				if _, notFound := errors.AsType[*extension.ErrNotFound](extErr); !notFound {
+					if cliErr, ok := errors.AsType[*clierrors.CLIError](extErr); ok {
+						if jsonFlagPresent() {
+							_, _ = fmt.Fprint(os.Stderr, cliErr.FormatJSON())
+						} else {
+							_, _ = fmt.Fprint(os.Stderr, cliErr.Format())
+						}
+						return cliErr.ExitCode
+					}
+					_, _ = fmt.Fprintln(os.Stderr, extErr)
+					return clierrors.ExitGeneralError
 				}
-				_, _ = fmt.Fprintln(os.Stderr, extErr)
-				return clierrors.ExitGeneralError
 			}
 		}
 
@@ -93,71 +100,6 @@ func run() int {
 	return clierrors.ExitSuccess
 }
 
-// rootUnknownCommand reports whether err is Cobra's "unknown command" error
-// for the root command specifically (not a nested group). Returns the command
-// name that was not found.
-//
-// Note: this relies on Cobra's internal error string format. The suffix uses
-// rootName so binary renames (e.g. "cci") are handled correctly.
-func rootUnknownCommand(err error, rootName string) (string, bool) {
-	msg := err.Error()
-	// Cobra formats this as: unknown command "foo" for "circleci"
-	const prefix = `unknown command "`
-	if !strings.HasPrefix(msg, prefix) {
-		return "", false
-	}
-	// Must be for the root, not a subgroup like "circleci pipeline".
-	suffix := fmt.Sprintf(` for %q`, rootName)
-	if !strings.HasSuffix(msg, suffix) {
-		return "", false
-	}
-	name, _, ok := strings.Cut(msg[len(prefix):], `"`)
-	if !ok {
-		return "", false
-	}
-	return name, true
-}
-
-// globalFlagsWithValues is the set of root-level persistent flags that consume
-// the following token as their value (i.e. not boolean flags). We skip over
-// these and their values when scanning os.Args for the extension command name,
-// so that a flag value that happens to equal the extension name is not mistaken
-// for the positional command argument.
-var globalFlagsWithValues = map[string]bool{
-	"--config": true,
-	"-c":       true,
-	"--theme":  true,
-}
-
-// extensionArgs returns the arguments to pass to the extension binary.
-// It scans os.Args, skipping global flags and their values, and returns
-// everything after the first positional argument matching name.
-func extensionArgs(name string) []string {
-	args := os.Args[1:]
-	i := 0
-	for i < len(args) {
-		arg := args[i]
-		if strings.HasPrefix(arg, "-") {
-			if strings.Contains(arg, "=") {
-				// --flag=value form: single token, no separate value token
-				i++
-				continue
-			}
-			if globalFlagsWithValues[arg] {
-				i += 2 // skip flag and its value token
-				continue
-			}
-			i++ // boolean flag
-			continue
-		}
-		if arg == name {
-			return args[i+1:]
-		}
-		i++
-	}
-	return nil
-}
-
 // jsonFlagPresent reports whether --json appears anywhere in the raw argument
 // list. This is intentionally a simple scan rather than full flag parsing —
 // we only need it to format errors before Cobra has had a chance to run.