diff --git a/.surface b/.surface index 70225d1e..56f27f83 100644 --- a/.surface +++ b/.surface @@ -13284,6 +13284,7 @@ FLAG basecamp todos move --ids-only type=bool FLAG basecamp todos move --in type=string FLAG basecamp todos move --jq type=string FLAG basecamp todos move --json type=bool +FLAG basecamp todos move --list type=string FLAG basecamp todos move --markdown type=bool FLAG basecamp todos move --md type=bool FLAG basecamp todos move --no-hints type=bool @@ -13307,6 +13308,7 @@ FLAG basecamp todos position --ids-only type=bool FLAG basecamp todos position --in type=string FLAG basecamp todos position --jq type=string FLAG basecamp todos position --json type=bool +FLAG basecamp todos position --list type=string FLAG basecamp todos position --markdown type=bool FLAG basecamp todos position --md type=bool FLAG basecamp todos position --no-hints type=bool @@ -13351,6 +13353,7 @@ FLAG basecamp todos reorder --ids-only type=bool FLAG basecamp todos reorder --in type=string FLAG basecamp todos reorder --jq type=string FLAG basecamp todos reorder --json type=bool +FLAG basecamp todos reorder --list type=string FLAG basecamp todos reorder --markdown type=bool FLAG basecamp todos reorder --md type=bool FLAG basecamp todos reorder --no-hints type=bool diff --git a/internal/commands/notifications.go b/internal/commands/notifications.go index d3dbfa2e..6810a978 100644 --- a/internal/commands/notifications.go +++ b/internal/commands/notifications.go @@ -77,10 +77,14 @@ func runNotificationsList(cmd *cobra.Command, page int32) error { if page == 0 { nextPage = 2 } + readCmd := "basecamp notifications read " + if page > 0 { + readCmd = fmt.Sprintf("basecamp notifications read --page %d", page) + } breadcrumbs := []output.Breadcrumb{ { Action: "read", - Cmd: "basecamp notifications read ", + Cmd: readCmd, Description: "Mark as read", }, { diff --git a/internal/commands/todos.go b/internal/commands/todos.go index 0d4947b5..2d6fc790 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -18,6 +18,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/dateparse" "github.com/basecamp/basecamp-cli/internal/output" "github.com/basecamp/basecamp-cli/internal/richtext" + "github.com/basecamp/basecamp-cli/internal/urlarg" ) // todosListFlags holds the flags for the todos list command. @@ -1640,17 +1641,26 @@ func reopenTodos(cmd *cobra.Command, todoIDs []string) error { } func newTodosPositionCmd() *cobra.Command { - var position int + var ( + position int + list string + ) cmd := &cobra.Command{ Use: "position ", Aliases: []string{"move", "reorder"}, - Short: "Change todo position", - Long: `Reorder a todo within its todolist. Position is 1-based (1 = top). + Short: "Change todo position or move between lists", + Long: `Reorder a todo within its todolist, or move it to a different list in the +same project. Position is 1-based (1 = top). You can pass either a todo ID or a Basecamp URL: basecamp todos position 789 --to 1 - basecamp todos position https://3.basecamp.com/123/buckets/456/todos/789 --to 1`, + basecamp todos position https://3.basecamp.com/123/buckets/456/todos/789 --to 1 + +Move to a different todolist in the same project: + basecamp todos position 789 --to 1 --list "Sprint 1" --in myproject + basecamp todos position 789 --to 1 --list 321 + basecamp todos position --to 1 --list `, RunE: func(cmd *cobra.Command, args []string) error { if len(args) == 0 { return missingArg(cmd, "") @@ -1669,21 +1679,97 @@ You can pass either a todo ID or a Basecamp URL: return output.ErrUsage("--to is required (1 = top)") } - // Extract ID from URL if provided - todoIDStr := extractID(args[0]) + // Extract todo ID and project from URL if provided + todoIDStr, todoProjectID := extractWithProject(args[0]) todoID, err := strconv.ParseInt(todoIDStr, 10, 64) if err != nil { return output.ErrUsage("Invalid todo ID") } - err = app.Account().Todos().Reposition(cmd.Context(), todoID, position, nil) + // Resolve destination todolist when --list is provided + var parentID *int64 + if list != "" { + listIDStr, listProjectID := extractWithProject(list) + + // When --list is a URL, validate it's a todolist URL — not a + // todo, project, or collection URL that would silently extract + // the wrong ID. + if parsed := urlarg.Parse(list); parsed != nil { + if parsed.RecordingID == "" || parsed.Type != "todolists" || parsed.IsCollection { + return output.ErrUsage("Expected a todolist URL (.../todolists/), " + + "or pass a todolist ID or name.") + } + } + + // Build project context: todo URL > --in flag > config + project := todoProjectID + if project == "" { + project = app.Flags.Project + } + if project == "" { + project = app.Config.ProjectID + } + + // Resolve project name to numeric ID only when needed: + // cross-project URL validation or todolist name resolution. + resolvedProject := project + needsResolve := (todoProjectID != "" && listProjectID != "") || !isNumeric(listIDStr) + if needsResolve && project != "" && !isNumeric(project) { + rp, _, resolveErr := app.Names.ResolveProject(cmd.Context(), project) + if resolveErr != nil { + return resolveErr + } + resolvedProject = rp + } + + // Cross-project moves are not supported by the reposition endpoint. + // Only enforce when the todo's project comes from its URL (high + // confidence). Config/flag project is a default context — it may + // not match where a bare-ID todo actually lives. + if todoProjectID != "" && listProjectID != "" && resolvedProject != listProjectID { + return output.ErrUsageHint( + "Cannot move a todo to a list in a different project.", + "Pass a todolist from the same project; cross-project moves are not supported.", + ) + } + + // Resolve todolist name to ID when not already numeric + if !isNumeric(listIDStr) { + if resolvedProject == "" { + return output.ErrUsage("--in is required to resolve todolist names") + } + resolved, resolveErr := resolveTodolistInTodoset(cmd, app, listIDStr, resolvedProject, "") + if resolveErr != nil { + return resolveErr + } + listIDStr = resolved + } + + listID, parseErr := strconv.ParseInt(listIDStr, 10, 64) + if parseErr != nil { + return output.ErrUsage("Invalid todolist ID") + } + parentID = &listID + } + + err = app.Account().Todos().Reposition(cmd.Context(), todoID, position, parentID) if err != nil { return convertSDKError(err) } - return app.OK(map[string]any{"repositioned": true, "position": position}, - output.WithSummary(fmt.Sprintf("Moved todo #%d to position %d", todoID, position)), + summary := fmt.Sprintf("Moved todo #%d to position %d", todoID, position) + if parentID != nil { + summary = fmt.Sprintf("Moved todo #%d to list #%d at position %d", todoID, *parentID, position) + } + + response := map[string]any{"repositioned": true, "position": position} + if parentID != nil { + response["todolist_id"] = *parentID + } + + return app.OK(response, + output.WithSummary(summary), output.WithBreadcrumbs( output.Breadcrumb{ Action: "show", @@ -1697,6 +1783,7 @@ You can pass either a todo ID or a Basecamp URL: cmd.Flags().IntVar(&position, "to", 0, "Target position, 1-based (1 = top)") cmd.Flags().IntVar(&position, "position", 0, "Target position (alias for --to)") + cmd.Flags().StringVarP(&list, "list", "l", "", "Destination todolist ID, name, or URL (move to a different list)") return cmd } diff --git a/internal/commands/todos_test.go b/internal/commands/todos_test.go index 469c0b21..4873b625 100644 --- a/internal/commands/todos_test.go +++ b/internal/commands/todos_test.go @@ -181,6 +181,92 @@ func TestTodosPositionRequiresPosition(t *testing.T) { assert.Equal(t, "--to is required (1 = top)", err.Error()) } +// TestTodosPositionRejectsCrossProjectListURL tests that --list with a URL from a +// different project than the todo URL is rejected with a clear error. +func TestTodosPositionRejectsCrossProjectListURL(t *testing.T) { + app, _ := setupTodosTestApp(t) + + cmd := NewTodosCmd() + + err := executeTodosCommand(cmd, app, "position", + "https://3.basecamp.com/99999/buckets/100/todos/789", + "--to", "1", + "--list", "https://3.basecamp.com/99999/buckets/200/todolists/321", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "Cannot move a todo to a list in a different project") +} + +// TestTodosPositionAcceptsSameProjectListURL tests that --list with a URL from the +// same project as the todo URL passes the cross-project check (fails at network, +// not at validation). +func TestTodosPositionAcceptsSameProjectListURL(t *testing.T) { + app, _ := setupTodosTestApp(t) + + cmd := NewTodosCmd() + + err := executeTodosCommand(cmd, app, "position", + "https://3.basecamp.com/99999/buckets/100/todos/789", + "--to", "1", + "--list", "https://3.basecamp.com/99999/buckets/100/todolists/321", + ) + // Should pass validation and fail at the SDK call (network disabled), + // not at the cross-project check. + require.Error(t, err) + assert.NotContains(t, err.Error(), "different project") +} + +// TestTodosPositionListNameRequiresProject tests that --list with a name (not numeric) +// requires a project context via --in or config. +func TestTodosPositionListNameRequiresProject(t *testing.T) { + app, _ := setupTodosTestApp(t) + + cmd := NewTodosCmd() + + err := executeTodosCommand(cmd, app, "position", "789", + "--to", "1", + "--list", "Sprint 1", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "--in is required to resolve todolist names") +} + +// TestTodosPositionBareIDSkipsCrossProjectGuard tests that the cross-project +// guard does not fire when the todo is a bare ID. Config project is a default +// context that may not match where the todo actually lives. +func TestTodosPositionBareIDSkipsCrossProjectGuard(t *testing.T) { + app, _ := setupTodosTestApp(t) + app.Config.ProjectID = "100" + + cmd := NewTodosCmd() + + // Todo is bare ID (config project = "100"), list URL has project 200. + // Should NOT reject — bare ID means we don't know the todo's project. + err := executeTodosCommand(cmd, app, "position", "789", + "--to", "1", + "--list", "https://3.basecamp.com/99999/buckets/200/todolists/321", + ) + require.Error(t, err) + assert.NotContains(t, err.Error(), "different project") +} + +// TestTodosPositionRejectsNonTodolistURL tests that --list rejects URLs that +// aren't todolist URLs (e.g. todo URLs, project URLs). +func TestTodosPositionRejectsNonTodolistURL(t *testing.T) { + app, _ := setupTodosTestApp(t) + + cmd := NewTodosCmd() + + // A todo URL, not a todolist URL — should not silently use the todo ID + err := executeTodosCommand(cmd, app, "position", + "https://3.basecamp.com/99999/buckets/100/todos/789", + "--to", "1", + "--list", "https://3.basecamp.com/99999/buckets/100/todos/555", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "todolist URL") +} + // TestTodoShortcutRequiresContent tests that todo shortcut requires content. func TestTodoShortcutShowsHelpWithoutContent(t *testing.T) { app, _ := setupTodosTestApp(t) diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index ccee186c..e5705ff1 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -373,6 +373,7 @@ basecamp unassign [id...] --card --from --in # Remove ca basecamp assign [id...] --step --to --in # Assign card step basecamp unassign [id...] --step --from --in # Remove step assignee basecamp todos position --to 1 # Move to top +basecamp todos position --to 1 --list # Move to different list basecamp todos sweep --overdue --complete --comment "Done" --in ```