From cceebb47cc340819ad2765346a5fdc67b8d6211a Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 24 Jul 2026 12:14:12 -0700 Subject: [PATCH 1/5] Add `todolists position` to reorder to-do lists (#484) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `basecamp todolists position ...` to reorder a to-do list within its todoset — the one repositioning gap in the to-do family (`todos` and `todolistgroups` already had it). Single-list mode sets one explicit position (`--to N`, 1-based). Bulk mode accepts several incomplete lists from the same todoset and sets their visible order top-to-bottom; it is restricted to position 1 because reverse-iterating onto any other slot cannot reconstruct the typed order once loose to-dos or hidden completed lists shift the base. Bulk preflight enforces a shared todoset/bucket and rejects completed lists before issuing any PUT, and application stops at the first failure (a half-applied reorder is a wrong order), reporting applied-vs-attempted. Uses the dedicated `PUT /todosets/todolists/{id}/position.json` route, not the generic `/recordings/{id}/position.json` the issue's workaround used — the generic one skips the to-do-list position math and mis-places lists when loose to-dos or hidden completed lists exist. Naming follows the to-do family (`position`, aliases `move`/`reorder`, `--to` leading with `--position` accepted); a to-do list has no destination container, so `--to N` is unambiguously an index. SDK bumped to 93ace8d5 (basecamp-sdk#398), which also absorbs #396 (Templates.CreateProject envelope fix, signature unchanged) and #392 (SDK tests only). Closes #484 --- .surface | 156 ++++++++ API-COVERAGE.md | 10 +- e2e/errors.bats | 38 ++ e2e/smoke/smoke_aliases.bats | 10 + e2e/smoke/smoke_todos_write.bats | 12 + go.sum | 2 + internal/commands/commands.go | 2 +- internal/commands/todolists.go | 192 +++++++++ internal/commands/todolists_test.go | 592 ++++++++++++++++++++++++++++ skills/basecamp/SKILL.md | 5 + 10 files changed, 1013 insertions(+), 6 deletions(-) diff --git a/.surface b/.surface index a02c2c4c2..14737e010 100644 --- a/.surface +++ b/.surface @@ -308,6 +308,9 @@ ARG basecamp tlgroups update 00 ARG basecamp tlgroups update 01 ARG basecamp todolist archive 00 ARG basecamp todolist create 00 +ARG basecamp todolist move 00 ... +ARG basecamp todolist position 00 ... +ARG basecamp todolist reorder 00 ... ARG basecamp todolist restore 00 ARG basecamp todolist show 00 ARG basecamp todolist trash 00 @@ -330,6 +333,9 @@ ARG basecamp todolistgroups update 00 ARG basecamp todolistgroups update 01 ARG basecamp todolists archive 00 ARG basecamp todolists create 00 +ARG basecamp todolists move 00 ... +ARG basecamp todolists position 00 ... +ARG basecamp todolists reorder 00 ... ARG basecamp todolists restore 00 ARG basecamp todolists show 00 ARG basecamp todolists trash 00 @@ -896,6 +902,9 @@ CMD basecamp todolist CMD basecamp todolist archive CMD basecamp todolist create CMD basecamp todolist list +CMD basecamp todolist move +CMD basecamp todolist position +CMD basecamp todolist reorder CMD basecamp todolist restore CMD basecamp todolist show CMD basecamp todolist trash @@ -920,6 +929,9 @@ CMD basecamp todolists CMD basecamp todolists archive CMD basecamp todolists create CMD basecamp todolists list +CMD basecamp todolists move +CMD basecamp todolists position +CMD basecamp todolists reorder CMD basecamp todolists restore CMD basecamp todolists show CMD basecamp todolists trash @@ -12482,6 +12494,75 @@ FLAG basecamp todolist list --styled type=bool FLAG basecamp todolist list --todolist type=string FLAG basecamp todolist list --todoset type=string FLAG basecamp todolist list --verbose type=count +FLAG basecamp todolist move --account type=string +FLAG basecamp todolist move --agent type=bool +FLAG basecamp todolist move --cache-dir type=string +FLAG basecamp todolist move --count type=bool +FLAG basecamp todolist move --help type=bool +FLAG basecamp todolist move --hints type=bool +FLAG basecamp todolist move --ids-only type=bool +FLAG basecamp todolist move --in type=string +FLAG basecamp todolist move --jq type=string +FLAG basecamp todolist move --json type=bool +FLAG basecamp todolist move --markdown type=bool +FLAG basecamp todolist move --md type=bool +FLAG basecamp todolist move --no-hints type=bool +FLAG basecamp todolist move --no-stats type=bool +FLAG basecamp todolist move --position type=int +FLAG basecamp todolist move --profile type=string +FLAG basecamp todolist move --project type=string +FLAG basecamp todolist move --quiet type=bool +FLAG basecamp todolist move --stats type=bool +FLAG basecamp todolist move --styled type=bool +FLAG basecamp todolist move --to type=int +FLAG basecamp todolist move --todolist type=string +FLAG basecamp todolist move --verbose type=count +FLAG basecamp todolist position --account type=string +FLAG basecamp todolist position --agent type=bool +FLAG basecamp todolist position --cache-dir type=string +FLAG basecamp todolist position --count type=bool +FLAG basecamp todolist position --help type=bool +FLAG basecamp todolist position --hints type=bool +FLAG basecamp todolist position --ids-only type=bool +FLAG basecamp todolist position --in type=string +FLAG basecamp todolist position --jq type=string +FLAG basecamp todolist position --json type=bool +FLAG basecamp todolist position --markdown type=bool +FLAG basecamp todolist position --md type=bool +FLAG basecamp todolist position --no-hints type=bool +FLAG basecamp todolist position --no-stats type=bool +FLAG basecamp todolist position --position type=int +FLAG basecamp todolist position --profile type=string +FLAG basecamp todolist position --project type=string +FLAG basecamp todolist position --quiet type=bool +FLAG basecamp todolist position --stats type=bool +FLAG basecamp todolist position --styled type=bool +FLAG basecamp todolist position --to type=int +FLAG basecamp todolist position --todolist type=string +FLAG basecamp todolist position --verbose type=count +FLAG basecamp todolist reorder --account type=string +FLAG basecamp todolist reorder --agent type=bool +FLAG basecamp todolist reorder --cache-dir type=string +FLAG basecamp todolist reorder --count type=bool +FLAG basecamp todolist reorder --help type=bool +FLAG basecamp todolist reorder --hints type=bool +FLAG basecamp todolist reorder --ids-only type=bool +FLAG basecamp todolist reorder --in type=string +FLAG basecamp todolist reorder --jq type=string +FLAG basecamp todolist reorder --json type=bool +FLAG basecamp todolist reorder --markdown type=bool +FLAG basecamp todolist reorder --md type=bool +FLAG basecamp todolist reorder --no-hints type=bool +FLAG basecamp todolist reorder --no-stats type=bool +FLAG basecamp todolist reorder --position type=int +FLAG basecamp todolist reorder --profile type=string +FLAG basecamp todolist reorder --project type=string +FLAG basecamp todolist reorder --quiet type=bool +FLAG basecamp todolist reorder --stats type=bool +FLAG basecamp todolist reorder --styled type=bool +FLAG basecamp todolist reorder --to type=int +FLAG basecamp todolist reorder --todolist type=string +FLAG basecamp todolist reorder --verbose type=count FLAG basecamp todolist restore --account type=string FLAG basecamp todolist restore --agent type=bool FLAG basecamp todolist restore --cache-dir type=string @@ -13025,6 +13106,75 @@ FLAG basecamp todolists list --styled type=bool FLAG basecamp todolists list --todolist type=string FLAG basecamp todolists list --todoset type=string FLAG basecamp todolists list --verbose type=count +FLAG basecamp todolists move --account type=string +FLAG basecamp todolists move --agent type=bool +FLAG basecamp todolists move --cache-dir type=string +FLAG basecamp todolists move --count type=bool +FLAG basecamp todolists move --help type=bool +FLAG basecamp todolists move --hints type=bool +FLAG basecamp todolists move --ids-only type=bool +FLAG basecamp todolists move --in type=string +FLAG basecamp todolists move --jq type=string +FLAG basecamp todolists move --json type=bool +FLAG basecamp todolists move --markdown type=bool +FLAG basecamp todolists move --md type=bool +FLAG basecamp todolists move --no-hints type=bool +FLAG basecamp todolists move --no-stats type=bool +FLAG basecamp todolists move --position type=int +FLAG basecamp todolists move --profile type=string +FLAG basecamp todolists move --project type=string +FLAG basecamp todolists move --quiet type=bool +FLAG basecamp todolists move --stats type=bool +FLAG basecamp todolists move --styled type=bool +FLAG basecamp todolists move --to type=int +FLAG basecamp todolists move --todolist type=string +FLAG basecamp todolists move --verbose type=count +FLAG basecamp todolists position --account type=string +FLAG basecamp todolists position --agent type=bool +FLAG basecamp todolists position --cache-dir type=string +FLAG basecamp todolists position --count type=bool +FLAG basecamp todolists position --help type=bool +FLAG basecamp todolists position --hints type=bool +FLAG basecamp todolists position --ids-only type=bool +FLAG basecamp todolists position --in type=string +FLAG basecamp todolists position --jq type=string +FLAG basecamp todolists position --json type=bool +FLAG basecamp todolists position --markdown type=bool +FLAG basecamp todolists position --md type=bool +FLAG basecamp todolists position --no-hints type=bool +FLAG basecamp todolists position --no-stats type=bool +FLAG basecamp todolists position --position type=int +FLAG basecamp todolists position --profile type=string +FLAG basecamp todolists position --project type=string +FLAG basecamp todolists position --quiet type=bool +FLAG basecamp todolists position --stats type=bool +FLAG basecamp todolists position --styled type=bool +FLAG basecamp todolists position --to type=int +FLAG basecamp todolists position --todolist type=string +FLAG basecamp todolists position --verbose type=count +FLAG basecamp todolists reorder --account type=string +FLAG basecamp todolists reorder --agent type=bool +FLAG basecamp todolists reorder --cache-dir type=string +FLAG basecamp todolists reorder --count type=bool +FLAG basecamp todolists reorder --help type=bool +FLAG basecamp todolists reorder --hints type=bool +FLAG basecamp todolists reorder --ids-only type=bool +FLAG basecamp todolists reorder --in type=string +FLAG basecamp todolists reorder --jq type=string +FLAG basecamp todolists reorder --json type=bool +FLAG basecamp todolists reorder --markdown type=bool +FLAG basecamp todolists reorder --md type=bool +FLAG basecamp todolists reorder --no-hints type=bool +FLAG basecamp todolists reorder --no-stats type=bool +FLAG basecamp todolists reorder --position type=int +FLAG basecamp todolists reorder --profile type=string +FLAG basecamp todolists reorder --project type=string +FLAG basecamp todolists reorder --quiet type=bool +FLAG basecamp todolists reorder --stats type=bool +FLAG basecamp todolists reorder --styled type=bool +FLAG basecamp todolists reorder --to type=int +FLAG basecamp todolists reorder --todolist type=string +FLAG basecamp todolists reorder --verbose type=count FLAG basecamp todolists restore --account type=string FLAG basecamp todolists restore --agent type=bool FLAG basecamp todolists restore --cache-dir type=string @@ -16545,6 +16695,9 @@ SUB basecamp todolist SUB basecamp todolist archive SUB basecamp todolist create SUB basecamp todolist list +SUB basecamp todolist move +SUB basecamp todolist position +SUB basecamp todolist reorder SUB basecamp todolist restore SUB basecamp todolist show SUB basecamp todolist trash @@ -16569,6 +16722,9 @@ SUB basecamp todolists SUB basecamp todolists archive SUB basecamp todolists create SUB basecamp todolists list +SUB basecamp todolists move +SUB basecamp todolists position +SUB basecamp todolists reorder SUB basecamp todolists restore SUB basecamp todolists show SUB basecamp todolists trash diff --git a/API-COVERAGE.md b/API-COVERAGE.md index 70acb4411..7c606d320 100644 --- a/API-COVERAGE.md +++ b/API-COVERAGE.md @@ -6,17 +6,17 @@ Coverage of Basecamp 3 API endpoints. Source: [bc3-api/sections](https://github. | Status | Sections | Endpoints | |--------|----------|-----------| -| ✅ Implemented | 43 | 162 | +| ✅ Implemented | 43 | 163 | | ⏭️ Out of scope | 4 | 12 | -| **Total (docs)** | **47** | **174** | +| **Total (docs)** | **47** | **175** | -**100% coverage of in-scope API** (162/162 endpoints) +**100% coverage of in-scope API** (163/163 endpoints) Out-of-scope sections are excluded from parity totals and scripts: chatbots (different auth), legacy Clientside (deprecated) > Note: the per-row `Endpoints` column in the Coverage by Section table sums higher than the Summary totals above. The discrepancy predates the BC5 baseline; the row count (47 sections) is authoritative for the `Since` column. Reconciling endpoint counts is pre-existing maintenance, tracked separately. -**SDK version:** v0.8.0 — adds `Campfires.UpdateLine` (`PUT /chats/{c}/lines/{l}`, basecamp/basecamp-sdk#295), consumed by `chat update`; also carries the BC5 `Tools.Create` create-by-type reshape. API date advanced to 2026-07-22. +**SDK version:** v0.8.1-0.20260723015714-93ace8d569b5 — adds `Todolists.Reposition` (`PUT /todosets/todolists/{id}/position.json`, basecamp/basecamp-sdk#398), consumed by `todolists position`; also carries `Campfires.UpdateLine` (`PUT /chats/{c}/lines/{l}`, basecamp/basecamp-sdk#295), consumed by `chat update`, and the BC5 `Tools.Create` create-by-type reshape. API date 2026-07-22. ## Coverage by Section @@ -27,7 +27,7 @@ The **Since** column tags each row with the Basecamp version that introduced its | **Core** | | projects | 9 | `projects` | ✅ | BC4 | - | list, show, create, update, delete | | todos | 11 | `todos`, `todo`, `done`, `reopen` | ✅ | BC4 | - | list, show, create, update, complete, uncomplete, position (BC5: `steps` shown on `todos show`; edit via `cards step`) | -| todolists | 8 | `todolists` | ✅ | BC4 | - | list, show, create, update | +| todolists | 9 | `todolists` | ✅ | BC4 | - | list, show, create, update, position | | todosets | 3 | `todosets` | ✅ | BC4 | - | Container for todolists, accessed via project dock (BC5: `todos_count`, `completed_loose_todos_count`, `todos_url`, `app_todos_url`) | | todolist_groups | 8 | `todolistgroups` | ✅ | BC4 | - | list, show, create, update, position | | dock_tools | 7 | `tools` | ✅ | BC4 | - | Dock tool management: show, update, trash, enable, disable, reposition. `create` is BC5-only (create-by-type: `POST /buckets/{id}/dock/tools.json`), replacing the removed clone call | diff --git a/e2e/errors.bats b/e2e/errors.bats index 5eebb7e1b..56b768aea 100644 --- a/e2e/errors.bats +++ b/e2e/errors.bats @@ -348,6 +348,44 @@ load test_helper assert_output_contains "required" } +@test "todolists position without id shows error" { + create_credentials + create_global_config '{"account_id": 99999, "project_id": 123}' + + run basecamp todolists position --to 1 + assert_failure + assert_json_value '.error' '... required' + assert_json_value '.code' 'usage' +} + +@test "todolists position without --to shows error" { + create_credentials + create_global_config '{"account_id": 99999, "project_id": 123}' + + run basecamp todolists position 123 + assert_failure + assert_output_contains "required" +} + +@test "todolists position multiple ids with --to 2 shows error" { + create_credentials + create_global_config '{"account_id": 99999, "project_id": 123}' + + run basecamp todolists position 123 456 --to 2 + assert_failure + assert_output_contains "position 1" +} + +@test "todolists position with only a comma shows error" { + create_credentials + create_global_config '{"account_id": 99999, "project_id": 123}' + + run basecamp todolists position "," + assert_failure + assert_json_value '.error' '... required' + assert_json_value '.code' 'usage' +} + @test "comments without subcommand shows help" { create_credentials create_global_config '{"account_id": 99999, "project_id": 123}' diff --git a/e2e/smoke/smoke_aliases.bats b/e2e/smoke/smoke_aliases.bats index 301b4f248..2b3ab278a 100644 --- a/e2e/smoke/smoke_aliases.bats +++ b/e2e/smoke/smoke_aliases.bats @@ -223,6 +223,16 @@ load smoke_helper mark_out_of_scope "Alias for todos position — tested via canonical form" } +# --- todolists --- + +@test "todolists move is out of scope" { + mark_out_of_scope "Alias for todolists position — tested via canonical form" +} + +@test "todolists reorder is out of scope" { + mark_out_of_scope "Alias for todolists position — tested via canonical form" +} + # --- todolistgroups --- @test "todolistgroups move is out of scope" { diff --git a/e2e/smoke/smoke_todos_write.bats b/e2e/smoke/smoke_todos_write.bats index 56ad9db1a..750980630 100644 --- a/e2e/smoke/smoke_todos_write.bats +++ b/e2e/smoke/smoke_todos_write.bats @@ -237,6 +237,18 @@ setup_file() { echo "$tl_id" > "$BATS_FILE_TMPDIR/todolist_target_id" } +@test "todolists position repositions a todolist" { + local id_file="$BATS_FILE_TMPDIR/todolist_target_id" + [[ -f "$id_file" ]] || mark_unverifiable "No todolist created in prior test" + local tl_id + tl_id=$(<"$id_file") + + # Runs before archive/trash so the target list is still active. + run_smoke basecamp todolists position "$tl_id" --to 1 -p "$QA_PROJECT" --json + assert_success + assert_json_value '.ok' 'true' +} + @test "todolists archive archives a todolist" { local id_file="$BATS_FILE_TMPDIR/todolist_target_id" [[ -f "$id_file" ]] || mark_unverifiable "No todolist created in prior test" diff --git a/go.sum b/go.sum index 14c85fd96..87b642f8f 100644 --- a/go.sum +++ b/go.sum @@ -25,6 +25,8 @@ github.com/aymerick/douceur v0.2.0 h1:Mv+mAeH1Q+n9Fr+oyamOlAkUNPWPlA8PPGR0QAaYuP github.com/aymerick/douceur v0.2.0/go.mod h1:wlT5vV2O3h55X9m7iVYN0TBM0NH/MmbLnd30/FjWUq4= github.com/basecamp/basecamp-sdk/go v0.8.1-0.20260724184307-e2c1abea4aea h1:dRwhvhnzbyXxitGbb3uGFaUIJHazztD9UgWC0XzM870= github.com/basecamp/basecamp-sdk/go v0.8.1-0.20260724184307-e2c1abea4aea/go.mod h1:eX5mEKCdtxSfEL4P/n5AwOl21JVA/K+gRPic/Hd8W/Y= +github.com/basecamp/basecamp-sdk/go v0.8.1-0.20260723015714-93ace8d569b5 h1:M0KUktNpOI3gP2rQ0YG6AfjTqm8HTQXuJiErvBn7ttA= +github.com/basecamp/basecamp-sdk/go v0.8.1-0.20260723015714-93ace8d569b5/go.mod h1:eX5mEKCdtxSfEL4P/n5AwOl21JVA/K+gRPic/Hd8W/Y= github.com/basecamp/cli v0.2.1 h1:8GyehPVtsTXla0oOPu4QgXRjwwzJ99prlByvyi+0HRQ= github.com/basecamp/cli v0.2.1/go.mod h1:p8tt/DatJ2LAzWO6N6tNfV8x3gF5T3IxDTo+U8FfWPo= github.com/bmatcuk/doublestar v1.1.1/go.mod h1:UD6OnuiIn0yFxxA2le/rnRU1G4RaI4UvFv1sNto9p6w= diff --git a/internal/commands/commands.go b/internal/commands/commands.go index e8c660dce..f63f2b4ad 100644 --- a/internal/commands/commands.go +++ b/internal/commands/commands.go @@ -36,7 +36,7 @@ func CommandCategories() []CommandCategory { Commands: []CommandInfo{ {Name: "projects", Category: "core", Description: "Manage projects", Actions: []string{"list", "show", "create", "update", "delete"}}, {Name: "todos", Category: "core", Description: "Manage to-dos", Actions: []string{"list", "show", "create", "update", "complete", "uncomplete", "position", "trash", "archive", "restore"}}, - {Name: "todolists", Category: "core", Description: "Manage to-do lists", Actions: []string{"list", "show", "create", "update", "trash", "archive", "restore"}}, + {Name: "todolists", Category: "core", Description: "Manage to-do lists", Actions: []string{"list", "show", "create", "update", "position", "trash", "archive", "restore"}}, {Name: "todosets", Category: "core", Description: "Manage to-do set containers", Actions: []string{"list", "show"}}, {Name: "hillcharts", Category: "core", Description: "Manage hill charts", Actions: []string{"show", "track", "untrack"}}, {Name: "gauges", Category: "core", Description: "Manage gauges", Actions: []string{"list", "needles", "needle", "create", "update", "delete", "enable", "disable"}}, diff --git a/internal/commands/todolists.go b/internal/commands/todolists.go index acab87915..890cccdcf 100644 --- a/internal/commands/todolists.go +++ b/internal/commands/todolists.go @@ -1,6 +1,7 @@ package commands import ( + "errors" "fmt" "strconv" @@ -36,6 +37,7 @@ to disambiguate when needed.`, newTodolistsShowCmd(&project), newTodolistsCreateCmd(&project, &todosetID), newTodolistsUpdateCmd(&project), + newTodolistsPositionCmd(&project), newRecordableTrashCmd("todolist"), newRecordableArchiveCmd("todolist"), newRecordableRestoreCmd("todolist"), @@ -457,3 +459,193 @@ You can pass either a todolist ID or a Basecamp URL: return cmd } + +func newTodolistsPositionCmd(project *string) *cobra.Command { + var position int + + cmd := &cobra.Command{ + Use: "position ...", + Aliases: []string{"move", "reorder"}, + Short: "Change todolist position", + Long: `Reorder a todolist within its todoset. Position is 1-based (1 = top). + + basecamp todolists position 789 --to 1 + basecamp todolists position https://3.basecamp.com/1/buckets/2/todolists/789 --to 1 + +Pass several todolists to set their order, top to bottom. Bulk reordering always +places them at the top of the todoset; every list must live in the same todoset +and be incomplete (completed lists are positioned separately by Basecamp): + + basecamp todolists position 701 702 703 704 705 + +Positioning is relative and cascades: sibling lists shift to make room, and the +server translates the position for loose to-dos and hidden completed lists. +Confirm with ` + "`basecamp todolists list`" + `.`, + RunE: func(cmd *cobra.Command, args []string) error { + if len(args) == 0 { + return missingArg(cmd, "...") + } + + app := appctx.FromContext(cmd.Context()) + if app == nil { + return fmt.Errorf("app not initialized") + } + + if err := ensureAccount(cmd, app); err != nil { + return err + } + + // ExtractIDs drops empty segments, so a bare "," yields zero IDs. + ids := extractIDs(args) + if len(ids) == 0 { + return missingArg(cmd, "...") + } + + // Distinguish an omitted flag from an explicit --to 0. + supplied := cmd.Flags().Changed("to") || cmd.Flags().Changed("position") + if supplied && position < 1 { + return output.ErrUsage("--to must be at least 1 (1 = top)") + } + switch { + case len(ids) == 1 && !supplied: + return output.ErrUsage("--to is required (1 = top)") + case len(ids) > 1 && supplied && position != 1: + return output.ErrUsage("Reordering multiple todolists is only supported at position 1; " + + "drop --to or pass --to 1") + case !supplied: + position = 1 // bulk default + } + + // Validate every ID and reject duplicates before mutating anything. + parsed := make([]int64, 0, len(ids)) + seen := make(map[int64]bool, len(ids)) + for _, idStr := range ids { + id, err := strconv.ParseInt(idStr, 10, 64) + if err != nil { + return output.ErrUsage("Invalid todolist ID") + } + if seen[id] { + return output.ErrUsage(fmt.Sprintf("Duplicate todolist ID %d", id)) + } + seen[id] = true + parsed = append(parsed, id) + } + + // Sibling preflight (bulk only): every list must belong to the same + // todoset and bucket, and be incomplete, before any PUT is issued. + var bucketID, parentID int64 + if len(parsed) > 1 { + for _, id := range parsed { + tl, err := app.Account().Todolists().Get(cmd.Context(), id) + if err != nil { + return convertSDKError(err) + } + if tl.Parent == nil || tl.Bucket == nil { + return output.ErrUsageHint( + fmt.Sprintf("Todolist #%d is missing its todoset or project context.", id), + "Reorder one todoset at a time.", + ) + } + if tl.Completed { + return output.ErrUsageHint( + fmt.Sprintf("Todolist #%d (%s) is completed; bulk reordering accepts incomplete lists only.", id, tl.Name), + "Basecamp positions completed lists separately — reorder the incomplete lists on their own.", + ) + } + if parentID == 0 { + parentID = tl.Parent.ID + bucketID = tl.Bucket.ID + } else if tl.Parent.ID != parentID || tl.Bucket.ID != bucketID { + return output.ErrUsageHint( + fmt.Sprintf("Todolist #%d belongs to a different todoset (#%d) than the others.", id, tl.Parent.ID), + "Reorder one todoset at a time.", + ) + } + } + } + + // Apply in reverse so the typed order lands top→bottom onto position 1. + // Stop at the first failure: a half-applied reorder is a wrong order. + applied := 0 + for i := len(parsed) - 1; i >= 0; i-- { + if err := app.Account().Todolists().Reposition(cmd.Context(), parsed[i], position); err != nil { + converted := convertSDKError(err) + msg := fmt.Sprintf("Reordered %d of %d todolists; failed at #%d", applied, len(parsed), parsed[i]) + hint := "Rerun the whole command once the cause is fixed; the todoset is now in an intermediate order." + var outErr *output.Error + if errors.As(converted, &outErr) { + return &output.Error{ + Code: outErr.Code, + Message: fmt.Sprintf("%s: %s", msg, outErr.Message), + Hint: hint, + HTTPStatus: outErr.HTTPStatus, + Retryable: outErr.Retryable, + Cause: outErr, + } + } + return output.ErrUsageHint(fmt.Sprintf("%s: %s", msg, converted.Error()), hint) + } + applied++ + } + + var summary string + var breadcrumbs []output.Breadcrumb + if len(parsed) == 1 { + summary = fmt.Sprintf("Moved todolist #%d to position %d", parsed[0], position) + // No preflight Get in single mode: resolve project from ambient + // context (URL > group flag > flags > config) for the breadcrumbs. + _, urlProjectID := extractWithProject(args[0]) + proj := urlProjectID + if proj == "" { + proj = *project + } + if proj == "" { + proj = app.Flags.Project + } + if proj == "" { + proj = app.Config.ProjectID + } + if proj != "" { + breadcrumbs = []output.Breadcrumb{ + { + Action: "show", + Cmd: fmt.Sprintf("basecamp todolists show %d --in %s", parsed[0], proj), + Description: "View todolist", + }, + { + Action: "list", + Cmd: fmt.Sprintf("basecamp todolists list --in %s", proj), + Description: "List todolists", + }, + } + } + } else { + summary = fmt.Sprintf("Reordered %d todolists to the top of the todoset", len(parsed)) + // Build from authoritative preflight data, not ambient project. + breadcrumbs = []output.Breadcrumb{ + { + Action: "list", + Cmd: fmt.Sprintf("basecamp todolists list --in %d --todoset %d", bucketID, parentID), + Description: "List todolists", + }, + } + } + + opts := []output.ResponseOption{output.WithSummary(summary)} + if len(breadcrumbs) > 0 { + opts = append(opts, output.WithBreadcrumbs(breadcrumbs...)) + } + + return app.OK(map[string]any{ + "repositioned": true, + "position": position, + "todolist_ids": parsed, + }, opts...) + }, + } + + cmd.Flags().IntVar(&position, "to", 0, "Target position, 1-based (1 = top)") + cmd.Flags().IntVar(&position, "position", 0, "Target position (alias for --to)") + + return cmd +} diff --git a/internal/commands/todolists_test.go b/internal/commands/todolists_test.go index 95b9e6db1..c7f73afa1 100644 --- a/internal/commands/todolists_test.go +++ b/internal/commands/todolists_test.go @@ -1,17 +1,609 @@ package commands import ( + "bytes" "encoding/json" "errors" + "fmt" "io" "net/http" + "net/http/httptest" "strings" + "sync" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/auth" + "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/names" + "github.com/basecamp/basecamp-cli/internal/output" ) +// --- A. Guard tests (no-network transport, guards fire before any request) --- + +func TestTodolistsPositionSingleRequiresTo(t *testing.T) { + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + app, _ := setupTestApp(t) + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "789") + + require.NotNil(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.Equal(t, "--to is required (1 = top)", e.Message) +} + +func TestTodolistsPositionRejectsZeroAndNegative(t *testing.T) { + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + + cases := []struct { + name string + args []string + }{ + {"single --to 0", []string{"789", "--to", "0"}}, + {"single --to -1", []string{"789", "--to", "-1"}}, + {"bulk --to 0", []string{"701", "702", "--to", "0"}}, + {"bulk --to -1", []string{"701", "702", "--to", "-1"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + app, _ := setupTestApp(t) + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, tc.args...) + + require.NotNil(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.Equal(t, "--to must be at least 1 (1 = top)", e.Message) + }) + } +} + +func TestTodolistsPositionAcceptsPositionAlias(t *testing.T) { + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + app, _ := setupTestApp(t) + + project := "" + cmd := newTodolistsPositionCmd(&project) + // --position 2 satisfies the requirement → guard does not fire; the command + // reaches the network layer and fails there, not with the usage message. + err := executeCommand(cmd, app, "789", "--position", "2") + + require.NotNil(t, err) + var e *output.Error + if errors.As(err, &e) { + assert.NotEqual(t, "--to is required (1 = top)", e.Message) + assert.NotEqual(t, "--to must be at least 1 (1 = top)", e.Message) + } +} + +func TestTodolistsPositionBulkRejectsNonOnePosition(t *testing.T) { + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + app, _ := setupTestApp(t) + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "701", "702", "--to", "2") + + require.NotNil(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.Contains(t, e.Message, "only supported at position 1") +} + +func TestTodolistsPositionBulkAllowsOmittedAndExplicitOne(t *testing.T) { + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + + for _, args := range [][]string{ + {"701", "702"}, + {"701", "702", "--to", "1"}, + } { + app, _ := setupTestApp(t) + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, args...) + + // Guard must not fire: the error comes from the preflight network call. + require.NotNil(t, err) + var e *output.Error + if errors.As(err, &e) { + assert.NotContains(t, e.Message, "only supported at position 1") + assert.NotEqual(t, "--to is required (1 = top)", e.Message) + } + } +} + +func TestTodolistsPositionEmptyAfterExtraction(t *testing.T) { + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + app, _ := setupTestApp(t) + + project := "" + cmd := newTodolistsPositionCmd(&project) + // A bare comma extracts to zero IDs — must report the missing-arg usage error. + err := executeCommand(cmd, app, ",", "--to", "1") + + require.NotNil(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.Equal(t, "... required", e.Message) +} + +func TestTodolistsPositionInvalidID(t *testing.T) { + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + app, _ := setupTestApp(t) + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "abc", "--to", "1") + + require.NotNil(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.Equal(t, "Invalid todolist ID", e.Message) +} + +func TestTodolistsPositionDuplicateID(t *testing.T) { + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + app, _ := setupTestApp(t) + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "701", "701") + + require.NotNil(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.Contains(t, e.Message, "Duplicate todolist ID 701") +} + +func TestTodolistsPositionURLArgPassesValidation(t *testing.T) { + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + app, _ := setupTestApp(t) + + project := "" + cmd := newTodolistsPositionCmd(&project) + // A todolist URL should extract its ID and get past validation, reaching the + // network layer (which fails) — not a "Invalid todolist ID" usage error. + err := executeCommand(cmd, app, + "https://3.basecamp.com/99999/buckets/2/todolists/789", "--to", "1") + + require.NotNil(t, err) + var e *output.Error + if errors.As(err, &e) { + assert.NotEqual(t, "Invalid todolist ID", e.Message) + } +} + +// --- B. Request-level tests (httptest server records methods/paths/bodies) --- + +type recordedRequest struct { + method string + path string + body map[string]any +} + +type requestRecorder struct { + mu sync.Mutex + requests []recordedRequest +} + +func (r *requestRecorder) record(req *http.Request) { + r.mu.Lock() + defer r.mu.Unlock() + var body map[string]any + if req.Body != nil { + data, _ := io.ReadAll(req.Body) + if len(data) > 0 { + _ = json.Unmarshal(data, &body) + } + } + r.requests = append(r.requests, recordedRequest{ + method: req.Method, + path: req.URL.Path, + body: body, + }) +} + +func (r *requestRecorder) puts() []recordedRequest { + r.mu.Lock() + defer r.mu.Unlock() + var out []recordedRequest + for _, req := range r.requests { + if req.method == http.MethodPut { + out = append(out, req) + } + } + return out +} + +// todolistGetResponse describes a canned Get response for a todolist ID. +type todolistGetResponse struct { + status int + parentID int64 + bucketID int64 + completed bool + nilParent bool + nilBucket bool +} + +// newTodolistsPositionServer builds an httptest server. getResponses maps a +// todolist ID to its Get response; putStatus maps a todolist ID to the status +// its reposition PUT returns (defaults to 204 when absent). +func newTodolistsPositionServer(t *testing.T, rec *requestRecorder, getResponses map[int64]todolistGetResponse, putStatus map[int64]int) *httptest.Server { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + rec.record(r) + w.Header().Set("Content-Type", "application/json") + + if r.Method == http.MethodPut && strings.Contains(r.URL.Path, "/position.json") { + id := idFromPositionPath(r.URL.Path) + status := http.StatusNoContent + if s, ok := putStatus[id]; ok { + status = s + } + w.WriteHeader(status) + if status >= 400 { + fmt.Fprintf(w, `{"error":"boom"}`) + } + return + } + + if r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/todolists/") { + id := idFromGetPath(r.URL.Path) + resp, ok := getResponses[id] + if !ok { + w.WriteHeader(http.StatusNotFound) + fmt.Fprintf(w, `{"error":"not found"}`) + return + } + if resp.status != 0 && resp.status != http.StatusOK { + w.WriteHeader(resp.status) + fmt.Fprintf(w, `{"error":"not found"}`) + return + } + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, todolistJSON(id, resp)) + return + } + + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, `{}`) + })) + t.Cleanup(server.Close) + return server +} + +func idFromPositionPath(path string) int64 { + // .../todosets/todolists/{id}/position.json + parts := strings.Split(path, "/") + for i, p := range parts { + if p == "todolists" && i+1 < len(parts) { + var id int64 + fmt.Sscanf(parts[i+1], "%d", &id) + return id + } + } + return 0 +} + +func idFromGetPath(path string) int64 { + // .../todolists/{id} + parts := strings.Split(path, "/") + last := parts[len(parts)-1] + last = strings.TrimSuffix(last, ".json") + var id int64 + fmt.Sscanf(last, "%d", &id) + return id +} + +func todolistJSON(id int64, resp todolistGetResponse) string { + parent := fmt.Sprintf(`,"parent":{"id":%d,"title":"Set","type":"Todoset"}`, resp.parentID) + if resp.nilParent { + parent = "" + } + bucket := fmt.Sprintf(`,"bucket":{"id":%d,"name":"Proj","type":"Project"}`, resp.bucketID) + if resp.nilBucket { + bucket = "" + } + return fmt.Sprintf(`{"id":%d,"name":"List %d","completed":%t%s%s}`, + id, id, resp.completed, parent, bucket) +} + +// newRequestLevelApp wires an app to a live test server URL. +func newRequestLevelApp(t *testing.T, serverURL string) (*appctx.App, *bytes.Buffer) { + t.Helper() + t.Setenv("BASECAMP_NO_KEYRING", "1") + + buf := &bytes.Buffer{} + cfg := &config.Config{AccountID: "99999"} + authMgr := auth.NewManager(cfg, nil) + sdkClient := basecamp.NewClient( + &basecamp.Config{BaseURL: serverURL}, + &testTokenProvider{}, + basecamp.WithMaxRetries(1), + ) + nameResolver := names.NewResolver(sdkClient, authMgr, cfg.AccountID) + + app := &appctx.App{ + Config: cfg, + Auth: authMgr, + SDK: sdkClient, + Names: nameResolver, + Output: output.New(output.Options{Format: output.FormatJSON, Writer: buf}), + } + app.Flags.Hints = true // emit breadcrumbs (stripped otherwise) + return app, buf +} + +func TestTodolistsPositionBulkReverseOrderAndBodies(t *testing.T) { + rec := &requestRecorder{} + gets := map[int64]todolistGetResponse{ + 701: {parentID: 10, bucketID: 20}, + 702: {parentID: 10, bucketID: 20}, + 703: {parentID: 10, bucketID: 20}, + } + server := newTodolistsPositionServer(t, rec, gets, nil) + app, buf := newRequestLevelApp(t, server.URL) + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "701", "702", "703") + require.NoError(t, err) + + puts := rec.puts() + require.Len(t, puts, 3) + // Reverse apply order: 703, 702, 701; each body {"position":1}. + assert.Equal(t, int64(703), idFromPositionPath(puts[0].path)) + assert.Equal(t, int64(702), idFromPositionPath(puts[1].path)) + assert.Equal(t, int64(701), idFromPositionPath(puts[2].path)) + for _, p := range puts { + assert.Contains(t, p.path, "/todosets/todolists/") + assert.Equal(t, float64(1), p.body["position"]) + } + + // Output shape: typed top→bottom order, not reversed. + env := decodeEnvelope(t, buf) + assert.True(t, env.OK) + assert.Equal(t, true, env.Data["repositioned"]) + assert.Equal(t, float64(1), env.Data["position"]) + assert.Equal(t, []any{float64(701), float64(702), float64(703)}, env.Data["todolist_ids"]) +} + +func TestTodolistsPositionSingleBody(t *testing.T) { + rec := &requestRecorder{} + server := newTodolistsPositionServer(t, rec, nil, nil) + app, _ := newRequestLevelApp(t, server.URL) + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "789", "--to", "3") + require.NoError(t, err) + + puts := rec.puts() + require.Len(t, puts, 1) + assert.Equal(t, int64(789), idFromPositionPath(puts[0].path)) + assert.Equal(t, float64(3), puts[0].body["position"]) +} + +func TestTodolistsPositionRejectsDifferentParents(t *testing.T) { + rec := &requestRecorder{} + gets := map[int64]todolistGetResponse{ + 701: {parentID: 10, bucketID: 20}, + 702: {parentID: 11, bucketID: 20}, + } + server := newTodolistsPositionServer(t, rec, gets, nil) + app, _ := newRequestLevelApp(t, server.URL) + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "701", "702") + + require.NotNil(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.Contains(t, e.Message, "different todoset") + assert.Empty(t, rec.puts(), "no PUT should be issued when preflight fails") +} + +func TestTodolistsPositionRejectsNilParentOrBucket(t *testing.T) { + rec := &requestRecorder{} + gets := map[int64]todolistGetResponse{ + 701: {parentID: 10, bucketID: 20}, + 702: {nilParent: true, bucketID: 20}, + } + server := newTodolistsPositionServer(t, rec, gets, nil) + app, _ := newRequestLevelApp(t, server.URL) + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "701", "702") + + require.NotNil(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.Contains(t, e.Message, "missing its todoset or project context") + assert.Empty(t, rec.puts()) +} + +func TestTodolistsPositionRejectsCompletedList(t *testing.T) { + rec := &requestRecorder{} + gets := map[int64]todolistGetResponse{ + 701: {parentID: 10, bucketID: 20}, + 702: {parentID: 10, bucketID: 20, completed: true}, + } + server := newTodolistsPositionServer(t, rec, gets, nil) + app, _ := newRequestLevelApp(t, server.URL) + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "701", "702") + + require.NotNil(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.Contains(t, e.Message, "#702") + assert.Contains(t, e.Message, "completed") + assert.Empty(t, rec.puts()) +} + +func TestTodolistsPositionPreflightFailureMeansZeroMutations(t *testing.T) { + rec := &requestRecorder{} + gets := map[int64]todolistGetResponse{ + 701: {parentID: 10, bucketID: 20}, + 702: {status: http.StatusNotFound}, + } + server := newTodolistsPositionServer(t, rec, gets, nil) + app, _ := newRequestLevelApp(t, server.URL) + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "701", "702") + + require.NotNil(t, err) + assert.Empty(t, rec.puts(), "a preflight Get failure must issue no PUTs") +} + +func TestTodolistsPositionPartialFailureAccounting(t *testing.T) { + rec := &requestRecorder{} + gets := map[int64]todolistGetResponse{ + 701: {parentID: 10, bucketID: 20}, + 702: {parentID: 10, bucketID: 20}, + 703: {parentID: 10, bucketID: 20}, + } + // Apply order is 703, 702, 701. Fail on 702 (the second PUT). + server := newTodolistsPositionServer(t, rec, gets, map[int64]int{702: http.StatusForbidden}) + app, _ := newRequestLevelApp(t, server.URL) + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "701", "702", "703") + + require.NotNil(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + // One applied (703) before failing at 702. + assert.Contains(t, e.Message, "Reordered 1 of 3 todolists; failed at #702") + assert.Contains(t, e.Hint, "Rerun the whole command") + assert.Equal(t, http.StatusForbidden, e.HTTPStatus) + + // Exactly two PUTs recorded: it stopped instead of continuing to 701. + assert.Len(t, rec.puts(), 2) +} + +func TestTodolistsPositionBulkBreadcrumbUsesPreflightData(t *testing.T) { + rec := &requestRecorder{} + gets := map[int64]todolistGetResponse{ + 701: {parentID: 55, bucketID: 66}, + 702: {parentID: 55, bucketID: 66}, + } + server := newTodolistsPositionServer(t, rec, gets, nil) + app, buf := newRequestLevelApp(t, server.URL) + // Configure an unrelated project — the breadcrumb must ignore it. + app.Config.ProjectID = "999" + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "701", "702") + require.NoError(t, err) + + env := decodeEnvelope(t, buf) + require.NotEmpty(t, env.Breadcrumbs) + joined := breadcrumbCmds(env) + assert.Contains(t, joined, "--in 66") + assert.Contains(t, joined, "--todoset 55") + assert.NotContains(t, joined, "--in 999") +} + +func TestTodolistsPositionSingleBreadcrumbContextKnown(t *testing.T) { + rec := &requestRecorder{} + server := newTodolistsPositionServer(t, rec, nil, nil) + app, buf := newRequestLevelApp(t, server.URL) + + project := "myproj" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "789", "--to", "1") + require.NoError(t, err) + + env := decodeEnvelope(t, buf) + require.NotEmpty(t, env.Breadcrumbs) + joined := breadcrumbCmds(env) + assert.Contains(t, joined, "basecamp todolists show 789 --in myproj") + assert.Contains(t, joined, "basecamp todolists list --in myproj") +} + +func TestTodolistsPositionSingleBreadcrumbProjectFromURL(t *testing.T) { + rec := &requestRecorder{} + server := newTodolistsPositionServer(t, rec, nil, nil) + app, buf := newRequestLevelApp(t, server.URL) + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, + "https://3.basecamp.com/99999/buckets/42/todolists/789", "--to", "1") + require.NoError(t, err) + + env := decodeEnvelope(t, buf) + require.NotEmpty(t, env.Breadcrumbs) + joined := breadcrumbCmds(env) + assert.Contains(t, joined, "--in 42") +} + +func TestTodolistsPositionSingleBreadcrumbContextAbsent(t *testing.T) { + rec := &requestRecorder{} + server := newTodolistsPositionServer(t, rec, nil, nil) + app, buf := newRequestLevelApp(t, server.URL) + // No project anywhere. + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "789", "--to", "1") + require.NoError(t, err) + + // Exactly one request: the PUT. No project-resolution GETs. + require.Len(t, rec.requests, 1) + assert.Equal(t, http.MethodPut, rec.requests[0].method) + + env := decodeEnvelope(t, buf) + assert.Empty(t, env.Breadcrumbs, "no breadcrumbs when no project context is known") +} + +// --- helpers --- + +type testEnvelope struct { + OK bool `json:"ok"` + Data map[string]any `json:"data"` + Summary string `json:"summary"` + Breadcrumbs []map[string]any `json:"breadcrumbs"` +} + +func decodeEnvelope(t *testing.T, buf *bytes.Buffer) testEnvelope { + t.Helper() + var env testEnvelope + require.NoError(t, json.Unmarshal(buf.Bytes(), &env)) + return env +} + +func breadcrumbCmds(env testEnvelope) string { + var b strings.Builder + for _, bc := range env.Breadcrumbs { + if cmd, ok := bc["cmd"].(string); ok { + b.WriteString(cmd) + b.WriteString("\n") + } + } + return b.String() +} + +// --- Client-visibility create tests (from #554) --- + // mockTodolistCreateTransport resolves the todoset via the project dock and // captures the POST body sent to create a todolist. type mockTodolistCreateTransport struct { diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 913284ae1..1d7c421e3 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -559,8 +559,13 @@ basecamp todolists create "Name" --in --json # Create basecamp todolists create "Name" --description "Desc" --in basecamp todolists create "Name" --visible-to-clients --in # Visible to clients basecamp todolists update --name "New" --in # Update +basecamp todolists position --to 1 # Reorder one list (1 = top) +basecamp todolists position # Order incomplete lists, top→bottom ``` +Bulk `position` sets the visible order in one command: pass incomplete lists from +the same todoset, top to bottom. It always places them at the top. + ### Cards (Kanban) **Note:** Cards do NOT support `--assignee` filtering like todos. Fetch all cards and filter client-side if needed. If a project has multiple card tables, you must specify `--card-table `. When you get an "Ambiguous card table" error, the hint shows available table IDs and names. From 2f5ca72768be5dc45358d10da94f834c1249c2a7 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 24 Jul 2026 12:38:45 -0700 Subject: [PATCH 2/5] Preserve error classification on partial reposition failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bulk-apply failure path reclassified every non-*output.Error (raw transport/runtime failures, context cancellation) as code=usage via ErrUsageHint, overwrote the SDK's structured hint with the rerun hint, and claimed the todoset was left in an intermediate order even when the failing PUT was the first/only one and nothing had been mutated. Now: when zero PUTs have landed (always true in single-list mode, and when a bulk run's first PUT fails), surface the underlying error unchanged — its code, HTTP status, and hint intact, no false intermediate-order claim. Only once a partial reorder has actually landed does it add the applied-count accounting and rerun hint, and even then it preserves the underlying error's classification (appending the rerun text to the original hint rather than replacing it) and wraps raw errors with %w instead of reclassifying them as usage. Adds regression coverage for Code/HTTPStatus preservation, original-hint preservation, and the zero-applied single-list and bulk-first-PUT paths. --- internal/commands/todolists.go | 20 +++++++++- internal/commands/todolists_test.go | 58 ++++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 4 deletions(-) diff --git a/internal/commands/todolists.go b/internal/commands/todolists.go index 890cccdcf..56cd31727 100644 --- a/internal/commands/todolists.go +++ b/internal/commands/todolists.go @@ -570,10 +570,26 @@ Confirm with ` + "`basecamp todolists list`" + `.`, for i := len(parsed) - 1; i >= 0; i-- { if err := app.Account().Todolists().Reposition(cmd.Context(), parsed[i], position); err != nil { converted := convertSDKError(err) + // No PUT has landed yet (single-list mode always takes this + // path): the todoset is untouched. Surface the underlying + // error unchanged — keep its code/category/hint and don't + // claim an intermediate order that doesn't exist. + if applied == 0 { + return converted + } + // A partial reorder has landed: the todoset is now in an + // intermediate order. Add applied-count accounting and a + // rerun hint while preserving the underlying error's + // classification (never reclassify a transport/runtime + // failure as a usage error). msg := fmt.Sprintf("Reordered %d of %d todolists; failed at #%d", applied, len(parsed), parsed[i]) - hint := "Rerun the whole command once the cause is fixed; the todoset is now in an intermediate order." + const rerun = "Rerun the whole command once the cause is fixed; the todoset is now in an intermediate order." var outErr *output.Error if errors.As(converted, &outErr) { + hint := rerun + if outErr.Hint != "" { + hint = outErr.Hint + " " + rerun + } return &output.Error{ Code: outErr.Code, Message: fmt.Sprintf("%s: %s", msg, outErr.Message), @@ -583,7 +599,7 @@ Confirm with ` + "`basecamp todolists list`" + `.`, Cause: outErr, } } - return output.ErrUsageHint(fmt.Sprintf("%s: %s", msg, converted.Error()), hint) + return fmt.Errorf("%s: %w", msg, converted) } applied++ } diff --git a/internal/commands/todolists_test.go b/internal/commands/todolists_test.go index c7f73afa1..f5a1c2ca3 100644 --- a/internal/commands/todolists_test.go +++ b/internal/commands/todolists_test.go @@ -251,7 +251,9 @@ func newTodolistsPositionServer(t *testing.T, rec *requestRecorder, getResponses } w.WriteHeader(status) if status >= 400 { - fmt.Fprintf(w, `{"error":"boom"}`) + // error_description becomes the SDK error Hint — lets tests + // assert the underlying hint is preserved, not overwritten. + fmt.Fprint(w, `{"error":"denied","error_description":"check your role"}`) } return } @@ -491,13 +493,65 @@ func TestTodolistsPositionPartialFailureAccounting(t *testing.T) { require.True(t, errors.As(err, &e)) // One applied (703) before failing at 702. assert.Contains(t, e.Message, "Reordered 1 of 3 todolists; failed at #702") - assert.Contains(t, e.Hint, "Rerun the whole command") + // Underlying error classification is preserved, not reclassified as usage. + assert.Equal(t, "forbidden", e.Code) assert.Equal(t, http.StatusForbidden, e.HTTPStatus) + // Both the original SDK hint and the rerun hint survive. + assert.Contains(t, e.Hint, "check your role") + assert.Contains(t, e.Hint, "Rerun the whole command") // Exactly two PUTs recorded: it stopped instead of continuing to 701. assert.Len(t, rec.puts(), 2) } +// A single-list PUT failure means zero mutations: the error is surfaced with +// its own classification, with no accounting prefix and no false claim that the +// todoset is in an intermediate order. +func TestTodolistsPositionSingleFailureSurfacesUnderlyingError(t *testing.T) { + rec := &requestRecorder{} + server := newTodolistsPositionServer(t, rec, nil, map[int64]int{789: http.StatusNotFound}) + app, _ := newRequestLevelApp(t, server.URL) + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "789", "--to", "3") + + require.NotNil(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.Equal(t, "not_found", e.Code, "must not be reclassified as usage") + assert.Equal(t, http.StatusNotFound, e.HTTPStatus) + assert.NotContains(t, e.Message, "Reordered") + assert.NotContains(t, e.Hint, "intermediate order") + assert.Len(t, rec.puts(), 1) +} + +// When the very first applied PUT in a bulk run fails, nothing has been +// mutated yet — the error is surfaced directly, with no accounting prefix and +// no intermediate-order claim, and exactly one PUT is recorded. +func TestTodolistsPositionBulkFirstPutFailureMeansZeroApplied(t *testing.T) { + rec := &requestRecorder{} + gets := map[int64]todolistGetResponse{ + 701: {parentID: 10, bucketID: 20}, + 702: {parentID: 10, bucketID: 20}, + } + // Apply order is 702 then 701; fail on the first PUT (702). + server := newTodolistsPositionServer(t, rec, gets, map[int64]int{702: http.StatusNotFound}) + app, _ := newRequestLevelApp(t, server.URL) + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "701", "702") + + require.NotNil(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.Equal(t, "not_found", e.Code) + assert.NotContains(t, e.Message, "Reordered") + assert.NotContains(t, e.Hint, "intermediate order") + assert.Len(t, rec.puts(), 1, "stopped after the first failing PUT") +} + func TestTodolistsPositionBulkBreadcrumbUsesPreflightData(t *testing.T) { rec := &requestRecorder{} gets := map[int64]todolistGetResponse{ From 5711c750ba757d17b9a25ad407a9d3764e0c0e54 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 24 Jul 2026 12:56:12 -0700 Subject: [PATCH 3/5] Reject wrong-type URL args to todolists position MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pasted Basecamp URL for a different recording type (a todo, a card, or a todolists *collection* URL) still yields a trailing numeric ID via ExtractIDs, which — because this command mutates — would silently reposition an unrelated todolist that happens to share that ID. Validate any URL argument up front: it must be a non-collection `todolists` URL, mirroring the guard todos position already applies to its --list destination URL. Bare numeric IDs are unaffected. --- internal/commands/todolists.go | 20 ++++++++++++++++++++ internal/commands/todolists_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/internal/commands/todolists.go b/internal/commands/todolists.go index 56cd31727..96cdb1211 100644 --- a/internal/commands/todolists.go +++ b/internal/commands/todolists.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "strconv" + "strings" "github.com/spf13/cobra" @@ -11,6 +12,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/urlarg" ) // NewTodolistsCmd creates the todolists command group. @@ -495,6 +497,24 @@ Confirm with ` + "`basecamp todolists list`" + `.`, return err } + // A pasted URL for a different recording type (a todo, card, or a + // collection URL) still extracts a trailing numeric ID, which would + // silently reposition an unrelated todolist. Require any URL arg to + // be a non-collection todolists URL before trusting its ID. + for _, arg := range args { + for _, token := range strings.Split(arg, ",") { + token = strings.TrimSpace(token) + if token == "" { + continue + } + if parsed := urlarg.Parse(token); parsed != nil { + if parsed.RecordingID == "" || parsed.Type != "todolists" || parsed.IsCollection { + return output.ErrUsage("Expected a todolist URL (.../todolists/), or pass a todolist ID.") + } + } + } + } + // ExtractIDs drops empty segments, so a bare "," yields zero IDs. ids := extractIDs(args) if len(ids) == 0 { diff --git a/internal/commands/todolists_test.go b/internal/commands/todolists_test.go index f5a1c2ca3..a309ae23a 100644 --- a/internal/commands/todolists_test.go +++ b/internal/commands/todolists_test.go @@ -182,6 +182,32 @@ func TestTodolistsPositionURLArgPassesValidation(t *testing.T) { } } +func TestTodolistsPositionRejectsWrongURLType(t *testing.T) { + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + + cases := []struct { + name string + url string + }{ + {"todo URL", "https://3.basecamp.com/99999/buckets/2/todos/789"}, + {"card URL", "https://3.basecamp.com/99999/buckets/2/card_tables/cards/789"}, + {"todolists collection URL", "https://3.basecamp.com/99999/buckets/2/todolists"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + app, _ := setupTestApp(t) + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, tc.url, "--to", "1") + + require.NotNil(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.Contains(t, e.Message, "Expected a todolist URL") + }) + } +} + // --- B. Request-level tests (httptest server records methods/paths/bodies) --- type recordedRequest struct { From 6499a183e0f1f4ac7ae4cf81591ee2f857510597 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 24 Jul 2026 14:55:45 -0700 Subject: [PATCH 4/5] Warn of partial reorder when a transport failure interrupts a bulk run The raw-error branch of the partial-failure path wrapped the cause with %w and the applied-count prefix, but dropped the recovery hint entirely. After one successful PUT followed by a transport failure (a dropped connection, a canceled context), the operator saw the count but was never told the todoset was left partially reordered and needs a rerun. Return a non-usage structured error that carries the applied-count message, the rerun/intermediate-order hint, and the raw cause (via Cause, so errors.Is/As still unwrap). Code is api_error, never usage. Adds a regression test that drops the connection on the second applied PUT and asserts the non-usage code, the accounting message, the rerun hint, the preserved cause, and that the third list is never touched. --- go.sum | 2 - internal/commands/todolists.go | 12 +++++- internal/commands/todolists_test.go | 60 ++++++++++++++++++++++++++++- 3 files changed, 70 insertions(+), 4 deletions(-) diff --git a/go.sum b/go.sum index 87b642f8f..14c85fd96 100644 --- a/go.sum +++ b/go.sum @@ -25,8 +25,6 @@ github.com/aymerick/douceur v0.2.0 h1:Mv+mAeH1Q+n9Fr+oyamOlAkUNPWPlA8PPGR0QAaYuP github.com/aymerick/douceur v0.2.0/go.mod h1:wlT5vV2O3h55X9m7iVYN0TBM0NH/MmbLnd30/FjWUq4= github.com/basecamp/basecamp-sdk/go v0.8.1-0.20260724184307-e2c1abea4aea h1:dRwhvhnzbyXxitGbb3uGFaUIJHazztD9UgWC0XzM870= github.com/basecamp/basecamp-sdk/go v0.8.1-0.20260724184307-e2c1abea4aea/go.mod h1:eX5mEKCdtxSfEL4P/n5AwOl21JVA/K+gRPic/Hd8W/Y= -github.com/basecamp/basecamp-sdk/go v0.8.1-0.20260723015714-93ace8d569b5 h1:M0KUktNpOI3gP2rQ0YG6AfjTqm8HTQXuJiErvBn7ttA= -github.com/basecamp/basecamp-sdk/go v0.8.1-0.20260723015714-93ace8d569b5/go.mod h1:eX5mEKCdtxSfEL4P/n5AwOl21JVA/K+gRPic/Hd8W/Y= github.com/basecamp/cli v0.2.1 h1:8GyehPVtsTXla0oOPu4QgXRjwwzJ99prlByvyi+0HRQ= github.com/basecamp/cli v0.2.1/go.mod h1:p8tt/DatJ2LAzWO6N6tNfV8x3gF5T3IxDTo+U8FfWPo= github.com/bmatcuk/doublestar v1.1.1/go.mod h1:UD6OnuiIn0yFxxA2le/rnRU1G4RaI4UvFv1sNto9p6w= diff --git a/internal/commands/todolists.go b/internal/commands/todolists.go index 96cdb1211..9405e69a7 100644 --- a/internal/commands/todolists.go +++ b/internal/commands/todolists.go @@ -619,7 +619,17 @@ Confirm with ` + "`basecamp todolists list`" + `.`, Cause: outErr, } } - return fmt.Errorf("%s: %w", msg, converted) + // Raw transport/runtime failure (e.g. a dropped connection or + // a canceled context): no *output.Error to preserve, but the + // partial reorder still demands the rerun warning. Wrap it in + // a non-usage structured error that carries the hint and the + // raw cause (Unwrap keeps errors.Is/As working). + return &output.Error{ + Code: output.CodeAPI, + Message: fmt.Sprintf("%s: %s", msg, converted.Error()), + Hint: rerun, + Cause: converted, + } } applied++ } diff --git a/internal/commands/todolists_test.go b/internal/commands/todolists_test.go index a309ae23a..32441096d 100644 --- a/internal/commands/todolists_test.go +++ b/internal/commands/todolists_test.go @@ -11,6 +11,7 @@ import ( "strings" "sync" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -260,9 +261,15 @@ type todolistGetResponse struct { nilBucket bool } +// putHijack is a sentinel putStatus value that makes the server drop the +// connection mid-request, forcing a raw transport error. +const putHijack = -1 + // newTodolistsPositionServer builds an httptest server. getResponses maps a // todolist ID to its Get response; putStatus maps a todolist ID to the status -// its reposition PUT returns (defaults to 204 when absent). +// its reposition PUT returns (defaults to 204 when absent). A putStatus value of +// putHijack simulates a transport failure by dropping the connection before any +// response, so the SDK surfaces a raw (non-*basecamp.Error) error. func newTodolistsPositionServer(t *testing.T, rec *requestRecorder, getResponses map[int64]todolistGetResponse, putStatus map[int64]int) *httptest.Server { t.Helper() server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -275,6 +282,16 @@ func newTodolistsPositionServer(t *testing.T, rec *requestRecorder, getResponses if s, ok := putStatus[id]; ok { status = s } + if status == putHijack { + // Drop the connection mid-request → client sees a transport + // error, not an HTTP status. + hj, ok := w.(http.Hijacker) + require.True(t, ok, "test server must support hijacking") + conn, _, err := hj.Hijack() + require.NoError(t, err) + _ = conn.Close() + return + } w.WriteHeader(status) if status >= 400 { // error_description becomes the SDK error Hint — lets tests @@ -357,6 +374,10 @@ func newRequestLevelApp(t *testing.T, serverURL string) (*appctx.App, *bytes.Buf &basecamp.Config{BaseURL: serverURL}, &testTokenProvider{}, basecamp.WithMaxRetries(1), + // Keep any retry backoff near-instant so transport-failure tests + // don't stall on exponential waits. + basecamp.WithBaseDelay(time.Millisecond), + basecamp.WithMaxJitter(0), ) nameResolver := names.NewResolver(sdkClient, authMgr, cfg.AccountID) @@ -578,6 +599,43 @@ func TestTodolistsPositionBulkFirstPutFailureMeansZeroApplied(t *testing.T) { assert.Len(t, rec.puts(), 1, "stopped after the first failing PUT") } +// A raw transport failure after a successful PUT must still warn that the +// todoset is partially reordered: non-usage code, applied-count accounting, the +// rerun hint, and the raw cause preserved. +func TestTodolistsPositionPartialTransportFailureWarnsIntermediate(t *testing.T) { + rec := &requestRecorder{} + gets := map[int64]todolistGetResponse{ + 701: {parentID: 10, bucketID: 20}, + 702: {parentID: 10, bucketID: 20}, + 703: {parentID: 10, bucketID: 20}, + } + // Apply order is 703, 702, 701. 703 succeeds; 702's connection is dropped. + server := newTodolistsPositionServer(t, rec, gets, map[int64]int{702: putHijack}) + app, _ := newRequestLevelApp(t, server.URL) + + project := "" + cmd := newTodolistsPositionCmd(&project) + err := executeCommand(cmd, app, "701", "702", "703") + + require.NotNil(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.NotEqual(t, "usage", e.Code, "transport failure must not be classified as usage") + assert.Contains(t, e.Message, "Reordered 1 of 3 todolists; failed at #702") + assert.Contains(t, e.Hint, "intermediate order", "operator must be warned of the partial reorder") + assert.NotNil(t, e.Cause, "raw transport cause preserved") + // The transport failure may be retried, so assert on which lists were + // touched rather than a raw request count: 703 applied, 702 attempted, + // and 701 never reached (the loop stopped). + touched := map[int64]bool{} + for _, p := range rec.puts() { + touched[idFromPositionPath(p.path)] = true + } + assert.True(t, touched[703], "703 should have been repositioned") + assert.True(t, touched[702], "702 should have been attempted") + assert.False(t, touched[701], "701 must not be attempted after the failure") +} + func TestTodolistsPositionBulkBreadcrumbUsesPreflightData(t *testing.T) { rec := &requestRecorder{} gets := map[int64]todolistGetResponse{ From fed08991ae870b7737923149dd9fc08344a6a813 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 24 Jul 2026 14:55:45 -0700 Subject: [PATCH 5/5] Reconcile SDK pin references and surface after rebase onto e2c1abea Rebasing onto main (#554) advanced the SDK pin to e2c1abea, five commits past the 93ace8d5 this branch first pinned. Refresh the API-COVERAGE SDK line to the merged pin (which additionally carries create-time visible_to_clients support) and regenerate the CLI surface so it reflects both todolists position and main's todolists create --visible-to-clients. --- API-COVERAGE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/API-COVERAGE.md b/API-COVERAGE.md index 7c606d320..42589e3b9 100644 --- a/API-COVERAGE.md +++ b/API-COVERAGE.md @@ -16,7 +16,7 @@ Out-of-scope sections are excluded from parity totals and scripts: chatbots (dif > Note: the per-row `Endpoints` column in the Coverage by Section table sums higher than the Summary totals above. The discrepancy predates the BC5 baseline; the row count (47 sections) is authoritative for the `Since` column. Reconciling endpoint counts is pre-existing maintenance, tracked separately. -**SDK version:** v0.8.1-0.20260723015714-93ace8d569b5 — adds `Todolists.Reposition` (`PUT /todosets/todolists/{id}/position.json`, basecamp/basecamp-sdk#398), consumed by `todolists position`; also carries `Campfires.UpdateLine` (`PUT /chats/{c}/lines/{l}`, basecamp/basecamp-sdk#295), consumed by `chat update`, and the BC5 `Tools.Create` create-by-type reshape. API date 2026-07-22. +**SDK version:** v0.8.1-0.20260724184307-e2c1abea4aea — adds `Todolists.Reposition` (`PUT /todosets/todolists/{id}/position.json`, basecamp/basecamp-sdk#398), consumed by `todolists position`; also carries the create-time `visible_to_clients` support behind `todolists create --visible-to-clients`, `Campfires.UpdateLine` (`PUT /chats/{c}/lines/{l}`, basecamp/basecamp-sdk#295), consumed by `chat update`, and the BC5 `Tools.Create` create-by-type reshape. API date 2026-07-24. ## Coverage by Section