diff --git a/internal/commands/tools.go b/internal/commands/tools.go index 73940d6a5..351b441d9 100644 --- a/internal/commands/tools.go +++ b/internal/commands/tools.go @@ -30,8 +30,8 @@ Disabling a tool hides it from the dock but preserves its content.`, Annotations: map[string]string{"agent_notes": "Dock tools are the sidebar navigation items in a project\nEnable/disable controls visibility without deleting\nEach tool has a type (e.g., Todoset, Schedule, MessageBoard, Vault, Chat::Campfire)"}, } - cmd.PersistentFlags().StringVarP(&project, "project", "p", "", "Project ID or name") - cmd.PersistentFlags().StringVar(&project, "in", "", "Project ID (alias for --project)") + cmd.PersistentFlags().StringVarP(&project, "project", "p", "", "Project ID or name (for breadcrumbs)") + cmd.PersistentFlags().StringVar(&project, "in", "", "Project ID or name (alias for --project)") cmd.AddCommand( newToolsShowCmd(&project), @@ -46,6 +46,47 @@ Disabling a tool hides it from the dock but preserves its content.`, return cmd } +// resolveToolsProject optionally resolves a project for breadcrumb display. +// Returns the resolved project ID string, or "" if no project was specified. +// If the user explicitly provided a project via flag (--in/--project) that +// cannot be resolved, the error is returned. Config defaults are best-effort: +// resolution failures are silently ignored since the user didn't ask for +// project context on this invocation. +func resolveToolsProject(cmd *cobra.Command, app *appctx.App, project string) (string, error) { + explicit := project != "" + + projectID := project + if projectID == "" { + projectID = app.Flags.Project + if projectID != "" { + explicit = true + } + } + if projectID == "" { + projectID = app.Config.ProjectID + } + if projectID == "" { + return "", nil + } + + resolved, _, err := app.Names.ResolveProject(cmd.Context(), projectID) + if err != nil { + if explicit { + return "", err + } + return "", nil + } + return resolved, nil +} + +// toolBreadcrumbFlag returns " --in " if projectID is non-empty, or "". +func toolBreadcrumbFlag(projectID string) string { + if projectID == "" { + return "" + } + return " --in " + projectID +} + func newToolsShowCmd(project *string) *cobra.Command { return &cobra.Command{ Use: "show ", @@ -64,25 +105,11 @@ func newToolsShowCmd(project *string) *cobra.Command { return output.ErrUsage("Invalid tool ID") } - // Resolve project, with interactive fallback - projectID := *project - if projectID == "" { - projectID = app.Flags.Project - } - if projectID == "" { - projectID = app.Config.ProjectID - } - if projectID == "" { - if err := ensureProject(cmd, app); err != nil { - return err - } - projectID = app.Config.ProjectID - } - - resolvedProjectID, _, err := app.Names.ResolveProject(cmd.Context(), projectID) + resolvedProjectID, err := resolveToolsProject(cmd, app, *project) if err != nil { return err } + inFlag := toolBreadcrumbFlag(resolvedProjectID) tool, err := app.Account().Tools().Get(cmd.Context(), toolID) if err != nil { @@ -95,25 +122,29 @@ func newToolsShowCmd(project *string) *cobra.Command { } summary := fmt.Sprintf("%s (%s) at position %s", tool.Title, tool.Name, posStr) + crumbs := []output.Breadcrumb{ + { + Action: "rename", + Cmd: fmt.Sprintf("basecamp tools update %d \"New Name\"%s", toolID, inFlag), + Description: "Rename tool", + }, + { + Action: "reposition", + Cmd: fmt.Sprintf("basecamp tools reposition %d --position 1%s", toolID, inFlag), + Description: "Move tool", + }, + } + if resolvedProjectID != "" { + crumbs = append(crumbs, output.Breadcrumb{ + Action: "project", + Cmd: fmt.Sprintf("basecamp projects show %s", resolvedProjectID), + Description: "View project", + }) + } + return app.OK(tool, output.WithSummary(summary), - output.WithBreadcrumbs( - output.Breadcrumb{ - Action: "rename", - Cmd: fmt.Sprintf("basecamp tools update %d \"New Name\" --in %s", toolID, resolvedProjectID), - Description: "Rename tool", - }, - output.Breadcrumb{ - Action: "reposition", - Cmd: fmt.Sprintf("basecamp tools reposition %d --position 1 --in %s", toolID, resolvedProjectID), - Description: "Move tool", - }, - output.Breadcrumb{ - Action: "project", - Cmd: fmt.Sprintf("basecamp projects show %s", resolvedProjectID), - Description: "View project", - }, - ), + output.WithBreadcrumbs(crumbs...), ) }, } @@ -155,25 +186,11 @@ For example, clone a Chat to create a second chat room in the same project.`, } } - // Resolve project, with interactive fallback - projectID := *project - if projectID == "" { - projectID = app.Flags.Project - } - if projectID == "" { - projectID = app.Config.ProjectID - } - if projectID == "" { - if err := ensureProject(cmd, app); err != nil { - return err - } - projectID = app.Config.ProjectID - } - - resolvedProjectID, _, err := app.Names.ResolveProject(cmd.Context(), projectID) + resolvedProjectID, err := resolveToolsProject(cmd, app, *project) if err != nil { return err } + inFlag := toolBreadcrumbFlag(resolvedProjectID) var cloneOpts *basecamp.CloneToolOptions if title != "" { @@ -185,20 +202,24 @@ For example, clone a Chat to create a second chat room in the same project.`, return convertSDKError(err) } + crumbs := []output.Breadcrumb{ + { + Action: "tool", + Cmd: fmt.Sprintf("basecamp tools show %d%s", created.ID, inFlag), + Description: "View tool", + }, + } + if resolvedProjectID != "" { + crumbs = append(crumbs, output.Breadcrumb{ + Action: "project", + Cmd: fmt.Sprintf("basecamp projects show %s", resolvedProjectID), + Description: "View project", + }) + } + return app.OK(created, output.WithSummary(fmt.Sprintf("Created: %s", created.Title)), - output.WithBreadcrumbs( - output.Breadcrumb{ - Action: "tool", - Cmd: fmt.Sprintf("basecamp tools show %d --in %s", created.ID, resolvedProjectID), - Description: "View tool", - }, - output.Breadcrumb{ - Action: "project", - Cmd: fmt.Sprintf("basecamp projects show %s", resolvedProjectID), - Description: "View project", - }, - ), + output.WithBreadcrumbs(crumbs...), ) }, } @@ -241,45 +262,35 @@ func newToolsUpdateCmd(project *string) *cobra.Command { return output.ErrUsage(fmt.Sprintf("Tool name too long (%d characters, max 64)", n)) } - // Resolve project, with interactive fallback - projectID := *project - if projectID == "" { - projectID = app.Flags.Project - } - if projectID == "" { - projectID = app.Config.ProjectID - } - if projectID == "" { - if err := ensureProject(cmd, app); err != nil { - return err - } - projectID = app.Config.ProjectID - } - - resolvedProjectID, _, err := app.Names.ResolveProject(cmd.Context(), projectID) + resolvedProjectID, err := resolveToolsProject(cmd, app, *project) if err != nil { return err } + inFlag := toolBreadcrumbFlag(resolvedProjectID) tool, err := app.Account().Tools().Update(cmd.Context(), toolID, title) if err != nil { return convertSDKError(err) } + crumbs := []output.Breadcrumb{ + { + Action: "tool", + Cmd: fmt.Sprintf("basecamp tools show %d%s", toolID, inFlag), + Description: "View tool", + }, + } + if resolvedProjectID != "" { + crumbs = append(crumbs, output.Breadcrumb{ + Action: "project", + Cmd: fmt.Sprintf("basecamp projects show %s", resolvedProjectID), + Description: "View project", + }) + } + return app.OK(tool, output.WithSummary(fmt.Sprintf("Renamed to: %s", tool.Title)), - output.WithBreadcrumbs( - output.Breadcrumb{ - Action: "tool", - Cmd: fmt.Sprintf("basecamp tools show %d --in %s", toolID, resolvedProjectID), - Description: "View tool", - }, - output.Breadcrumb{ - Action: "project", - Cmd: fmt.Sprintf("basecamp projects show %s", resolvedProjectID), - Description: "View project", - }, - ), + output.WithBreadcrumbs(crumbs...), ) }, } @@ -308,22 +319,7 @@ WARNING: This permanently removes the tool and all its content.`, return output.ErrUsage("Invalid tool ID") } - // Resolve project, with interactive fallback - projectID := *project - if projectID == "" { - projectID = app.Flags.Project - } - if projectID == "" { - projectID = app.Config.ProjectID - } - if projectID == "" { - if err := ensureProject(cmd, app); err != nil { - return err - } - projectID = app.Config.ProjectID - } - - resolvedProjectID, _, err := app.Names.ResolveProject(cmd.Context(), projectID) + resolvedProjectID, err := resolveToolsProject(cmd, app, *project) if err != nil { return err } @@ -333,15 +329,18 @@ WARNING: This permanently removes the tool and all its content.`, return convertSDKError(err) } + var crumbs []output.Breadcrumb + if resolvedProjectID != "" { + crumbs = append(crumbs, output.Breadcrumb{ + Action: "project", + Cmd: fmt.Sprintf("basecamp projects show %s", resolvedProjectID), + Description: "View project", + }) + } + return app.OK(map[string]any{"trashed": true}, output.WithSummary(fmt.Sprintf("Tool %d trashed", toolID)), - output.WithBreadcrumbs( - output.Breadcrumb{ - Action: "project", - Cmd: fmt.Sprintf("basecamp projects show %s", resolvedProjectID), - Description: "View project", - }, - ), + output.WithBreadcrumbs(crumbs...), ) }, } @@ -367,25 +366,11 @@ func newToolsEnableCmd(project *string) *cobra.Command { return output.ErrUsage("Invalid tool ID") } - // Resolve project, with interactive fallback - projectID := *project - if projectID == "" { - projectID = app.Flags.Project - } - if projectID == "" { - projectID = app.Config.ProjectID - } - if projectID == "" { - if err := ensureProject(cmd, app); err != nil { - return err - } - projectID = app.Config.ProjectID - } - - resolvedProjectID, _, err := app.Names.ResolveProject(cmd.Context(), projectID) + resolvedProjectID, err := resolveToolsProject(cmd, app, *project) if err != nil { return err } + inFlag := toolBreadcrumbFlag(resolvedProjectID) err = app.Account().Tools().Enable(cmd.Context(), toolID) if err != nil { @@ -397,7 +382,7 @@ func newToolsEnableCmd(project *string) *cobra.Command { output.WithBreadcrumbs( output.Breadcrumb{ Action: "tool", - Cmd: fmt.Sprintf("basecamp tools show %d --in %s", toolID, resolvedProjectID), + Cmd: fmt.Sprintf("basecamp tools show %d%s", toolID, inFlag), Description: "View tool", }, ), @@ -426,25 +411,11 @@ The tool is not deleted - just hidden. Use 'basecamp tools enable' to restore.`, return output.ErrUsage("Invalid tool ID") } - // Resolve project, with interactive fallback - projectID := *project - if projectID == "" { - projectID = app.Flags.Project - } - if projectID == "" { - projectID = app.Config.ProjectID - } - if projectID == "" { - if err := ensureProject(cmd, app); err != nil { - return err - } - projectID = app.Config.ProjectID - } - - resolvedProjectID, _, err := app.Names.ResolveProject(cmd.Context(), projectID) + resolvedProjectID, err := resolveToolsProject(cmd, app, *project) if err != nil { return err } + inFlag := toolBreadcrumbFlag(resolvedProjectID) err = app.Account().Tools().Disable(cmd.Context(), toolID) if err != nil { @@ -456,7 +427,7 @@ The tool is not deleted - just hidden. Use 'basecamp tools enable' to restore.`, output.WithBreadcrumbs( output.Breadcrumb{ Action: "enable", - Cmd: fmt.Sprintf("basecamp tools enable %d --in %s", toolID, resolvedProjectID), + Cmd: fmt.Sprintf("basecamp tools enable %d%s", toolID, inFlag), Description: "Re-enable tool", }, ), @@ -490,25 +461,11 @@ func newToolsRepositionCmd(project *string) *cobra.Command { return output.ErrUsage("Invalid tool ID") } - // Resolve project, with interactive fallback - projectID := *project - if projectID == "" { - projectID = app.Flags.Project - } - if projectID == "" { - projectID = app.Config.ProjectID - } - if projectID == "" { - if err := ensureProject(cmd, app); err != nil { - return err - } - projectID = app.Config.ProjectID - } - - resolvedProjectID, _, err := app.Names.ResolveProject(cmd.Context(), projectID) + resolvedProjectID, err := resolveToolsProject(cmd, app, *project) if err != nil { return err } + inFlag := toolBreadcrumbFlag(resolvedProjectID) err = app.Account().Tools().Reposition(cmd.Context(), toolID, position) if err != nil { @@ -520,7 +477,7 @@ func newToolsRepositionCmd(project *string) *cobra.Command { output.WithBreadcrumbs( output.Breadcrumb{ Action: "tool", - Cmd: fmt.Sprintf("basecamp tools show %d --in %s", toolID, resolvedProjectID), + Cmd: fmt.Sprintf("basecamp tools show %d%s", toolID, inFlag), Description: "View tool", }, ), diff --git a/internal/commands/tools_test.go b/internal/commands/tools_test.go index 5187a09b4..33c58a839 100644 --- a/internal/commands/tools_test.go +++ b/internal/commands/tools_test.go @@ -1,7 +1,10 @@ package commands import ( + "encoding/json" "errors" + "io" + "net/http" "strings" "testing" @@ -154,3 +157,217 @@ func TestToolsCreateAcceptsMaxTitle(t *testing.T) { assert.NotContains(t, e.Message, "too long") } } + +// TestToolsShowNoProjectRequired verifies that tools show works without --in or a default project. +func TestToolsShowNoProjectRequired(t *testing.T) { + app, _ := setupTestApp(t) + // No ProjectID configured — should not prompt or error about project + + project := "" + cmd := newToolsShowCmd(&project) + + err := executeCommand(cmd, app, "123") + // Should reach the API call (network error), not fail on project resolution + require.NotNil(t, err) + assert.NotContains(t, strings.ToLower(err.Error()), "project") +} + +// TestToolsEnableNoProjectRequired verifies that tools enable works without --in. +func TestToolsEnableNoProjectRequired(t *testing.T) { + app, _ := setupTestApp(t) + + project := "" + cmd := newToolsEnableCmd(&project) + + err := executeCommand(cmd, app, "123") + require.NotNil(t, err) + assert.NotContains(t, strings.ToLower(err.Error()), "project") +} + +// TestToolsDisableNoProjectRequired verifies that tools disable works without --in. +func TestToolsDisableNoProjectRequired(t *testing.T) { + app, _ := setupTestApp(t) + + project := "" + cmd := newToolsDisableCmd(&project) + + err := executeCommand(cmd, app, "123") + require.NotNil(t, err) + assert.NotContains(t, strings.ToLower(err.Error()), "project") +} + +// TestToolsTrashNoProjectRequired verifies that tools trash works without --in. +func TestToolsTrashNoProjectRequired(t *testing.T) { + app, _ := setupTestApp(t) + + project := "" + cmd := newToolsTrashCmd(&project) + + err := executeCommand(cmd, app, "123") + require.NotNil(t, err) + assert.NotContains(t, strings.ToLower(err.Error()), "project") +} + +// TestToolsRepositionNoProjectRequired verifies that tools reposition works without --in. +func TestToolsRepositionNoProjectRequired(t *testing.T) { + app, _ := setupTestApp(t) + + project := "" + cmd := newToolsRepositionCmd(&project) + + err := executeCommand(cmd, app, "456", "--position", "2") + require.NotNil(t, err) + assert.NotContains(t, strings.ToLower(err.Error()), "project") +} + +// mockToolProjectFailTransport returns tools successfully but fails project resolution. +// This lets us prove that an explicit --in error stops the command before the tools API. +type mockToolProjectFailTransport struct { + toolsCalled bool +} + +func (t *mockToolProjectFailTransport) RoundTrip(req *http.Request) (*http.Response, error) { + header := make(http.Header) + header.Set("Content-Type", "application/json") + + switch { + case strings.Contains(req.URL.Path, "/projects.json"): + // Return empty project list so name resolution fails with "not found" + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(`[]`)), + Header: header, + }, nil + case strings.Contains(req.URL.Path, "/tools/"): + t.toolsCalled = true + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(`{"id": 123, "title": "Chat", "name": "chat"}`)), + Header: header, + }, nil + default: + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(`{}`)), + Header: header, + }, nil + } +} + +// TestToolsShowWithExplicitProjectErrorSurfaces verifies that an invalid explicit --in +// produces an error rather than silently dropping breadcrumbs, and the tools API is never called. +func TestToolsShowWithExplicitProjectErrorSurfaces(t *testing.T) { + transport := &mockToolProjectFailTransport{} + app, _ := newTestAppWithTransport(t, transport) + app.Config.ProjectID = "" + + // Explicit --in with a name that won't match any project + project := "nonexistent-project" + cmd := newToolsShowCmd(&project) + + err := executeCommand(cmd, app, "123") + require.NotNil(t, err) + assert.Contains(t, strings.ToLower(err.Error()), "not found") + assert.False(t, transport.toolsCalled, "tools API should not be called when project resolution fails") +} + +// TestToolsShowConfigProjectErrorIgnored verifies that a config default project that +// fails resolution is silently ignored (best-effort breadcrumbs). +func TestToolsShowConfigProjectErrorIgnored(t *testing.T) { + app, _ := setupTestApp(t) + // Config default with a name (not numeric) — resolution will fail on noNetworkTransport + app.Config.ProjectID = "stale-project-name" + + project := "" // no explicit flag + cmd := newToolsShowCmd(&project) + + err := executeCommand(cmd, app, "123") + // Should reach the API call, not fail on project resolution + require.NotNil(t, err) + assert.NotContains(t, err.Error(), "stale-project-name") +} + +// mockToolTransport serves canned responses for tools and project resolution. +type mockToolTransport struct{} + +func (t *mockToolTransport) RoundTrip(req *http.Request) (*http.Response, error) { + header := make(http.Header) + header.Set("Content-Type", "application/json") + + var body string + switch { + case strings.Contains(req.URL.Path, "/projects.json"): + body = `[{"id": 123, "name": "Test Project"}]` + case strings.HasSuffix(req.URL.Path, "/tools/555"): + body = `{"id": 555, "title": "Chat", "name": "chat", "enabled": true, "position": 2,` + + `"status": "active", "url": "https://example.com", "app_url": "https://example.com",` + + `"created_at": "2024-01-01T00:00:00Z", "updated_at": "2024-01-01T00:00:00Z"}` + default: + body = `{}` + } + + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(body)), + Header: header, + }, nil +} + +// TestToolsShowBreadcrumbsWithProject verifies that --in produces breadcrumbs containing --in . +func TestToolsShowBreadcrumbsWithProject(t *testing.T) { + app, buf := newTestAppWithTransport(t, &mockToolTransport{}) + app.Config.ProjectID = "" // clear default so only explicit flag matters + app.Flags.Hints = true // enable breadcrumbs in output + + project := "123" + cmd := newToolsShowCmd(&project) + + err := executeCommand(cmd, app, "555") + require.NoError(t, err) + + var envelope struct { + Breadcrumbs []struct { + Action string `json:"action"` + Cmd string `json:"cmd"` + } `json:"breadcrumbs"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + + // Should have rename, reposition, and project breadcrumbs + require.Len(t, envelope.Breadcrumbs, 3) + + assert.Contains(t, envelope.Breadcrumbs[0].Cmd, "--in 123") + assert.Contains(t, envelope.Breadcrumbs[1].Cmd, "--in 123") + assert.Equal(t, "project", envelope.Breadcrumbs[2].Action) + assert.Contains(t, envelope.Breadcrumbs[2].Cmd, "projects show 123") +} + +// TestToolsShowBreadcrumbsWithoutProject verifies that omitting --in produces +// breadcrumbs without --in and no "View project" breadcrumb. +func TestToolsShowBreadcrumbsWithoutProject(t *testing.T) { + app, buf := newTestAppWithTransport(t, &mockToolTransport{}) + app.Config.ProjectID = "" // no default project + app.Flags.Hints = true // enable breadcrumbs in output + + project := "" + cmd := newToolsShowCmd(&project) + + err := executeCommand(cmd, app, "555") + require.NoError(t, err) + + var envelope struct { + Breadcrumbs []struct { + Action string `json:"action"` + Cmd string `json:"cmd"` + } `json:"breadcrumbs"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + + // Should have rename and reposition breadcrumbs only — no project breadcrumb + require.Len(t, envelope.Breadcrumbs, 2) + + for _, bc := range envelope.Breadcrumbs { + assert.NotContains(t, bc.Cmd, "--in") + assert.NotEqual(t, "project", bc.Action) + } +}