One shell-quoting implementation, not four

internal/auth/agent.go

@@ -15,6 +15,7 @@ import (
 
 	"github.com/basecamp/basecamp-cli/internal/config"
 	"github.com/basecamp/basecamp-cli/internal/output"
+	"github.com/basecamp/basecamp-cli/internal/richtext"
 )
 
 // Agent self-tokens: the RFC 6749 §4.4 client_credentials grant.
@@ -465,7 +466,7 @@ func (m *Manager) accountToBind() string {
 	source := config.Source(m.cfg.Sources["account_id"])
 	supplied := source == config.SourceFlag || source == config.SourceEnv
 	if supplied && m.cfg.AccountID != "" {
-		return shellQuote(m.cfg.AccountID)
+		return richtext.ShellQuote(m.cfg.AccountID)
 	}
 	return "<account-id>"
 }
@@ -511,11 +512,11 @@ func (m *Manager) agentRemedy(err error, clientID, scope string) error {
 func (m *Manager) agentLoginCommand(clientID, scope string) string {
 	id := "<client-id>"
 	if clientID != "" {
-		id = shellQuote(clientID)
+		id = richtext.ShellQuote(clientID)
 	}
 	profile := "<profile>"
 	if m.cfg.ActiveProfile != "" {
-		profile = shellQuote(m.cfg.ActiveProfile)
+		profile = richtext.ShellQuote(m.cfg.ActiveProfile)
 	}
 	command := "... | basecamp auth login --with-client-credentials --client-id " + id + " -P " + profile
 	// A profile with no entry yet is one the login creates, and creating
@@ -538,7 +539,7 @@ func (m *Manager) agentLoginCommand(clientID, scope string) string {
 	// back with full access, or be refused for asking for more than its
 	// client is allowed. full is the default and adding it says nothing.
 	if scope != "" && scope != scopeFull {
-		command += " --scope " + shellQuote(scope)
+		command += " --scope " + richtext.ShellQuote(scope)
 	}
 	return command
 }

internal/auth/auth.go

@@ -192,35 +192,11 @@ func (m *Manager) loginRemedy() (command, lead string) {
 		return m.agentLoginCommand(kind.clientID, kind.scope), "Pipe the agent's client secret in:"
 	}
 	if m.cfg.ActiveProfile != "" {
-		return "basecamp auth login -P " + shellQuote(m.cfg.ActiveProfile), "Run:"
+		return "basecamp auth login -P " + richtext.ShellQuote(m.cfg.ActiveProfile), "Run:"
 	}
 	return "basecamp auth login", "Run:"
 }
 
-// shellQuote renders s safe to embed in an emitted shell command: a clearly
-// inert name passes through bare, anything else is single-quoted — the one
-// POSIX form in which nothing substitutes — with embedded single quotes
-// spelled:
-//
-//	'\''
-//
-// indented so gofmt leaves it as written. Profile names come from
-// configuration files, which do not
-// apply the create-time name check.
-func shellQuote(s string) string {
-	if s != "" && strings.IndexFunc(s, shellActive) < 0 {
-		return s
-	}
-	return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'"
-}
-
-// shellActive reports whether r can mean anything to a POSIX shell outside
-// quotes; letters, digits and a few inert punctuation marks cannot.
-func shellActive(r rune) bool {
-	inert := r >= 'a' && r <= 'z' || r >= 'A' && r <= 'Z' || r >= '0' && r <= '9' || strings.ContainsRune("_./:@%+=-", r)
-	return !inert
-}
-
 // remember records what the credential in play is, for the remedy an
 // error will carry. Every Manager path that gets its hands on a
 // credential — serving a token, renewing one, storing one, answering

internal/commands/config.go

@@ -16,6 +16,7 @@ import (
 	"github.com/basecamp/basecamp-cli/internal/config"
 	"github.com/basecamp/basecamp-cli/internal/hostutil"
 	"github.com/basecamp/basecamp-cli/internal/output"
+	"github.com/basecamp/basecamp-cli/internal/richtext"
 	"github.com/basecamp/basecamp-cli/internal/tui/resolve"
 )
 
@@ -361,7 +362,7 @@ Valid keys: account_id, project_id (or project), todolist_id, base_url, cache_di
 				absPath, _ := filepath.Abs(configPath)
 				ts := config.LoadTrustStore(config.GlobalConfigDir())
 				if ts == nil || !ts.IsTrusted(configPath) {
-					fmt.Fprintf(os.Stderr, "warning: %q in local config requires trust to take effect; run:\n  basecamp config trust %s\n", key, config.ShellQuote(absPath))
+					fmt.Fprintf(os.Stderr, "warning: %q in local config requires trust to take effect; run:\n  basecamp config trust %s\n", key, richtext.ShellQuote(absPath))
 				}
 			}
 

internal/commands/config_test.go

@@ -18,6 +18,7 @@ import (
 	"github.com/basecamp/basecamp-cli/internal/config"
 	"github.com/basecamp/basecamp-cli/internal/names"
 	"github.com/basecamp/basecamp-cli/internal/output"
+	"github.com/basecamp/basecamp-cli/internal/richtext"
 )
 
 func TestAtomicWriteFile_OverwriteExisting(t *testing.T) {
@@ -310,37 +311,73 @@ func TestIsAuthorityKey(t *testing.T) {
 
 // --- config set authority-key warning test ---
 
+// The warning names a `basecamp config trust <path>` a person pastes, so the
+// path in it is shell-quoted.
+//
+// This used to assert single quotes around the path unconditionally, because
+// the copy of the quoting this package called wrapped every value whether or
+// not it needed it. The shared richtext.ShellQuote leaves a value that can
+// mean nothing to a shell alone, so on a POSIX filesystem an ordinary path
+// now appears bare — the same word, spelled shorter. What "for shell safety"
+// was actually claiming is the second case: a path that does carry shell
+// syntax is encoded, and an apostrophe in it is spliced rather than merely
+// wrapped, which is the one spelling a wrapper gets wrong.
+//
+// The expectation is asked of richtext.ShellQuote rather than spelled out,
+// because this path is absolute and so is rooted in whatever the host calls
+// its temporary directory — a spelling that may hold apostrophes or spaces
+// of its own, and that is different again on Windows. Comparing against the
+// encoder means the same thing on every machine. Whether an ordinary path
+// comes out bare cannot be asked of an absolute path for that reason; it is
+// pinned against a fixed relative fixture in
+// internal/config/trust_unix_test.go.
 func TestConfigSet_AuthorityKeyWarnsWithPath(t *testing.T) {
-	app, _ := setupConfigTestApp(t)
-
-	// Work in a temp dir so config set writes .basecamp/config.json there
-	tmpDir, _ := filepath.EvalSymlinks(t.TempDir())
-	origDir, _ := os.Getwd()
-	require.NoError(t, os.Chdir(tmpDir))
-	defer os.Chdir(origDir)
-
-	require.NoError(t, os.MkdirAll(".basecamp", 0755))
-
-	// Capture stderr for the warning
-	origStderr := os.Stderr
-	r, w, _ := os.Pipe()
-	os.Stderr = w
-
-	err := executeConfigCommand(app, "set", "base_url", "https://custom.example.com")
-	require.NoError(t, err)
-
-	w.Close()
-	var buf [4096]byte
-	n, _ := r.Read(buf[:])
-	os.Stderr = origStderr
-
-	stderr := string(buf[:n])
-	absPath := filepath.Join(tmpDir, ".basecamp", "config.json")
-
-	assert.Contains(t, stderr, `"base_url"`)
-	assert.Contains(t, stderr, "requires trust")
-	assert.Contains(t, stderr, absPath, "warning must include the exact config path")
-	assert.Contains(t, stderr, "'"+absPath+"'", "path must be single-quoted for shell safety")
+	for name, dir := range map[string]string{
+		"an ordinary path":             "plain",
+		"a path with an apostrophe in": "o'brien",
+	} {
+		t.Run(name, func(t *testing.T) {
+			app, _ := setupConfigTestApp(t)
+
+			// Work in a temp dir so config set writes .basecamp/config.json there
+			tmpDir, _ := filepath.EvalSymlinks(t.TempDir())
+			tmpDir = filepath.Join(tmpDir, dir)
+			require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, ".basecamp"), 0o755))
+			origDir, _ := os.Getwd()
+			require.NoError(t, os.Chdir(tmpDir))
+			defer os.Chdir(origDir)
+
+			// Capture stderr for the warning
+			origStderr := os.Stderr
+			r, w, _ := os.Pipe()
+			os.Stderr = w
+
+			err := executeConfigCommand(app, "set", "base_url", "https://custom.example.com")
+			require.NoError(t, err)
+
+			w.Close()
+			var buf [4096]byte
+			n, _ := r.Read(buf[:])
+			os.Stderr = origStderr
+
+			stderr := string(buf[:n])
+			absPath := filepath.Join(tmpDir, ".basecamp", "config.json")
+
+			assert.Contains(t, stderr, `"base_url"`)
+			assert.Contains(t, stderr, "requires trust")
+			assert.Contains(t, stderr, "basecamp config trust "+richtext.ShellQuote(absPath),
+				"the warning must name the command with the path encoded for a shell")
+			// Only the apostrophe fixture gets extra assertions. The
+			// ordinary one is covered by the exact ShellQuote comparison
+			// above; asking it for a raw substring would be asking about
+			// TMPDIR, which may hold an apostrophe of its own and would
+			// then correctly be spliced (Copilot on #769).
+			if dir != "plain" {
+				assert.NotContains(t, stderr, absPath, "a path with an apostrophe cannot appear raw")
+				assert.Contains(t, stderr, `'\''`, "the apostrophe must be spliced out and back in")
+			}
+		})
+	}
 }
 
 // TestConfigSet_GatedNonAuthorityKeyWarns verifies the trust warning also fires

internal/commands/connect.go

@@ -130,7 +130,7 @@ func runConnectShow(app *appctx.App) error {
 	switch {
 	case errors.Is(err, os.ErrNotExist):
 		return output.ErrNotFoundHint("connect.json for profile", name,
-			"The profile has not been set up. Set it up: basecamp connect setup -P "+shellQuote(name)+" --operator-profile '<your profile>' --serve <project-id>")
+			"The profile has not been set up. Set it up: basecamp connect setup -P "+richtext.ShellQuote(name)+" --operator-profile '<your profile>' --serve <project-id>")
 	case err != nil && runtime.GOOS == "windows":
 		return output.ErrUsageHint("connect.json cannot be used: "+setup.ErrorText(err),
 			"The connector's setup is not supported on Windows: this CLI cannot verify who can change connect.json there.")
@@ -618,7 +618,7 @@ func runConnectSetup(cmd *cobra.Command, app *appctx.App, f *connectSetupFlags)
 // connect.json nobody else may change is usage.
 func classifyWriteError(name string, err error) error {
 	var apiErr *output.Error
-	profile := shellQuote(name)
+	profile := richtext.ShellQuote(name)
 	switch {
 	case errors.Is(err, context.Canceled), errors.Is(err, context.DeadlineExceeded):
 		// A person who stopped the command, or a deadline: the run ended,
@@ -853,7 +853,7 @@ func parsePositiveID(flag, raw string) (int64, error) {
 // against an Agent's credential or connect.json, and a person's login with
 // nothing pinning it to the bot.
 func refuseCredentialConflicts(name, path, held string, expect int64, exists bool, existing setup.File) error {
-	profile := shellQuote(name)
+	profile := richtext.ShellQuote(name)
 	switch {
 	// What connect.json already says comes first: a remediation that the
 	// existing file would refuse anyway is no remediation.
@@ -891,7 +891,7 @@ func connectAccount(app *appctx.App, name string) (string, error) {
 	p := app.Config.Profiles[name]
 	if p == nil {
 		return "", output.ErrUsageHint(fmt.Sprintf("Profile %q does not exist", name),
-			"Connect the agent first: basecamp auth agent connect -P "+shellQuote(name))
+			"Connect the agent first: basecamp auth agent connect -P "+richtext.ShellQuote(name))
 	}
 	bound, err := canonicalAccount(p.AccountID)
 	if err != nil {
@@ -937,7 +937,7 @@ func unboundProfileError(cfg *config.Config, name string) error {
 	}
 	global := filepath.Join(config.GlobalConfigDir(), "config.json")
 	return output.ErrUsageHint(message,
-		"For an Agent, connecting it binds its own account: basecamp auth agent connect -P "+shellQuote(name)+
+		"For an Agent, connecting it binds its own account: basecamp auth agent connect -P "+richtext.ShellQuote(name)+
 			". A bot user's browser login binds none, so for a bot add account_id to the profile's entry in "+richtext.SanitizeSingleLine(global)+".")
 }
 
@@ -1036,7 +1036,7 @@ func operatorProfileManager(ctx context.Context, app *appctx.App, profile string
 	case err != nil:
 		return nil, err
 	case kind == "":
-		return nil, output.ErrUsageHint(fmt.Sprintf("Operator profile %q holds no credential", profile), "Log in: basecamp auth login -P "+shellQuote(profile))
+		return nil, output.ErrUsageHint(fmt.Sprintf("Operator profile %q holds no credential", profile), "Log in: basecamp auth login -P "+richtext.ShellQuote(profile))
 	case kind == setup.KindAgent:
 		return nil, output.ErrUsage(fmt.Sprintf("Operator profile %q holds an Agent's credential; an operator is a person", profile))
 	}

internal/commands/connect_doctor.go

@@ -170,7 +170,7 @@ func ledgerChecks(ctx context.Context, p connectProfile) []setup.Check {
 	if s.Hold != nil {
 		checks = append(checks, setup.Check{Name: "Hold", Status: setup.StatusWarn,
 			Message: fmt.Sprintf("Held since %s by %s: nothing dispatches or posts", s.Hold.HeldAt.UTC().Format(time.RFC3339), richtext.SanitizeSingleLine(s.Hold.HeldBy)),
-			Hint:    "Review held records in basecamp connect status, then basecamp connect release -P " + shellQuote(p.name)})
+			Hint:    "Review held records in basecamp connect status, then basecamp connect release -P " + richtext.ShellQuote(p.name)})
 	}
 	if len(s.Indeterminate) > 0 {
 		checks = append(checks, setup.Check{Name: "Lifecycle messages", Status: setup.StatusWarn,
@@ -373,7 +373,7 @@ func driverChecks(p connectProfile) []setup.Check {
 	if p.file.Driver != setup.DriverSpawn && p.file.Driver != setup.DriverACP {
 		checks = append(checks, setup.Check{Name: "Driver", Status: setup.StatusFail,
 			Message: fmt.Sprintf("Driver %q is not %q or %q, and the connector refuses to start on it", p.file.Driver, setup.DriverSpawn, setup.DriverACP),
-			Hint:    "basecamp connect setup -P " + shellQuote(p.name) + " --driver spawn"})
+			Hint:    "basecamp connect setup -P " + richtext.ShellQuote(p.name) + " --driver spawn"})
 	}
 	return checks
 }

internal/commands/connect_doctor_mcp_unix.go

@@ -14,6 +14,7 @@ import (
 	"github.com/basecamp/basecamp-cli/internal/connector"
 	"github.com/basecamp/basecamp-cli/internal/connector/driver"
 	"github.com/basecamp/basecamp-cli/internal/connector/setup"
+	"github.com/basecamp/basecamp-cli/internal/richtext"
 	"github.com/basecamp/basecamp-cli/internal/version"
 )
 
@@ -83,7 +84,7 @@ func mcpHandshakeCheck(ctx context.Context, profile string) setup.Check {
 	if err != nil {
 		session = nil
 		c.Status, c.Message = setup.StatusFail, "The agent's MCP server did not complete the handshake: "+setup.ErrorText(err)
-		c.Hint = "Run basecamp mcp -P " + shellQuote(profile) + " and read its stderr."
+		c.Hint = "Run basecamp mcp -P " + richtext.ShellQuote(profile) + " and read its stderr."
 		return c
 	}
 	tools := 0
@@ -98,6 +99,6 @@ func mcpHandshakeCheck(ctx context.Context, profile string) setup.Check {
 		c.Status, c.Message = setup.StatusFail, "The agent's MCP server lists no tools"
 		return c
 	}
-	c.Status, c.Message = setup.StatusPass, fmt.Sprintf("The agent's MCP server (basecamp mcp -P %s) answered with %d tools; the basecamp_connect domain is served only to a dispatched worker", shellQuote(profile), tools)
+	c.Status, c.Message = setup.StatusPass, fmt.Sprintf("The agent's MCP server (basecamp mcp -P %s) answered with %d tools; the basecamp_connect domain is served only to a dispatched worker", richtext.ShellQuote(profile), tools)
 	return c
 }

internal/commands/connect_operator.go

@@ -71,7 +71,7 @@ func loadConnectProfile(cmd *cobra.Command) (connectProfile, error) {
 	file, err := setup.Load(path)
 	switch {
 	case errors.Is(err, os.ErrNotExist):
-		return connectProfile{}, output.ErrUsageHint(fmt.Sprintf("Profile %q is not set up as a connector", name), "Run: basecamp connect setup -P "+shellQuote(name))
+		return connectProfile{}, output.ErrUsageHint(fmt.Sprintf("Profile %q is not set up as a connector", name), "Run: basecamp connect setup -P "+richtext.ShellQuote(name))
 	case err != nil:
 		return connectProfile{}, output.ErrUsage("connect.json cannot be used: " + err.Error())
 	}
@@ -119,7 +119,7 @@ func openConnectLedger(ctx context.Context, p connectProfile, requireStopped boo
 	}
 	path := filepath.Join(dir, connector.LedgerFile)
 	if _, err := os.Lstat(path); errors.Is(err, os.ErrNotExist) {
-		return nil, done, output.ErrUsageHint(fmt.Sprintf("Profile %q's connector has no ledger yet", p.name), "Run the connector first: basecamp connect -P "+shellQuote(p.name))
+		return nil, done, output.ErrUsageHint(fmt.Sprintf("Profile %q's connector has no ledger yet", p.name), "Run the connector first: basecamp connect -P "+richtext.ShellQuote(p.name))
 	}
 	// Opening for a decision migrates the ledger, and a connector running on
 	// an older binary's schema must not have its triggers replaced under it.
@@ -232,9 +232,9 @@ func runConnectStatus(cmd *cobra.Command, shadow bool) error {
 	ledger, err := connector.OpenLedgerReadOnly(cmd.Context(), filepath.Join(dir, connector.LedgerFile))
 	if errors.Is(err, os.ErrNotExist) {
 		if shadow {
-			return output.ErrUsageHint(fmt.Sprintf("Profile %q has no shadow ledger", p.name), "Run the shadow connector first: basecamp connect -P "+shellQuote(p.name)+" --shadow")
+			return output.ErrUsageHint(fmt.Sprintf("Profile %q has no shadow ledger", p.name), "Run the shadow connector first: basecamp connect -P "+richtext.ShellQuote(p.name)+" --shadow")
 		}
-		return output.ErrUsageHint(fmt.Sprintf("Profile %q's connector has no ledger yet", p.name), "Run the connector first: basecamp connect -P "+shellQuote(p.name))
+		return output.ErrUsageHint(fmt.Sprintf("Profile %q's connector has no ledger yet", p.name), "Run the connector first: basecamp connect -P "+richtext.ShellQuote(p.name))
 	}
 	if err != nil {
 		return err
@@ -764,7 +764,7 @@ The file is JSON:
 				return err
 			}
 			if _, err := os.Lstat(filepath.Join(dir, connector.LedgerFile)); errors.Is(err, os.ErrNotExist) {
-				return output.ErrUsageHint(fmt.Sprintf("Profile %q's connector has no ledger yet", p.name), "Promote the shadow first: basecamp connect shadow promote -P "+shellQuote(p.name))
+				return output.ErrUsageHint(fmt.Sprintf("Profile %q's connector has no ledger yet", p.name), "Promote the shadow first: basecamp connect shadow promote -P "+richtext.ShellQuote(p.name))
 			}
 			// The connector must be stopped: the open takes the instance lock
 			// and holds it for the import.

internal/commands/feed.go

@@ -15,6 +15,7 @@ import (
 
 	"github.com/basecamp/basecamp-cli/internal/appctx"
 	"github.com/basecamp/basecamp-cli/internal/output"
+	"github.com/basecamp/basecamp-cli/internal/richtext"
 )
 
 // The account event feed: the catch-up poll lane (`events poll`), the agent
@@ -549,7 +550,7 @@ func (r feedRequest) resumeNotice(position string, capped bool, maxPages int) st
 // into a command somebody pastes, and nothing about it promises to be a bare
 // word.
 func (r feedRequest) positionCommand(position string) string {
-	return fmt.Sprintf("%s --position %s%s", r.lane.pollCmd, shellQuote(position), r.filters)
+	return fmt.Sprintf("%s --position %s%s", r.lane.pollCmd, richtext.ShellQuote(position), r.filters)
 }
 
 // newEventsPollCmd builds `basecamp events poll`.