diff --git a/internal/cli/followup_profile_test.go b/internal/cli/followup_profile_test.go new file mode 100644 index 000000000..27d90e226 --- /dev/null +++ b/internal/cli/followup_profile_test.go @@ -0,0 +1,114 @@ +package cli + +import ( + "bytes" + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/commands" + "github.com/basecamp/basecamp-cli/internal/output" +) + +// A suggested follow-up is pasted into another shell, which may resolve a +// different profile. However this run chose its profile, the follow-up names it. +func TestFollowUpNamesTheProfileHoweverItWasChosen(t *testing.T) { + cases := map[string]struct { + config func(url string) string + env string + args []string + followUp string + }{ + "no profiles": { + config: func(url string) string { + return fmt.Sprintf(`{"base_url": %q, "account_id": "99999"}`, url) + }, + followUp: "basecamp unspotlight 456", + }, + "default_profile": { + config: profilesConfig("staging"), + followUp: "basecamp unspotlight 456 --profile staging", + }, + "BASECAMP_PROFILE": { + config: profilesConfig("production"), + env: "staging", + followUp: "basecamp unspotlight 456 --profile staging", + }, + "--profile": { + config: profilesConfig("production"), + args: []string{"--profile", "staging"}, + followUp: "basecamp unspotlight 456 --profile staging", + }, + } + for name, c := range cases { + t.Run(name, func(t *testing.T) { + var requested []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requested = append(requested, r.Method+" "+r.URL.Path) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":456,"type":"Message","title":"Kickoff"}`)) + })) + t.Cleanup(srv.Close) + + isolateRootTest(t) + t.Setenv("HOME", t.TempDir()) + t.Setenv("BASECAMP_TOKEN", "fake") + t.Setenv("BASECAMP_PROFILE", c.env) + t.Setenv("BASECAMP_ACCOUNT_ID", "") + t.Setenv("BASECAMP_BASE_URL", "") + dir := filepath.Join(os.Getenv("XDG_CONFIG_HOME"), "basecamp") + require.NoError(t, os.MkdirAll(dir, 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "config.json"), + []byte(c.config(srv.URL)), 0o600)) + + root := NewRootCmd() + root.AddCommand(commands.NewSpotlightCmd()) + root.SetArgs(append(c.args, "spotlight", "456", "--hints", "--json")) + root.SetOut(&bytes.Buffer{}) + root.SetErr(&bytes.Buffer{}) + stdout := captureStdout(t, func() { require.NoError(t, root.Execute()) }) + + var envelope output.Response + require.NoError(t, json.Unmarshal(stdout, &envelope), string(stdout)) + assert.Equal(t, []string{"POST /99999/recordings/456/spotlight.json"}, requested) + assert.Contains(t, envelope.Breadcrumbs, output.Breadcrumb{ + Action: "unspotlight", Cmd: c.followUp, Description: "Remove the spotlight", + }) + }) + } +} + +func profilesConfig(defaultProfile string) func(url string) string { + return func(url string) string { + return fmt.Sprintf(`{"default_profile": %q, "profiles": { + "production": {"base_url": %q, "account_id": "11111"}, + "staging": {"base_url": %q, "account_id": "99999"}}}`, defaultProfile, url, url) + } +} + +func captureStdout(t *testing.T, run func()) []byte { + t.Helper() + reader, writer, err := os.Pipe() + require.NoError(t, err) + orig := os.Stdout + os.Stdout = writer + t.Cleanup(func() { os.Stdout = orig }) + done := make(chan []byte) + go func() { + out, _ := io.ReadAll(reader) + done <- out + }() + run() + os.Stdout = orig + writer.Close() + return <-done +} diff --git a/internal/commands/comment.go b/internal/commands/comment.go index 0b03a5cee..8db4ad9c8 100644 --- a/internal/commands/comment.go +++ b/internal/commands/comment.go @@ -17,7 +17,6 @@ import ( "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/basecamp/basecamp-cli/internal/appctx" - "github.com/basecamp/basecamp-cli/internal/config" "github.com/basecamp/basecamp-cli/internal/editor" "github.com/basecamp/basecamp-cli/internal/hostutil" "github.com/basecamp/basecamp-cli/internal/output" @@ -316,13 +315,7 @@ You can pass either a comment ID or a Basecamp URL: } app := appctx.FromContext(cmd.Context()) - - // Whether the account is persistently available (on-disk config or env) - // before resolution. When it isn't — adopted from the URL, resolved - // interactively, or passed via the process-local --account flag — the - // advertised follow-ups must spell out --account so they remain runnable - // in a fresh process. - persistentAccount := hasPersistentAccount(app.Config) + scope := newFollowUpScope(app) // Same safe front door as `thread`: classify, guard the account, and // validate the ID — so a plain recording URL or wrong-account URL is @@ -331,7 +324,7 @@ You can pass either a comment ID or a Basecamp URL: if err != nil { return err } - accountArg := replyAccountArg(persistentAccount, app.Config.AccountID) + scopeArgs := scope.args(app.Config.AccountID) comment, err := app.Account().Comments().Get(cmd.Context(), commentID) if err != nil { @@ -376,14 +369,14 @@ You can pass either a comment ID or a Basecamp URL: breadcrumbs := make([]output.Breadcrumb, 0, 2) breadcrumbs = append(breadcrumbs, output.Breadcrumb{ Action: "update", - Cmd: fmt.Sprintf("basecamp comments update %d %s", commentID, accountArg), + Cmd: fmt.Sprintf("basecamp comments update %d %s", commentID, scopeArgs), Description: "Update comment", }) if comment.Parent != nil && comment.Parent.ID != 0 { m["reply_target"] = replyTarget(comment.Parent.ID, app.Config.AccountID) breadcrumbs = append(breadcrumbs, output.Breadcrumb{ Action: "reply", - Cmd: fmt.Sprintf("basecamp comments create %d %s", comment.Parent.ID, accountArg), + Cmd: fmt.Sprintf("basecamp comments create %d %s", comment.Parent.ID, scopeArgs), Description: "Reply on the parent recording", }) } @@ -451,16 +444,12 @@ func runCommentsThread(cmd *cobra.Command, arg string, all bool, window int) err // and `comments show` are equally safe: a plain recording URL / collection is // rejected toward `basecamp show`, a wrong-account URL and a non-positive ID // are rejected before any API call. - persistentAccount := hasPersistentAccount(app.Config) + scope := newFollowUpScope(app) triggerID, err := resolveCommentTrigger(cmd, app, arg) if err != nil { return err } - // When the account isn't persistently available (adopted from the URL, - // resolved interactively, or passed via the process-local --account flag), the - // advertised follow-ups must carry --account so they stay runnable in a fresh - // process. - accountArg := replyAccountArg(persistentAccount, app.Config.AccountID) + scopeArgs := scope.args(app.Config.AccountID) // Step 1 — resolve the focus comment. trigger, err := app.Account().Comments().Get(cmd.Context(), triggerID) @@ -555,13 +544,13 @@ func runCommentsThread(cmd *cobra.Command, arg string, all bool, window int) err if len(trigger.ContentAttachments) > 0 { breadcrumbs = append(breadcrumbs, output.Breadcrumb{ Action: "download", - Cmd: fmt.Sprintf("basecamp attachments download %d --type comment%s", trigger.ID, accountArg), + Cmd: fmt.Sprintf("basecamp attachments download %d --type comment%s", trigger.ID, scopeArgs), Description: "Download comment attachments", }) } breadcrumbs = append(breadcrumbs, output.Breadcrumb{ Action: "reply", - Cmd: fmt.Sprintf("basecamp comments create %s %s", parentIDStr, accountArg), + Cmd: fmt.Sprintf("basecamp comments create %s %s", parentIDStr, scopeArgs), Description: "Reply on the parent recording", }) @@ -668,37 +657,6 @@ func resolveCommentTrigger(cmd *cobra.Command, app *appctx.App, arg string) (int return id, nil } -// hasPersistentAccount reports whether the account is available to a fresh -// process without re-specifying it — i.e. backed by an on-disk config layer or -// an exported env var. A flag- or prompt-sourced account is process-local: it -// lives only for this invocation, so an advertised follow-up copied into a new -// process would fail to resolve it. Call this BEFORE resolveCommentTrigger, so a -// URL-adopted / interactively-resolved account (AccountID empty at entry) also -// reads as non-persistent. -func hasPersistentAccount(cfg *config.Config) bool { - if cfg.AccountID == "" { - return false - } - switch cfg.Sources["account_id"] { - case string(config.SourceFlag), string(config.SourcePrompt): - return false - default: - return true - } -} - -// replyAccountArg returns " --account " to append to an advertised -// follow-up command when the account is not persistently available (adopted from -// the URL, resolved interactively, or passed via the process-local --account -// flag this run), so the command stays runnable in a fresh process; otherwise -// "". A persistent account needs no echo — the follow-up inherits it. -func replyAccountArg(persistentAccount bool, accountID string) string { - if persistentAccount || accountID == "" { - return "" - } - return " --account " + accountID -} - // replyTarget builds the machine reply-target contract: the parent recording to // reply on, plus the account it belongs to so a consumer can construct a // fully-qualified reply regardless of its own configuration. diff --git a/internal/commands/comment_test.go b/internal/commands/comment_test.go index 9c40d2203..a2292ba1b 100644 --- a/internal/commands/comment_test.go +++ b/internal/commands/comment_test.go @@ -432,29 +432,29 @@ func TestCommentsShowRejectsZeroComment(t *testing.T) { // --- Iteration 2: account provenance in reply breadcrumbs ------------------- -func TestHasPersistentAccount(t *testing.T) { - mk := func(id, src string) *config.Config { - c := &config.Config{AccountID: id, Sources: map[string]string{}} - if src != "" { - c.Sources["account_id"] = src +func TestFollowUpScopeNamesWhatAFreshProcessLacks(t *testing.T) { + args := func(activeProfile, accountID, accountSource string) string { + app := &appctx.App{Config: &config.Config{ + ActiveProfile: activeProfile, AccountID: accountID, Sources: map[string]string{}, + }} + if accountSource != "" { + app.Config.Sources["account_id"] = accountSource } - return c + return newFollowUpScope(app).args("99999") } - // No account → not persistent. - assert.False(t, hasPersistentAccount(mk("", ""))) - // Process-local sources → not persistent (follow-ups must echo --account). - assert.False(t, hasPersistentAccount(mk("99999", string(config.SourceFlag)))) - assert.False(t, hasPersistentAccount(mk("99999", string(config.SourcePrompt)))) - // On-disk / exported-env sources → persistent (follow-ups inherit it). - assert.True(t, hasPersistentAccount(mk("99999", string(config.SourceGlobal)))) - assert.True(t, hasPersistentAccount(mk("99999", string(config.SourceLocal)))) - assert.True(t, hasPersistentAccount(mk("99999", string(config.SourceEnv)))) -} - -func TestReplyAccountArg(t *testing.T) { - assert.Equal(t, "", replyAccountArg(true, "99999")) // persistent → no echo - assert.Equal(t, "", replyAccountArg(false, "")) // nothing to echo - assert.Equal(t, " --account 99999", replyAccountArg(false, "99999")) // process-local → echo + // Account resolved after the scope was read (URL, picker) → named. + assert.Equal(t, " --account 99999", args("", "", "")) + // Process-local account → named. + assert.Equal(t, " --account 99999", args("", "99999", string(config.SourceFlag))) + assert.Equal(t, " --account 99999", args("", "99999", string(config.SourcePrompt))) + // Account a fresh process resolves again → inherited. + assert.Equal(t, "", args("", "99999", string(config.SourceGlobal))) + assert.Equal(t, "", args("", "99999", string(config.SourceLocal))) + assert.Equal(t, "", args("", "99999", string(config.SourceEnv))) + // The active profile is always named; its account is inherited through it. + assert.Equal(t, " --profile staging", args("staging", "99999", "profile")) + assert.Equal(t, " --profile staging --account 99999", args("staging", "99999", string(config.SourceFlag))) + assert.Equal(t, " --profile 'my profile' --account 99999", args("my profile", "", "")) } func TestCommentsShowFlagAccountEchoedInBreadcrumb(t *testing.T) { diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index bd06d2d10..3fa2e48dc 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -349,6 +349,42 @@ func ensureAccount(cmd *cobra.Command, app *appctx.App) error { return nil } +// followUpScope is what an advertised follow-up command must name to act on the +// same profile and account when run in a fresh process. The active profile is +// always named: the shell it is pasted into may have another BASECAMP_PROFILE, +// and a profile picked interactively is not picked again. An account given with +// --account, adopted from a URL or picked interactively lasts only for this run, +// so it is named too. Any other account comes back with the profile or config. +type followUpScope struct { + profile string + accountInherited bool +} + +// newFollowUpScope reads the scope before the command resolves its account, so +// an account adopted from a URL or picked interactively (empty at entry) reads +// as process-local. +func newFollowUpScope(app *appctx.App) followUpScope { + inherited := app.Config.AccountID != "" + switch app.Config.Sources["account_id"] { + case string(config.SourceFlag), string(config.SourcePrompt): + inherited = false + } + return followUpScope{profile: app.Config.ActiveProfile, accountInherited: inherited} +} + +// args returns the flags to append to a follow-up command, such as +// " --profile staging --account 123", or "" when it inherits both. +func (s followUpScope) args(accountID string) string { + args := "" + if s.profile != "" { + args += " --profile " + richtext.ShellQuote(s.profile) + } + if !s.accountInherited && accountID != "" { + args += " --account " + accountID + } + return args +} + // ensureProject resolves the project ID if not already configured. // This enables interactive prompts when --project flag and config are both missing. // The account must be resolved first (call ensureAccount before this). diff --git a/internal/commands/spotlight.go b/internal/commands/spotlight.go index 1b0919baf..1cf29d746 100644 --- a/internal/commands/spotlight.go +++ b/internal/commands/spotlight.go @@ -39,7 +39,7 @@ nothing. Requires edit access to the project.`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) - persistentAccount := hasPersistentAccount(app.Config) + scope := newFollowUpScope(app) recordingID, err := spotlightTarget(cmd, app, args[0]) if err != nil { return err @@ -50,18 +50,18 @@ nothing. Requires edit access to the project.`, return spotlightError(err) } - accountArg := replyAccountArg(persistentAccount, app.Config.AccountID) + scopeArgs := scope.args(app.Config.AccountID) return app.OK(recording, output.WithSummary(fmt.Sprintf("Spotlighted #%d on its project page", recordingID)), output.WithBreadcrumbs( output.Breadcrumb{ Action: "unspotlight", - Cmd: fmt.Sprintf("basecamp unspotlight %d%s", recordingID, accountArg), + Cmd: fmt.Sprintf("basecamp unspotlight %d%s", recordingID, scopeArgs), Description: "Remove the spotlight", }, output.Breadcrumb{ Action: "show", - Cmd: fmt.Sprintf("basecamp show %d%s", recordingID, accountArg), + Cmd: fmt.Sprintf("basecamp show %d%s", recordingID, scopeArgs), Description: "View item", }, ), @@ -90,7 +90,7 @@ nothing. Requires edit access to the project.`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) - persistentAccount := hasPersistentAccount(app.Config) + scope := newFollowUpScope(app) recordingID, err := spotlightTarget(cmd, app, args[0]) if err != nil { return err @@ -100,18 +100,18 @@ nothing. Requires edit access to the project.`, return convertSDKError(err) } - accountArg := replyAccountArg(persistentAccount, app.Config.AccountID) + scopeArgs := scope.args(app.Config.AccountID) return app.OK(map[string]any{"id": recordingID, "spotlighted": false}, output.WithSummary(fmt.Sprintf("Spotlight removed from #%d", recordingID)), output.WithBreadcrumbs( output.Breadcrumb{ Action: "spotlight", - Cmd: fmt.Sprintf("basecamp spotlight %d%s", recordingID, accountArg), + Cmd: fmt.Sprintf("basecamp spotlight %d%s", recordingID, scopeArgs), Description: "Spotlight it again", }, output.Breadcrumb{ Action: "show", - Cmd: fmt.Sprintf("basecamp show %d%s", recordingID, accountArg), + Cmd: fmt.Sprintf("basecamp show %d%s", recordingID, scopeArgs), Description: "View item", }, ), diff --git a/internal/commands/spotlight_test.go b/internal/commands/spotlight_test.go index 80c3e728e..fc83ff7a4 100644 --- a/internal/commands/spotlight_test.go +++ b/internal/commands/spotlight_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/config" "github.com/basecamp/basecamp-cli/internal/output" ) @@ -154,6 +155,68 @@ func TestSpotlightAdoptsTheURLAccount(t *testing.T) { } } +// A follow-up copied into a fresh shell must act on the same item. The shell +// may resolve another profile, and a profile picked interactively is not picked +// again, so the follow-up names the active profile however it was chosen. An +// account that came from the URL or --account is named as well. +func TestSpotlightFollowUpCarriesTheProfileAndAccount(t *testing.T) { + const itemURL = "https://app.basecamp.com/99999/buckets/123/messages/456" + cases := map[string]struct { + activeProfile string + accountSource string + arg string + followUp string + }{ + "no profile, account from config": { + accountSource: string(config.SourceGlobal), + arg: "456", + followUp: "basecamp unspotlight 456", + }, + "profile chosen without --profile": { + activeProfile: "staging", + accountSource: "profile", + arg: "456", + followUp: "basecamp unspotlight 456 --profile staging", + }, + "no profile, account from the URL": { + arg: itemURL, + followUp: "basecamp unspotlight 456 --account 99999", + }, + "profile, account from the URL": { + activeProfile: "staging", + arg: itemURL, + followUp: "basecamp unspotlight 456 --profile staging --account 99999", + }, + "profile, account from --account": { + activeProfile: "staging", + accountSource: string(config.SourceFlag), + arg: "456", + followUp: "basecamp unspotlight 456 --profile staging --account 99999", + }, + } + for name, c := range cases { + t.Run(name, func(t *testing.T) { + app, _ := setupRecordingTestApp(t, stubRoute{ + method: http.MethodPost, path: spotlightPath, status: http.StatusCreated, body: `{"id":456}`, + }) + app.Config.ActiveProfile = c.activeProfile + app.Config.Sources = map[string]string{} + if c.accountSource == "" { + app.Config.AccountID = "" + } else { + app.Config.Sources["account_id"] = c.accountSource + } + buf := captureSpotlightOutput(app) + + require.NoError(t, executeRecordingCommand(NewSpotlightCmd(), app, c.arg)) + + assert.Contains(t, decodeSpotlightEnvelope(t, buf).Breadcrumbs, output.Breadcrumb{ + Action: "unspotlight", Cmd: c.followUp, Description: "Remove the spotlight", + }) + }) + } +} + func TestSpotlightSurfacesARefusal(t *testing.T) { app, _ := setupRecordingTestApp(t, stubRoute{ method: http.MethodPost, path: spotlightPath, status: http.StatusUnprocessableEntity, diff --git a/internal/commands/templates.go b/internal/commands/templates.go index 39665d716..b10876490 100644 --- a/internal/commands/templates.go +++ b/internal/commands/templates.go @@ -14,7 +14,6 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/dateparse" "github.com/basecamp/basecamp-cli/internal/output" - "github.com/basecamp/basecamp-cli/internal/richtext" ) // NewTemplatesCmd creates the templates command for managing project and to-do list templates. @@ -146,15 +145,11 @@ func newTemplatesLibraryCmd() *cobra.Command { Long: "List the account's active to-do list templates.", RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) - persistentAccount := hasPersistentAccount(app.Config) + scope := newFollowUpScope(app) if err := ensureAccount(cmd, app); err != nil { return err } - contextArgs := templateCommandContextArgs( - app.Config.ActiveProfile, - persistentAccount, - app.Config.AccountID, - ) + contextArgs := scope.args(app.Config.AccountID) library, err := app.Account().Templates().GetLibraryTodolists(cmd.Context()) if err != nil { @@ -209,15 +204,11 @@ when --confirm-adding-people is explicitly provided.`, } app := appctx.FromContext(cmd.Context()) - persistentAccount := hasPersistentAccount(app.Config) + scope := newFollowUpScope(app) if err := ensureAccount(cmd, app); err != nil { return err } - contextArgs := templateCommandContextArgs( - app.Config.ActiveProfile, - persistentAccount, - app.Config.AccountID, - ) + contextArgs := scope.args(app.Config.AccountID) resolvedProjectID, err := resolveProjectID(cmd, app, project) if err != nil { @@ -293,14 +284,6 @@ func validateTemplateCopyTodoset(cmd *cobra.Command, app *appctx.App, todosetID, return output.ErrUsage(fmt.Sprintf("--todoset %s is not a To-dos tool in project %s", todosetID, projectID)) } -func templateCommandContextArgs(profile string, persistentAccount bool, accountID string) string { - args := "" - if profile != "" { - args += " --profile " + richtext.ShellQuote(profile) - } - return args + replyAccountArg(persistentAccount, accountID) -} - func templateCopyError(err error, templateID int64, projectID, todosetID, contextArgs string) error { var confirmationErr *basecamp.PeopleConfirmationRequiredError if !errors.As(err, &confirmationErr) { @@ -338,15 +321,11 @@ func newTemplatesCopyStatusCmd() *cobra.Command { } app := appctx.FromContext(cmd.Context()) - persistentAccount := hasPersistentAccount(app.Config) + scope := newFollowUpScope(app) if err := ensureAccount(cmd, app); err != nil { return err } - contextArgs := templateCommandContextArgs( - app.Config.ActiveProfile, - persistentAccount, - app.Config.AccountID, - ) + contextArgs := scope.args(app.Config.AccountID) templateCopy, err := app.Account().Templates().GetLibraryCopy(cmd.Context(), copyID) if err != nil {