From e8e9cb0fefb9dcd87abe11b9066636b676e804c7 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Thu, 22 Oct 2026 20:04:39 -0400 Subject: [PATCH 1/3] Test that spotlight follow-ups carry a --profile chosen this run Three cases fail: a profile chosen with --profile is dropped from the suggested follow-up, so a fresh shell runs it under the default profile. --- internal/commands/spotlight_test.go | 65 +++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/internal/commands/spotlight_test.go b/internal/commands/spotlight_test.go index 80c3e728e..a1923d32d 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,70 @@ func TestSpotlightAdoptsTheURLAccount(t *testing.T) { } } +// A follow-up copied into a fresh shell must act on the same item. A profile +// chosen with --profile lasts only for this run, so the follow-up names it. A +// profile a fresh process picks again, from default_profile or an exported +// BASECAMP_PROFILE, needs no flag: the command never sees which of the two +// chose it, only that --profile did not. +func TestSpotlightFollowUpCarriesTheProfileAndAccount(t *testing.T) { + const itemURL = "https://app.basecamp.com/99999/buckets/123/messages/456" + cases := map[string]struct { + profileFlag string + accountSource string + arg string + followUp string + }{ + "profile and account from config": { + accountSource: "profile", + arg: "456", + followUp: "basecamp unspotlight 456", + }, + "profile from --profile": { + profileFlag: "staging", + accountSource: "profile", + arg: "456", + followUp: "basecamp unspotlight 456 --profile staging", + }, + "account from the URL": { + arg: itemURL, + followUp: "basecamp unspotlight 456 --account 99999", + }, + "account from the URL under --profile": { + profileFlag: "staging", + arg: itemURL, + followUp: "basecamp unspotlight 456 --profile staging --account 99999", + }, + "account from --account under --profile": { + profileFlag: "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.Flags.Profile = c.profileFlag + app.Config.ActiveProfile = "staging" + 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, From 7f8c97e2bc68ae3d8c3eeaf637447025eeeb56d9 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Thu, 22 Oct 2026 20:10:14 -0400 Subject: [PATCH 2/3] Carry a --profile chosen this run in follow-up commands followUpScope replaces hasPersistentAccount, replyAccountArg and templateCommandContextArgs. It names --profile when the profile came from the flag, and --account when the account would not resolve again in a fresh process. A profile from default_profile or BASECAMP_PROFILE is left out, as an account from config or env already was. Template follow-ups used to name every active profile. They now follow the same rule as the rest. --- internal/commands/comment.go | 58 ++++------------------------- internal/commands/comment_test.go | 42 ++++++++++----------- internal/commands/helpers.go | 36 ++++++++++++++++++ internal/commands/spotlight.go | 16 ++++---- internal/commands/templates.go | 33 +++------------- internal/commands/templates_test.go | 12 ++++-- 6 files changed, 87 insertions(+), 110 deletions(-) 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..167bd8029 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(profileFlag, accountID, accountSource string) string { + app := &appctx.App{Config: &config.Config{AccountID: accountID, Sources: map[string]string{}}} + app.Flags.Profile = profileFlag + 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))) + assert.Equal(t, "", args("", "99999", "profile")) + // --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..968516dec 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. A profile chosen with +// --profile lasts only for this run, as does an account given with --account, +// adopted from a URL or picked interactively. A fresh process resolves config, +// default_profile and exported env vars again, so those are left out. +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. An account from a --profile profile is inherited, because +// the follow-up names that profile. +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.Flags.Profile, 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/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 { diff --git a/internal/commands/templates_test.go b/internal/commands/templates_test.go index 8e484ebd0..d87fb7c77 100644 --- a/internal/commands/templates_test.go +++ b/internal/commands/templates_test.go @@ -73,7 +73,8 @@ func TestTemplatesLibraryUsesSDKLibraryEndpoint(t *testing.T) { body: templateLibraryJSON, }) buf := captureTemplateOutput(app) - app.Config.ActiveProfile = "work profile" + app.Flags.Profile = "work profile" + app.Config.ActiveProfile = app.Flags.Profile app.Config.Sources = map[string]string{"account_id": string(config.SourceFlag)} err := executeRecordingCommand(NewTemplatesCmd(), app, "library") @@ -117,7 +118,8 @@ func TestTemplatesCopyResolvesProjectAndTodosTool(t *testing.T) { }, ) buf := captureTemplateOutput(app) - app.Config.ActiveProfile = "work profile" + app.Flags.Profile = "work profile" + app.Config.ActiveProfile = app.Flags.Profile app.Config.Sources = map[string]string{"account_id": string(config.SourceFlag)} err := executeRecordingCommand(NewTemplatesCmd(), app, "copy", "3", "--in", "Test Project") @@ -212,7 +214,8 @@ func TestTemplatesCopyExplainsPeopleConfirmation(t *testing.T) { }, ) - app.Config.ActiveProfile = "work profile; echo unsafe" + app.Flags.Profile = "work profile; echo unsafe" + app.Config.ActiveProfile = app.Flags.Profile app.Config.Sources = map[string]string{"account_id": string(config.SourceFlag)} err := executeRecordingCommand(NewTemplatesCmd(), app, "copy", "3", "--in", "123") @@ -353,7 +356,8 @@ func TestTemplatesCopyStatusStates(t *testing.T) { body: test.body, }) buf := captureTemplateOutput(app) - app.Config.ActiveProfile = "work profile" + app.Flags.Profile = "work profile" + app.Config.ActiveProfile = app.Flags.Profile app.Config.Sources = map[string]string{"account_id": string(config.SourceFlag)} err := executeRecordingCommand(NewTemplatesCmd(), app, "copy-status", "5") From 43ac084b1ea5bee4e4f57bf9d3545be5c2beaacb Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Thu, 22 Oct 2026 20:33:53 -0400 Subject: [PATCH 3/3] Name the active profile in follow-up commands A profile picked from the interactive picker never set the --profile flag, so follow-ups left it out. Pasted into a shell, they asked for a profile again, or failed when not interactive. Template follow-ups had named the active profile before, so they regressed. Follow-ups now name the active profile however it was chosen. That also keeps them right in a shell that exports another BASECAMP_PROFILE, at the cost of a redundant flag when the profile came from default_profile or the environment. Users without profiles see no change. --- internal/cli/followup_profile_test.go | 114 ++++++++++++++++++++++++++ internal/commands/comment_test.go | 10 +-- internal/commands/helpers.go | 14 ++-- internal/commands/spotlight_test.go | 36 ++++---- internal/commands/templates_test.go | 12 +-- 5 files changed, 147 insertions(+), 39 deletions(-) create mode 100644 internal/cli/followup_profile_test.go 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_test.go b/internal/commands/comment_test.go index 167bd8029..a2292ba1b 100644 --- a/internal/commands/comment_test.go +++ b/internal/commands/comment_test.go @@ -433,9 +433,10 @@ func TestCommentsShowRejectsZeroComment(t *testing.T) { // --- Iteration 2: account provenance in reply breadcrumbs ------------------- func TestFollowUpScopeNamesWhatAFreshProcessLacks(t *testing.T) { - args := func(profileFlag, accountID, accountSource string) string { - app := &appctx.App{Config: &config.Config{AccountID: accountID, Sources: map[string]string{}}} - app.Flags.Profile = profileFlag + 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 } @@ -450,8 +451,7 @@ func TestFollowUpScopeNamesWhatAFreshProcessLacks(t *testing.T) { assert.Equal(t, "", args("", "99999", string(config.SourceGlobal))) assert.Equal(t, "", args("", "99999", string(config.SourceLocal))) assert.Equal(t, "", args("", "99999", string(config.SourceEnv))) - assert.Equal(t, "", args("", "99999", "profile")) - // --profile is always named; its account is inherited through it. + // 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", "", "")) diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index 968516dec..3fa2e48dc 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -350,10 +350,11 @@ func ensureAccount(cmd *cobra.Command, app *appctx.App) error { } // followUpScope is what an advertised follow-up command must name to act on the -// same profile and account when run in a fresh process. A profile chosen with -// --profile lasts only for this run, as does an account given with --account, -// adopted from a URL or picked interactively. A fresh process resolves config, -// default_profile and exported env vars again, so those are left out. +// 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 @@ -361,15 +362,14 @@ type followUpScope struct { // 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. An account from a --profile profile is inherited, because -// the follow-up names that profile. +// 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.Flags.Profile, accountInherited: inherited} + return followUpScope{profile: app.Config.ActiveProfile, accountInherited: inherited} } // args returns the flags to append to a follow-up command, such as diff --git a/internal/commands/spotlight_test.go b/internal/commands/spotlight_test.go index a1923d32d..fc83ff7a4 100644 --- a/internal/commands/spotlight_test.go +++ b/internal/commands/spotlight_test.go @@ -155,41 +155,40 @@ func TestSpotlightAdoptsTheURLAccount(t *testing.T) { } } -// A follow-up copied into a fresh shell must act on the same item. A profile -// chosen with --profile lasts only for this run, so the follow-up names it. A -// profile a fresh process picks again, from default_profile or an exported -// BASECAMP_PROFILE, needs no flag: the command never sees which of the two -// chose it, only that --profile did not. +// 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 { - profileFlag string + activeProfile string accountSource string arg string followUp string }{ - "profile and account from config": { - accountSource: "profile", + "no profile, account from config": { + accountSource: string(config.SourceGlobal), arg: "456", followUp: "basecamp unspotlight 456", }, - "profile from --profile": { - profileFlag: "staging", + "profile chosen without --profile": { + activeProfile: "staging", accountSource: "profile", arg: "456", followUp: "basecamp unspotlight 456 --profile staging", }, - "account from the URL": { + "no profile, account from the URL": { arg: itemURL, followUp: "basecamp unspotlight 456 --account 99999", }, - "account from the URL under --profile": { - profileFlag: "staging", - arg: itemURL, - followUp: "basecamp unspotlight 456 --profile staging --account 99999", + "profile, account from the URL": { + activeProfile: "staging", + arg: itemURL, + followUp: "basecamp unspotlight 456 --profile staging --account 99999", }, - "account from --account under --profile": { - profileFlag: "staging", + "profile, account from --account": { + activeProfile: "staging", accountSource: string(config.SourceFlag), arg: "456", followUp: "basecamp unspotlight 456 --profile staging --account 99999", @@ -200,8 +199,7 @@ func TestSpotlightFollowUpCarriesTheProfileAndAccount(t *testing.T) { app, _ := setupRecordingTestApp(t, stubRoute{ method: http.MethodPost, path: spotlightPath, status: http.StatusCreated, body: `{"id":456}`, }) - app.Flags.Profile = c.profileFlag - app.Config.ActiveProfile = "staging" + app.Config.ActiveProfile = c.activeProfile app.Config.Sources = map[string]string{} if c.accountSource == "" { app.Config.AccountID = "" diff --git a/internal/commands/templates_test.go b/internal/commands/templates_test.go index d87fb7c77..8e484ebd0 100644 --- a/internal/commands/templates_test.go +++ b/internal/commands/templates_test.go @@ -73,8 +73,7 @@ func TestTemplatesLibraryUsesSDKLibraryEndpoint(t *testing.T) { body: templateLibraryJSON, }) buf := captureTemplateOutput(app) - app.Flags.Profile = "work profile" - app.Config.ActiveProfile = app.Flags.Profile + app.Config.ActiveProfile = "work profile" app.Config.Sources = map[string]string{"account_id": string(config.SourceFlag)} err := executeRecordingCommand(NewTemplatesCmd(), app, "library") @@ -118,8 +117,7 @@ func TestTemplatesCopyResolvesProjectAndTodosTool(t *testing.T) { }, ) buf := captureTemplateOutput(app) - app.Flags.Profile = "work profile" - app.Config.ActiveProfile = app.Flags.Profile + app.Config.ActiveProfile = "work profile" app.Config.Sources = map[string]string{"account_id": string(config.SourceFlag)} err := executeRecordingCommand(NewTemplatesCmd(), app, "copy", "3", "--in", "Test Project") @@ -214,8 +212,7 @@ func TestTemplatesCopyExplainsPeopleConfirmation(t *testing.T) { }, ) - app.Flags.Profile = "work profile; echo unsafe" - app.Config.ActiveProfile = app.Flags.Profile + app.Config.ActiveProfile = "work profile; echo unsafe" app.Config.Sources = map[string]string{"account_id": string(config.SourceFlag)} err := executeRecordingCommand(NewTemplatesCmd(), app, "copy", "3", "--in", "123") @@ -356,8 +353,7 @@ func TestTemplatesCopyStatusStates(t *testing.T) { body: test.body, }) buf := captureTemplateOutput(app) - app.Flags.Profile = "work profile" - app.Config.ActiveProfile = app.Flags.Profile + app.Config.ActiveProfile = "work profile" app.Config.Sources = map[string]string{"account_id": string(config.SourceFlag)} err := executeRecordingCommand(NewTemplatesCmd(), app, "copy-status", "5")