From 97cd27f1f9ece4fda676e95a1254792e2e562055 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 27 Oct 2026 23:55:37 -0500 Subject: [PATCH 1/6] Create, show, update and delete timesheet entries The CLI read timesheets through the three reports but couldn't log time: SDK v0.29.1's TimesheetService Get, Create, Update and Destroy went unused, while API-COVERAGE.md claimed the whole set. timesheet entry create logs hours against an item (date defaults to today), show and update address one entry by id or URL, and delete removes it for good behind --force or a confirmation, since bc3 never trashes an entry. --- .surface | 151 +++++++ API-COVERAGE.md | 2 +- e2e/smoke/run_smoke.sh | 1 + e2e/smoke/smoke_timesheet_write.bats | 54 +++ e2e/timesheet.bats | 39 ++ internal/commands/commands.go | 2 +- internal/commands/timesheet.go | 6 +- internal/commands/timesheet_entries.go | 444 ++++++++++++++++++++ internal/commands/timesheet_entries_test.go | 238 +++++++++++ skills/basecamp/SKILL.md | 22 + 10 files changed, 955 insertions(+), 4 deletions(-) create mode 100644 e2e/smoke/smoke_timesheet_write.bats create mode 100644 internal/commands/timesheet_entries.go create mode 100644 internal/commands/timesheet_entries_test.go diff --git a/.surface b/.surface index 7931f86a4..a8ee1dcbd 100644 --- a/.surface +++ b/.surface @@ -410,6 +410,10 @@ ARG basecamp templates delete 00 ARG basecamp templates show 00 ARG basecamp templates update 00 ARG basecamp timeline 00 [me] +ARG basecamp timesheet entry create 00 +ARG basecamp timesheet entry delete 00 +ARG basecamp timesheet entry show 00 +ARG basecamp timesheet entry update 00 ARG basecamp timesheet item 00 ARG basecamp timesheet recording 00 ARG basecamp tlgroup copy 00 @@ -1169,6 +1173,11 @@ CMD basecamp templates show CMD basecamp templates update CMD basecamp timeline CMD basecamp timesheet +CMD basecamp timesheet entry +CMD basecamp timesheet entry create +CMD basecamp timesheet entry delete +CMD basecamp timesheet entry show +CMD basecamp timesheet entry update CMD basecamp timesheet item CMD basecamp timesheet project CMD basecamp timesheet recording @@ -15734,6 +15743,143 @@ FLAG basecamp timesheet --styled type=bool FLAG basecamp timesheet --to type=string FLAG basecamp timesheet --todolist type=string FLAG basecamp timesheet --verbose type=count +FLAG basecamp timesheet entry --account type=string +FLAG basecamp timesheet entry --agent type=bool +FLAG basecamp timesheet entry --cache-dir type=string +FLAG basecamp timesheet entry --count type=bool +FLAG basecamp timesheet entry --end type=string +FLAG basecamp timesheet entry --from type=string +FLAG basecamp timesheet entry --help type=bool +FLAG basecamp timesheet entry --hints type=bool +FLAG basecamp timesheet entry --ids-only type=bool +FLAG basecamp timesheet entry --in type=string +FLAG basecamp timesheet entry --jq type=string +FLAG basecamp timesheet entry --json type=bool +FLAG basecamp timesheet entry --markdown type=bool +FLAG basecamp timesheet entry --md type=bool +FLAG basecamp timesheet entry --no-hints type=bool +FLAG basecamp timesheet entry --no-stats type=bool +FLAG basecamp timesheet entry --person type=string +FLAG basecamp timesheet entry --profile type=string +FLAG basecamp timesheet entry --project type=string +FLAG basecamp timesheet entry --quiet type=bool +FLAG basecamp timesheet entry --start type=string +FLAG basecamp timesheet entry --stats type=bool +FLAG basecamp timesheet entry --styled type=bool +FLAG basecamp timesheet entry --to type=string +FLAG basecamp timesheet entry --todolist type=string +FLAG basecamp timesheet entry --verbose type=count +FLAG basecamp timesheet entry create --account type=string +FLAG basecamp timesheet entry create --agent type=bool +FLAG basecamp timesheet entry create --cache-dir type=string +FLAG basecamp timesheet entry create --count type=bool +FLAG basecamp timesheet entry create --date type=string +FLAG basecamp timesheet entry create --description type=string +FLAG basecamp timesheet entry create --end type=string +FLAG basecamp timesheet entry create --from type=string +FLAG basecamp timesheet entry create --help type=bool +FLAG basecamp timesheet entry create --hints type=bool +FLAG basecamp timesheet entry create --hours type=string +FLAG basecamp timesheet entry create --ids-only type=bool +FLAG basecamp timesheet entry create --in type=string +FLAG basecamp timesheet entry create --jq type=string +FLAG basecamp timesheet entry create --json type=bool +FLAG basecamp timesheet entry create --markdown type=bool +FLAG basecamp timesheet entry create --md type=bool +FLAG basecamp timesheet entry create --no-hints type=bool +FLAG basecamp timesheet entry create --no-stats type=bool +FLAG basecamp timesheet entry create --person type=string +FLAG basecamp timesheet entry create --profile type=string +FLAG basecamp timesheet entry create --project type=string +FLAG basecamp timesheet entry create --quiet type=bool +FLAG basecamp timesheet entry create --start type=string +FLAG basecamp timesheet entry create --stats type=bool +FLAG basecamp timesheet entry create --styled type=bool +FLAG basecamp timesheet entry create --to type=string +FLAG basecamp timesheet entry create --todolist type=string +FLAG basecamp timesheet entry create --verbose type=count +FLAG basecamp timesheet entry delete --account type=string +FLAG basecamp timesheet entry delete --agent type=bool +FLAG basecamp timesheet entry delete --cache-dir type=string +FLAG basecamp timesheet entry delete --count type=bool +FLAG basecamp timesheet entry delete --end type=string +FLAG basecamp timesheet entry delete --force type=bool +FLAG basecamp timesheet entry delete --from type=string +FLAG basecamp timesheet entry delete --help type=bool +FLAG basecamp timesheet entry delete --hints type=bool +FLAG basecamp timesheet entry delete --ids-only type=bool +FLAG basecamp timesheet entry delete --in type=string +FLAG basecamp timesheet entry delete --jq type=string +FLAG basecamp timesheet entry delete --json type=bool +FLAG basecamp timesheet entry delete --markdown type=bool +FLAG basecamp timesheet entry delete --md type=bool +FLAG basecamp timesheet entry delete --no-hints type=bool +FLAG basecamp timesheet entry delete --no-stats type=bool +FLAG basecamp timesheet entry delete --person type=string +FLAG basecamp timesheet entry delete --profile type=string +FLAG basecamp timesheet entry delete --project type=string +FLAG basecamp timesheet entry delete --quiet type=bool +FLAG basecamp timesheet entry delete --start type=string +FLAG basecamp timesheet entry delete --stats type=bool +FLAG basecamp timesheet entry delete --styled type=bool +FLAG basecamp timesheet entry delete --to type=string +FLAG basecamp timesheet entry delete --todolist type=string +FLAG basecamp timesheet entry delete --verbose type=count +FLAG basecamp timesheet entry show --account type=string +FLAG basecamp timesheet entry show --agent type=bool +FLAG basecamp timesheet entry show --cache-dir type=string +FLAG basecamp timesheet entry show --count type=bool +FLAG basecamp timesheet entry show --end type=string +FLAG basecamp timesheet entry show --from type=string +FLAG basecamp timesheet entry show --help type=bool +FLAG basecamp timesheet entry show --hints type=bool +FLAG basecamp timesheet entry show --ids-only type=bool +FLAG basecamp timesheet entry show --in type=string +FLAG basecamp timesheet entry show --jq type=string +FLAG basecamp timesheet entry show --json type=bool +FLAG basecamp timesheet entry show --markdown type=bool +FLAG basecamp timesheet entry show --md type=bool +FLAG basecamp timesheet entry show --no-hints type=bool +FLAG basecamp timesheet entry show --no-stats type=bool +FLAG basecamp timesheet entry show --person type=string +FLAG basecamp timesheet entry show --profile type=string +FLAG basecamp timesheet entry show --project type=string +FLAG basecamp timesheet entry show --quiet type=bool +FLAG basecamp timesheet entry show --start type=string +FLAG basecamp timesheet entry show --stats type=bool +FLAG basecamp timesheet entry show --styled type=bool +FLAG basecamp timesheet entry show --to type=string +FLAG basecamp timesheet entry show --todolist type=string +FLAG basecamp timesheet entry show --verbose type=count +FLAG basecamp timesheet entry update --account type=string +FLAG basecamp timesheet entry update --agent type=bool +FLAG basecamp timesheet entry update --cache-dir type=string +FLAG basecamp timesheet entry update --count type=bool +FLAG basecamp timesheet entry update --date type=string +FLAG basecamp timesheet entry update --description type=string +FLAG basecamp timesheet entry update --end type=string +FLAG basecamp timesheet entry update --from type=string +FLAG basecamp timesheet entry update --help type=bool +FLAG basecamp timesheet entry update --hints type=bool +FLAG basecamp timesheet entry update --hours type=string +FLAG basecamp timesheet entry update --ids-only type=bool +FLAG basecamp timesheet entry update --in type=string +FLAG basecamp timesheet entry update --jq type=string +FLAG basecamp timesheet entry update --json type=bool +FLAG basecamp timesheet entry update --markdown type=bool +FLAG basecamp timesheet entry update --md type=bool +FLAG basecamp timesheet entry update --no-hints type=bool +FLAG basecamp timesheet entry update --no-stats type=bool +FLAG basecamp timesheet entry update --person type=string +FLAG basecamp timesheet entry update --profile type=string +FLAG basecamp timesheet entry update --project type=string +FLAG basecamp timesheet entry update --quiet type=bool +FLAG basecamp timesheet entry update --start type=string +FLAG basecamp timesheet entry update --stats type=bool +FLAG basecamp timesheet entry update --styled type=bool +FLAG basecamp timesheet entry update --to type=string +FLAG basecamp timesheet entry update --todolist type=string +FLAG basecamp timesheet entry update --verbose type=count FLAG basecamp timesheet item --account type=string FLAG basecamp timesheet item --agent type=bool FLAG basecamp timesheet item --cache-dir type=string @@ -21224,6 +21370,11 @@ SUB basecamp templates show SUB basecamp templates update SUB basecamp timeline SUB basecamp timesheet +SUB basecamp timesheet entry +SUB basecamp timesheet entry create +SUB basecamp timesheet entry delete +SUB basecamp timesheet entry show +SUB basecamp timesheet entry update SUB basecamp timesheet item SUB basecamp timesheet project SUB basecamp timesheet recording diff --git a/API-COVERAGE.md b/API-COVERAGE.md index c07aadd28..b3853c0f0 100644 --- a/API-COVERAGE.md +++ b/API-COVERAGE.md @@ -274,7 +274,7 @@ cannot faithfully cover at least one endpoint for a reason outside the CLI. A | **Templates** | | templates | 10 | `templates` | ✅ | BC4 | - | list, show, create, update, delete, construct, construction, library, copy, copy-status | | **Time Tracking** | -| timesheets | 6 | `timesheet` | ✅ | BC4 | - | list, show, create, update, delete | +| timesheets | 7 | `timesheet` | ✅ | BC4 | - | Reports: account-wide (`timesheet report`), project (`timesheet project`), item (`timesheet item`). Entries: `timesheet entry show`, `create` (on an item), `update`, `delete` (permanent: bc3 refuses to trash or archive an entry) | | **Subscriptions** | | subscriptions | 4 | `subscriptions` | ✅ | BC4 | - | show, subscribe, unsubscribe, add/remove | | **Check-ins (Automatic)** | diff --git a/e2e/smoke/run_smoke.sh b/e2e/smoke/run_smoke.sh index 99aff3a23..b8bb6962e 100755 --- a/e2e/smoke/run_smoke.sh +++ b/e2e/smoke/run_smoke.sh @@ -92,6 +92,7 @@ echo "--- Level 1: Mutation tests (parallel, $jobs jobs) ---" level1=( "$SMOKE_DIR"/smoke_todos_write.bats "$SMOKE_DIR"/smoke_subtasks.bats + "$SMOKE_DIR"/smoke_timesheet_write.bats "$SMOKE_DIR"/smoke_messages_write.bats "$SMOKE_DIR"/smoke_files_write.bats "$SMOKE_DIR"/smoke_cards_write.bats diff --git a/e2e/smoke/smoke_timesheet_write.bats b/e2e/smoke/smoke_timesheet_write.bats new file mode 100644 index 000000000..b1e246f97 --- /dev/null +++ b/e2e/smoke/smoke_timesheet_write.bats @@ -0,0 +1,54 @@ +#!/usr/bin/env bats +# smoke_timesheet_write.bats - Level 1: Timesheet entry lifecycle on a to-do + +load smoke_helper + +setup_file() { + ensure_token || return 1 + ensure_todo || return 1 +} + +@test "timesheet entry create logs time against a to-do" { + run_smoke basecamp timesheet entry create "$QA_TODO" --hours 0.25 --description "Smoke entry $(date +%s)" --json + # Timesheets may not be enabled on all accounts (403 forbidden) + [[ "$status" -eq 4 ]] && mark_unverifiable "Timesheets not enabled in this account" + assert_success + assert_json_value '.ok' 'true' + assert_json_not_null '.data.id' + + echo "$output" | jq -r '.data.id' > "$BATS_FILE_TMPDIR/timesheet_entry_id" +} + +@test "timesheet entry show returns entry detail" { + local id_file="$BATS_FILE_TMPDIR/timesheet_entry_id" + [[ -f "$id_file" ]] || mark_unverifiable "No timesheet entry created in prior test" + local entry_id + entry_id=$(<"$id_file") + + run_smoke basecamp timesheet entry show "$entry_id" --json + assert_success + assert_json_value '.ok' 'true' + assert_json_value '.data.parent.id' "$QA_TODO" +} + +@test "timesheet entry update changes the hours" { + local id_file="$BATS_FILE_TMPDIR/timesheet_entry_id" + [[ -f "$id_file" ]] || mark_unverifiable "No timesheet entry created in prior test" + local entry_id + entry_id=$(<"$id_file") + + run_smoke basecamp timesheet entry update "$entry_id" --hours 0:30 --json + assert_success + assert_json_value '.ok' 'true' +} + +@test "timesheet entry delete removes the entry" { + local id_file="$BATS_FILE_TMPDIR/timesheet_entry_id" + [[ -f "$id_file" ]] || mark_unverifiable "No timesheet entry created in prior test" + local entry_id + entry_id=$(<"$id_file") + + run_smoke basecamp timesheet entry delete "$entry_id" --force --json + assert_success + assert_json_value '.data.deleted' 'true' +} diff --git a/e2e/timesheet.bats b/e2e/timesheet.bats index 87cd49417..2c3933bcb 100644 --- a/e2e/timesheet.bats +++ b/e2e/timesheet.bats @@ -90,3 +90,42 @@ load test_helper run basecamp timesheet foobar # Command may show help or require project - just verify it runs } + + +# Entry verbs: refused locally, before any request + +@test "timesheet entry create without --hours shows error" { + create_credentials + create_global_config '{"account_id": 99999}' + + run basecamp timesheet entry create 789 + assert_failure + assert_output_contains "--hours is required" +} + +@test "timesheet entry update with nothing to change shows error" { + create_credentials + create_global_config '{"account_id": 99999}' + + run basecamp timesheet entry update 123 + assert_failure + assert_output_contains "Nothing to update" +} + +@test "timesheet entry delete refuses an item URL" { + create_credentials + create_global_config '{"account_id": 99999}' + + run basecamp timesheet entry delete "https://3.basecamp.com/99999/buckets/89/todos/789" --force + assert_failure + assert_output_contains "does not name a timesheet entry" +} + +@test "timesheet entry delete needs --force without a terminal" { + create_credentials + create_global_config '{"account_id": 99999}' + + run basecamp timesheet entry delete 123 --json + assert_failure + assert_output_contains "--force" +} diff --git a/internal/commands/commands.go b/internal/commands/commands.go index 3c4bc76ff..ed1385cd5 100644 --- a/internal/commands/commands.go +++ b/internal/commands/commands.go @@ -73,7 +73,7 @@ func CommandCategories() []CommandCategory { { Name: "Scheduling & Time", Commands: []CommandInfo{ - {Name: "timesheet", Category: "scheduling", Description: "Manage time tracking", Actions: []string{"report", "project", "item"}}, + {Name: "timesheet", Category: "scheduling", Description: "Manage time tracking", Actions: []string{"report", "project", "item", "entry"}}, {Name: "timeline", Category: "scheduling", Description: "View activity timelines", Actions: []string{}}, {Name: "reports", Category: "scheduling", Description: "View reports", Actions: []string{"assignable", "assigned", "overdue", "schedule"}}, {Name: "assignments", Category: "scheduling", Description: "View my assignments", Actions: []string{"list", "completed", "due", "prioritize", "deprioritize", "reorder"}}, diff --git a/internal/commands/timesheet.go b/internal/commands/timesheet.go index 5e8ed99d3..8e5824e76 100644 --- a/internal/commands/timesheet.go +++ b/internal/commands/timesheet.go @@ -25,8 +25,9 @@ func NewTimesheetCmd() *cobra.Command { Timesheet entries track time logged against any item (todo, message, document, etc.). The account-wide report defaults to the last month if no -date range is specified.`, - Annotations: map[string]string{"agent_notes": "Time is logged against items (todos, cards, messages, etc.)\nbasecamp clock is a shortcut for timesheet entry create\nUse basecamp reports assigned --json to see assigned items, then clock time against them"}, +date range is specified. "timesheet entry" logs, shows, changes and deletes +one entry.`, + Annotations: map[string]string{"agent_notes": "Time is logged against items (todos, cards, messages, etc.)\nbasecamp timesheet entry create --hours 1.5 logs time; show, update and delete take the entry id\nUse basecamp reports assigned --json to see assigned items, then log time against them"}, RunE: func(cmd *cobra.Command, args []string) error { return runTimesheetReport(cmd, startDate, endDate, personID) }, @@ -44,6 +45,7 @@ date range is specified.`, newTimesheetReportCmd(&startDate, &endDate, &personID), newTimesheetProjectCmd(&project), newTimesheetRecordingCmd(&project), + newTimesheetEntryCmd(), ) return cmd diff --git a/internal/commands/timesheet_entries.go b/internal/commands/timesheet_entries.go new file mode 100644 index 000000000..23b12b386 --- /dev/null +++ b/internal/commands/timesheet_entries.go @@ -0,0 +1,444 @@ +package commands + +import ( + "errors" + "fmt" + "regexp" + "strconv" + "strings" + "time" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/spf13/cobra" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/dateparse" + "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/tui" + "github.com/basecamp/basecamp-cli/internal/urlarg" +) + +// newTimesheetEntryCmd creates the `timesheet entry` group: one timesheet +// entry at a time. +// +// bc3 routes timesheet entries by account alone — POST +// /recordings/:id/timesheet/entries.json to log time against a recording, and +// /timesheet_entries/:id for one entry — so no verb needs a project, and +// none reads the --project/--in the timesheet group carries for its reports. +func newTimesheetEntryCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "entry", + Short: "Log, view, change and delete timesheet entries", + Long: `Log, view, change and delete individual timesheet entries. + +An entry is time logged against one item: a to-do, message, card, document, +upload, schedule entry, or a project's own timesheet. Every verb but create +addresses the entry itself, by id or by its Basecamp URL. + + basecamp timesheet entry create 789 --hours 1.5 + basecamp timesheet entry show 1069480283 + basecamp timesheet entry update 1069480283 --hours 2:15 + basecamp timesheet entry delete 1069480283 --force`, + } + + cmd.AddCommand( + newTimesheetEntryShowCmd(), + newTimesheetEntryCreateCmd(), + newTimesheetEntryUpdateCmd(), + newTimesheetEntryDeleteCmd(), + ) + + return cmd +} + +func newTimesheetEntryShowCmd() *cobra.Command { + return &cobra.Command{ + Use: "show ", + Short: "Show a timesheet entry", + Long: `Show one timesheet entry: its date, hours, description, the person the +time is for, and the item it is logged against. + + basecamp timesheet entry show 1069480283 + basecamp timesheet entry show https://3.basecamp.com/123/projects/456/timesheet/entries/1069480283`, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + entryID, err := timesheetEntryIDArg(args[0]) + if err != nil { + return err + } + + app := appctx.FromContext(cmd.Context()) + if err := ensureAccount(cmd, app); err != nil { + return err + } + + entry, err := app.Account().Timesheet().Get(cmd.Context(), entryID) + if err != nil { + return convertSDKError(err) + } + + return app.OK(entry, + output.WithSummary(timesheetEntrySummary("Timesheet entry", entry)), + output.WithBreadcrumbs(timesheetEntryBreadcrumbs(entry)...), + ) + }, + } +} + +func newTimesheetEntryCreateCmd() *cobra.Command { + var hours, date, description, person string + + cmd := &cobra.Command{ + Use: "create ", + Short: "Log time against an item", + Long: `Log time against an item: a to-do, message, card, document, upload or +schedule entry, by its id or pasted Basecamp URL. + +To log time to a project without an item, pass the id of the project's own +timesheet: the parent id of a project-level entry in +"basecamp timesheet project". + +--hours is required, as a decimal (1.5) or hours and minutes (1:30). +--date defaults to today and takes ISO 8601 or a relative date (yesterday, +monday). --person is who the time is for, by id, name, +email or "me"; it defaults to you, and must be a non-client member of the +project. + + basecamp timesheet entry create 789 --hours 1.5 + basecamp timesheet entry create https://3.basecamp.com/123/buckets/456/todos/789 --hours 0:45 --date yesterday --description "Review"`, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + recordingID, err := timesheetRecordingIDArg(args[0]) + if err != nil { + return err + } + if strings.TrimSpace(hours) == "" { + return output.ErrUsageHint("--hours is required", `Pass the time spent, as a decimal (1.5) or hours and minutes (1:30)`) + } + if err := validateTimesheetHours(hours); err != nil { + return err + } + entryDate := time.Now().Format("2006-01-02") + if cmd.Flags().Changed("date") { + if entryDate, err = timesheetDate(date); err != nil { + return err + } + } + + app := appctx.FromContext(cmd.Context()) + if err := ensureAccount(cmd, app); err != nil { + return err + } + + req := &basecamp.CreateTimesheetEntryRequest{ + Date: entryDate, + Hours: strings.TrimSpace(hours), + Description: description, + } + if cmd.Flags().Changed("person") { + if req.PersonID, err = timesheetPersonID(cmd, app, person); err != nil { + return err + } + } + + entry, err := app.Account().Timesheet().Create(cmd.Context(), recordingID, req) + if err != nil { + return convertSDKError(err) + } + + return app.OK(entry, + output.WithSummary(timesheetEntrySummary("Logged timesheet entry", entry)), + output.WithBreadcrumbs(timesheetEntryBreadcrumbs(entry)...), + ) + }, + } + + cmd.Flags().StringVar(&hours, "hours", "", `Hours spent: decimal ("1.5") or hours and minutes ("1:30")`) + cmd.Flags().StringVarP(&date, "date", "d", "", "Date the time was spent (ISO 8601 or relative, e.g. yesterday; default today)") + cmd.Flags().StringVar(&description, "description", "", "Note about the work") + cmd.Flags().StringVar(&person, "person", "", `Person the time is for: ID, name, email or "me" (default: you)`) + + return cmd +} + +func newTimesheetEntryUpdateCmd() *cobra.Command { + var hours, date, description, person string + + cmd := &cobra.Command{ + Use: "update ", + Short: "Change a timesheet entry", + Long: `Change a timesheet entry's hours, date, description or person. Only the +flags you pass are sent; at least one is required. A description cannot be +cleared through the API, only replaced. + + basecamp timesheet entry update 1069480283 --hours 2.5 + basecamp timesheet entry update 1069480283 --date 2026-10-26 --description "Client call"`, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + entryID, err := timesheetEntryIDArg(args[0]) + if err != nil { + return err + } + + req := &basecamp.UpdateTimesheetEntryRequest{} + changed := false + if cmd.Flags().Changed("hours") { + if err := validateTimesheetHours(hours); err != nil { + return err + } + req.Hours = strings.TrimSpace(hours) + changed = true + } + if cmd.Flags().Changed("date") { + if req.Date, err = timesheetDate(date); err != nil { + return err + } + changed = true + } + if cmd.Flags().Changed("description") { + if description == "" { + return output.ErrUsageHint("--description cannot be empty", + "The API cannot clear a timesheet entry's description; pass the new text") + } + req.Description = description + changed = true + } + if !changed && !cmd.Flags().Changed("person") { + return output.ErrUsageHint("Nothing to update", + "Pass at least one of --hours, --date, --description or --person") + } + + app := appctx.FromContext(cmd.Context()) + if err := ensureAccount(cmd, app); err != nil { + return err + } + + if cmd.Flags().Changed("person") { + if req.PersonID, err = timesheetPersonID(cmd, app, person); err != nil { + return err + } + } + + entry, err := app.Account().Timesheet().Update(cmd.Context(), entryID, req) + if err != nil { + return convertSDKError(err) + } + + return app.OK(entry, + output.WithSummary(timesheetEntrySummary("Updated timesheet entry", entry)), + output.WithBreadcrumbs(timesheetEntryBreadcrumbs(entry)...), + ) + }, + } + + cmd.Flags().StringVar(&hours, "hours", "", `Hours spent: decimal ("1.5") or hours and minutes ("1:30")`) + cmd.Flags().StringVarP(&date, "date", "d", "", "Date the time was spent (ISO 8601 or relative, e.g. yesterday)") + cmd.Flags().StringVar(&description, "description", "", "Note about the work") + cmd.Flags().StringVar(&person, "person", "", `Person the time is for: ID, name, email or "me"`) + + return cmd +} + +func newTimesheetEntryDeleteCmd() *cobra.Command { + var force bool + + cmd := &cobra.Command{ + Use: "delete ", + Short: "Delete a timesheet entry", + Long: `Permanently delete a timesheet entry. It never goes to the trash and +cannot be restored, so this asks for confirmation; machine-output modes and +non-interactive sessions cannot answer one and need --force instead. It is the +only way to remove an entry: Basecamp refuses to trash or archive one. People +who may not remove the entry are refused. + + basecamp timesheet entry delete 1069480283 + basecamp timesheet entry delete 1069480283 --json --force`, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + entryID, err := timesheetEntryIDArg(args[0]) + if err != nil { + return err + } + + // Permanent, so only with --force or a confirmation somebody can + // answer — refused before any account lookup or request. + if err := ensureDeleteConfirmable(cmd, force); err != nil { + return err + } + + app := appctx.FromContext(cmd.Context()) + if err := ensureAccount(cmd, app); err != nil { + return err + } + + if !force { + confirmed, err := tui.ConfirmDangerous("Permanently delete this timesheet entry?") + switch { + case errors.Is(err, tui.ErrCanceled): + return nil + case err != nil: + return fmt.Errorf("confirming the delete: %w", err) + case !confirmed: + return nil + } + } + + if err := app.Account().Timesheet().Destroy(cmd.Context(), entryID); err != nil { + return convertSDKError(err) + } + + return app.OK(map[string]any{"id": entryID, "deleted": true}, + output.WithSummary(fmt.Sprintf("Deleted timesheet entry #%d", entryID)), + output.WithBreadcrumbs(output.Breadcrumb{ + Action: "report", + Cmd: "basecamp timesheet report", + Description: "View timesheet report", + }), + ) + }, + } + + cmd.Flags().BoolVarP(&force, "force", "f", false, "Skip confirmation prompt") + + return cmd +} + +// timesheetEntryURL matches an entry's own URL: its app URL +// (projects/:p/timesheet/entries/:id) and its API URL in either form. +var timesheetEntryURL = regexp.MustCompile(`^https?://[^/]+/\d+/(?:(?:projects|buckets)/\d+/timesheet/entries|timesheet_entries)/(\d+)(?:\.json)?/?(?:[?#].*)?$`) + +// timesheetEntryIDArg resolves the positional the per-entry verbs +// take: a bare id or the entry's own URL. Any other URL is refused rather than +// read as an entry id — a to-do's URL names the item the time is logged +// against, and `timesheet entry delete` must never aim at one of those. +func timesheetEntryIDArg(arg string) (int64, error) { + raw := strings.TrimSpace(arg) + if strings.Contains(raw, "://") { + m := timesheetEntryURL.FindStringSubmatch(raw) + if m == nil { + return 0, output.ErrUsageHint( + fmt.Sprintf("%q does not name a timesheet entry", arg), + "Pass the entry id, or its URL (…/timesheet/entries/); "+ + "basecamp timesheet item lists the entries on an item") + } + raw = m[1] + } + id, err := strconv.ParseInt(raw, 10, 64) + if err != nil || id <= 0 { + return 0, output.ErrUsageHint( + fmt.Sprintf("%q is not a timesheet entry id or URL", arg), + "Pass the numeric entry id, or paste its Basecamp URL") + } + return id, nil +} + +// timesheetRecordingIDArg resolves create's : the item the +// time is logged against. A project's timesheet URL is refused: it carries the +// project's id, not the id of the project's timesheet recording. +func timesheetRecordingIDArg(arg string) (int64, error) { + if parsed := urlarg.Parse(arg); parsed != nil && (parsed.Type == "timesheet" || parsed.IsCollection) { + return 0, output.ErrUsageHint( + fmt.Sprintf("%q does not name one item", arg), + "Pass the id or URL of the to-do, message, card or other item to log time against. "+ + "For a project-level entry, pass the project timesheet's id: the parent id of a "+ + "project-level entry in basecamp timesheet project") + } + idStr, err := extractID(arg) + if err != nil { + return 0, err + } + id, err := strconv.ParseInt(idStr, 10, 64) + if err != nil || id <= 0 { + return 0, output.ErrUsageHint( + fmt.Sprintf("%q is not an item id or Basecamp URL", arg), + "Pass the numeric id of the item to log time against, or paste its Basecamp URL") + } + return id, nil +} + +// timesheetHours is what bc3 accepts for hours: a decimal, or hours and +// minutes. +var timesheetHours = regexp.MustCompile(`^(?:\d+(?:\.\d+)?|\.\d+|\d+:[0-5]\d)$`) + +func validateTimesheetHours(hours string) error { + h := strings.TrimSpace(hours) + if !timesheetHours.MatchString(h) { + return output.ErrUsageHint(fmt.Sprintf("Invalid hours %q", hours), + `Pass hours as a decimal ("1.5") or hours and minutes ("1:30")`) + } + return nil +} + +// timesheetISODate is the only form bc3 takes for an entry's date. +var timesheetISODate = regexp.MustCompile(`^\d{4}-\d{2}-\d{2}$`) + +// timesheetDate turns --date into the ISO 8601 date bc3 takes. dateparse +// returns what it doesn't recognize unchanged, so anything that still isn't a +// real date is refused here rather than sent. +func timesheetDate(input string) (string, error) { + parsed := dateparse.Parse(input) + if timesheetISODate.MatchString(parsed) { + if _, err := time.Parse("2006-01-02", parsed); err == nil { + return parsed, nil + } + } + return "", output.ErrUsageHint(fmt.Sprintf("Invalid date %q", input), + "Pass an ISO 8601 date (2026-10-27) or a relative one (today, yesterday, monday)") +} + +// timesheetPersonID resolves --person: a numeric id is sent as given, and a +// name, email or "me" is looked up. +func timesheetPersonID(cmd *cobra.Command, app *appctx.App, person string) (int64, error) { + p := strings.TrimSpace(person) + if p == "" { + return 0, output.ErrUsageHint("--person cannot be empty", `Pass a person's ID, name, email or "me"`) + } + if id, err := strconv.ParseInt(p, 10, 64); err == nil { + if id <= 0 { + return 0, output.ErrUsage("Invalid person ID") + } + return id, nil + } + resolved, _, err := app.Names.ResolvePerson(cmd.Context(), p) + if err != nil { + return 0, err + } + id, err := strconv.ParseInt(resolved, 10, 64) + if err != nil { + return 0, output.ErrUsage("Invalid person ID") + } + return id, nil +} + +func timesheetEntrySummary(prefix string, entry *basecamp.TimesheetEntry) string { + summary := fmt.Sprintf("%s #%d: %sh on %s", prefix, entry.ID, entry.Hours, entry.Date) + if entry.Parent != nil && entry.Parent.Title != "" { + summary += " for " + entry.Parent.Title + } + return summary +} + +// timesheetEntryBreadcrumbs are the follow-ups for one entry: change or delete +// it, and see the rest of the time on its item. +func timesheetEntryBreadcrumbs(entry *basecamp.TimesheetEntry) []output.Breadcrumb { + crumbs := []output.Breadcrumb{ + { + Action: "update", + Cmd: fmt.Sprintf("basecamp timesheet entry update %d --hours ", entry.ID), + Description: "Change entry", + }, + { + Action: "delete", + Cmd: fmt.Sprintf("basecamp timesheet entry delete %d", entry.ID), + Description: "Delete entry", + }, + } + if entry.Parent != nil && entry.Parent.ID != 0 { + crumbs = append(crumbs, output.Breadcrumb{ + Action: "item", + Cmd: fmt.Sprintf("basecamp timesheet item %d", entry.Parent.ID), + Description: "View the item's timesheet", + }) + } + return crumbs +} diff --git a/internal/commands/timesheet_entries_test.go b/internal/commands/timesheet_entries_test.go new file mode 100644 index 000000000..22f99fc2b --- /dev/null +++ b/internal/commands/timesheet_entries_test.go @@ -0,0 +1,238 @@ +package commands + +import ( + "encoding/json" + "fmt" + "net/http" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const timesheetEntryFixture = `{ + "id": 1069480283, + "status": "active", + "type": "Timesheet::Entry", + "title": "Timesheet entry", + "date": "2026-10-26", + "hours": "1.5", + "description": "Review", + "created_at": "2026-10-26T10:00:00Z", + "updated_at": "2026-10-26T10:00:00Z", + "parent": {"id": 789, "title": "Plan the offsite", "type": "Todo"}, + "bucket": {"id": 89, "name": "Offsite", "type": "Project"}, + "creator": {"id": 7, "name": "Victor Cooper"}, + "person": {"id": 7, "name": "Victor Cooper"} +}` + +func timesheetEntryPath(id int64) string { + return fmt.Sprintf("/99999/timesheet_entries/%d", id) +} + +func timesheetEntriesPath(recordingID int64) string { + return fmt.Sprintf("/99999/recordings/%d/timesheet/entries.json", recordingID) +} + +func runTimesheet(t *testing.T, routes []stubRoute, args ...string) (*recordingTransport, string, json.RawMessage, error) { + t.Helper() + app, transport, out := setupPersonalFeedApp(t, routes...) + err := executeRecordingCommand(NewTimesheetCmd(), app, args...) + if err != nil { + return transport, "", nil, err + } + data, summary := subtaskEnvelope(t, out) + return transport, summary, data, nil +} + +func TestTimesheetEntryShowGetsTheEntry(t *testing.T) { + transport, summary, data, err := runTimesheet(t, + []stubRoute{{method: http.MethodGet, path: timesheetEntryPath(1069480283), status: http.StatusOK, body: timesheetEntryFixture}}, + "entry", "show", "1069480283") + require.NoError(t, err) + + call := transport.last(t) + assert.Equal(t, http.MethodGet, call.Method) + assert.Equal(t, timesheetEntryPath(1069480283), call.Path) + assert.Equal(t, "Timesheet entry #1069480283: 1.5h on 2026-10-26 for Plan the offsite", summary) + + var entry map[string]any + require.NoError(t, json.Unmarshal(data, &entry)) + assert.Equal(t, "1.5", entry["hours"]) +} + +// An entry's own URL names it, in its app form and both API forms. +func TestTimesheetEntryShowAcceptsTheEntryURL(t *testing.T) { + for _, url := range []string{ + "https://3.basecamp.com/99999/projects/89/timesheet/entries/1069480283", + "https://3.basecampapi.com/99999/projects/89/timesheet/entries/1069480283.json", + "https://3.basecampapi.com/99999/timesheet_entries/1069480283.json", + } { + transport, _, _, err := runTimesheet(t, + []stubRoute{{method: http.MethodGet, path: timesheetEntryPath(1069480283), status: http.StatusOK, body: timesheetEntryFixture}}, + "entry", "show", url) + require.NoError(t, err, url) + assert.Equal(t, timesheetEntryPath(1069480283), transport.last(t).Path, url) + } +} + +func TestTimesheetEntryCreatePostsDateHoursDescriptionAndPerson(t *testing.T) { + transport, summary, _, err := runTimesheet(t, + []stubRoute{{method: http.MethodPost, path: timesheetEntriesPath(789), status: http.StatusCreated, body: timesheetEntryFixture}}, + "entry", "create", "789", "--hours", "1:30", "--date", "2026-10-26", "--description", "Review", "--person", "7") + require.NoError(t, err) + + call := transport.last(t) + assert.Equal(t, http.MethodPost, call.Method) + assert.Equal(t, timesheetEntriesPath(789), call.Path) + assert.Equal(t, map[string]any{ + "date": "2026-10-26", "hours": "1:30", "description": "Review", "person_id": float64(7), + }, decodeSubtaskBody(t, call)) + assert.Equal(t, "Logged timesheet entry #1069480283: 1.5h on 2026-10-26 for Plan the offsite", summary) +} + +// --date defaults to today, and unset optional fields are not sent. +func TestTimesheetEntryCreateDefaultsToTodayAndOmitsUnset(t *testing.T) { + transport, _, _, err := runTimesheet(t, + []stubRoute{{method: http.MethodPost, path: timesheetEntriesPath(789), status: http.StatusCreated, body: timesheetEntryFixture}}, + "entry", "create", "https://3.basecamp.com/99999/buckets/89/todos/789", "--hours", "2") + require.NoError(t, err) + + assert.Equal(t, map[string]any{ + "date": time.Now().Format("2006-01-02"), "hours": "2", + }, decodeSubtaskBody(t, transport.last(t))) +} + +// "me" is resolved to the caller's person id before the entry is created. +func TestTimesheetEntryCreateResolvesPersonMe(t *testing.T) { + transport, _, _, err := runTimesheet(t, + []stubRoute{ + {method: http.MethodGet, path: "/99999/my/profile.json", status: http.StatusOK, body: `{"id": 42, "name": "Me"}`}, + {method: http.MethodPost, path: timesheetEntriesPath(789), status: http.StatusCreated, body: timesheetEntryFixture}, + }, + "entry", "create", "789", "--hours", "1", "--person", "me") + require.NoError(t, err) + + assert.Equal(t, float64(42), decodeSubtaskBody(t, transport.last(t))["person_id"]) +} + +func TestTimesheetEntryCreateRefusesBadInput(t *testing.T) { + for _, args := range [][]string{ + {"entry", "create", "789"}, + {"entry", "create", "789", "--hours", ""}, + {"entry", "create", "789", "--hours", "an hour"}, + {"entry", "create", "789", "--hours", "1:75"}, + {"entry", "create", "789", "--hours", "1", "--date", "someday"}, + {"entry", "create", "789", "--hours", "1", "--date", "2026-13-40"}, + {"entry", "create", "789", "--hours", "1", "--person", ""}, + {"entry", "create", "not-an-id", "--hours", "1"}, + // A project's timesheet URL carries the project id, not the + // timesheet recording's. + {"entry", "create", "https://3.basecamp.com/99999/projects/89/timesheet", "--hours", "1"}, + } { + transport, _, _, err := runTimesheet(t, nil, args...) + requireBookmarksUsageError(t, err) + assert.Empty(t, transport.recorded(), "no request for %v", args) + } +} + +func TestTimesheetEntryUpdateSendsOnlyWhatChanged(t *testing.T) { + transport, summary, _, err := runTimesheet(t, + []stubRoute{{method: http.MethodPut, path: timesheetEntryPath(1069480283), status: http.StatusOK, body: timesheetEntryFixture}}, + "entry", "update", "1069480283", "--hours", "2.5") + require.NoError(t, err) + + call := transport.last(t) + assert.Equal(t, http.MethodPut, call.Method) + assert.Equal(t, timesheetEntryPath(1069480283), call.Path) + assert.Equal(t, map[string]any{"hours": "2.5"}, decodeSubtaskBody(t, call)) + assert.Equal(t, "Updated timesheet entry #1069480283: 1.5h on 2026-10-26 for Plan the offsite", summary) +} + +func TestTimesheetEntryUpdateSendsEveryField(t *testing.T) { + transport, _, _, err := runTimesheet(t, + []stubRoute{{method: http.MethodPut, path: timesheetEntryPath(1069480283), status: http.StatusOK, body: timesheetEntryFixture}}, + "entry", "update", "1069480283", "--hours", "3", "--date", "2026-10-25", "--description", "Call", "--person", "8") + require.NoError(t, err) + + assert.Equal(t, map[string]any{ + "date": "2026-10-25", "hours": "3", "description": "Call", "person_id": float64(8), + }, decodeSubtaskBody(t, transport.last(t))) +} + +func TestTimesheetEntryUpdateRefusesBadInput(t *testing.T) { + for _, args := range [][]string{ + {"entry", "update", "1069480283"}, + {"entry", "update", "1069480283", "--hours", "x"}, + {"entry", "update", "1069480283", "--date", "someday"}, + {"entry", "update", "1069480283", "--description", ""}, + {"entry", "update", "1069480283", "--person", ""}, + {"entry", "update", "not-an-id", "--hours", "1"}, + } { + transport, _, _, err := runTimesheet(t, nil, args...) + requireBookmarksUsageError(t, err) + assert.Empty(t, transport.recorded(), "no request for %v", args) + } +} + +func TestTimesheetEntryDeleteDestroysTheEntry(t *testing.T) { + transport, summary, data, err := runTimesheet(t, + []stubRoute{{method: http.MethodDelete, path: timesheetEntryPath(1069480283), status: http.StatusNoContent}}, + "entry", "delete", "1069480283", "--force") + require.NoError(t, err) + + call := transport.last(t) + assert.Equal(t, http.MethodDelete, call.Method) + assert.Equal(t, timesheetEntryPath(1069480283), call.Path) + assert.Equal(t, "Deleted timesheet entry #1069480283", summary) + assert.JSONEq(t, `{"id": 1069480283, "deleted": true}`, string(data)) +} + +// A permanent delete in machine-output mode has nobody to confirm it, so it +// needs --force and is refused before any request without it. +func TestTimesheetEntryDeleteNeedsForceWhenNothingCanConfirm(t *testing.T) { + transport, _, _, err := runTimesheet(t, nil, "entry", "delete", "1069480283") + outErr := requireBookmarksUsageError(t, err) + assert.Contains(t, outErr.Message, "--force") + assert.Empty(t, transport.recorded()) +} + +// A URL that is not an entry's own names something else — most often the item +// the time is logged against — and must never be read as an entry id, least +// of all by delete. +func TestTimesheetEntryVerbsRefuseOtherURLs(t *testing.T) { + for _, url := range []string{ + "https://3.basecamp.com/99999/buckets/89/todos/789", + "https://3.basecamp.com/99999/projects/89/timesheet", + "https://example.com/not/basecamp", + } { + for _, verb := range [][]string{{"show"}, {"update", "--hours", "1"}, {"delete", "--force"}} { + args := append([]string{"entry", verb[0], url}, verb[1:]...) + transport, _, _, err := runTimesheet(t, nil, args...) + requireBookmarksUsageError(t, err) + assert.Empty(t, transport.recorded(), "no request for %v", args) + } + } +} + +func TestTimesheetEntryVerbsRejectANonID(t *testing.T) { + for _, args := range [][]string{ + {"entry", "show", "not-an-id"}, + {"entry", "show", "0"}, + {"entry", "delete", "12abc", "--force"}, + } { + transport, _, _, err := runTimesheet(t, nil, args...) + requireBookmarksUsageError(t, err) + assert.Empty(t, transport.recorded(), "no request for %v", args) + } +} + +// A refusal from Basecamp (403 when the caller may not remove the entry) is +// returned as an error, not reported as a delete. +func TestTimesheetEntryDeleteReportsARefusal(t *testing.T) { + _, _, _, err := runTimesheet(t, + []stubRoute{{method: http.MethodDelete, path: timesheetEntryPath(1069480283), status: http.StatusForbidden, body: `{"error":"forbidden"}`}}, + "entry", "delete", "1069480283", "--force") + require.Error(t, err) +} diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index b5503556a..a8eb61b90 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -30,6 +30,8 @@ triggers: - basecamp checkin - basecamp check-in - basecamp timeline + - basecamp timesheet + - log time - basecamp template - basecamp webhook - basecamp gauge @@ -284,6 +286,26 @@ basecamp projects logo remove --in "" --json requires authentication and redirects to the image; it is not a public image link. +## Timesheet entries + +Time is logged against one item (to-do, message, card, document, upload, +schedule entry). `timesheet report`, `timesheet project` and `timesheet item` +read entries; `timesheet entry` writes them, addressed by account alone with +no `--in`: + +```bash +basecamp timesheet entry create --hours 1.5 --date yesterday --json +basecamp timesheet entry show --json +basecamp timesheet entry update --hours 2:15 --json +basecamp timesheet entry delete --force --json +``` + +- `--hours` is a decimal or `h:mm`; `--date` defaults to today on create; + `--person` (ID, name, email or `me`) logs time for someone else. +- `delete` is permanent: entries are never trashed. Machine output needs + `--force`. +- Accounts without timesheets enabled answer 403. + ## Content and stdin Command help declares content arguments and stdin support. Titles are generally From 8c2ad45b9a763afb5cb75d4a79995e7438268103 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 28 Oct 2026 00:11:28 -0500 Subject: [PATCH 2/6] Hold a pasted timesheet URL to its host and account A URL on a look-alike host, or from another account than the configured one, is refused before any request, and create refuses project and listing URLs, which name no item. --- internal/commands/timesheet_entries.go | 101 ++++++++++++++------ internal/commands/timesheet_entries_test.go | 28 ++++++ 2 files changed, 99 insertions(+), 30 deletions(-) diff --git a/internal/commands/timesheet_entries.go b/internal/commands/timesheet_entries.go index 23b12b386..f631c9789 100644 --- a/internal/commands/timesheet_entries.go +++ b/internal/commands/timesheet_entries.go @@ -13,6 +13,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/dateparse" + "github.com/basecamp/basecamp-cli/internal/hostutil" "github.com/basecamp/basecamp-cli/internal/output" "github.com/basecamp/basecamp-cli/internal/tui" "github.com/basecamp/basecamp-cli/internal/urlarg" @@ -62,12 +63,15 @@ time is for, and the item it is logged against. basecamp timesheet entry show https://3.basecamp.com/123/projects/456/timesheet/entries/1069480283`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - entryID, err := timesheetEntryIDArg(args[0]) + entryID, urlAccountID, err := timesheetEntryIDArg(args[0]) if err != nil { return err } app := appctx.FromContext(cmd.Context()) + if err := timesheetURLAccount(app, args[0], urlAccountID); err != nil { + return err + } if err := ensureAccount(cmd, app); err != nil { return err } @@ -108,7 +112,7 @@ project. basecamp timesheet entry create https://3.basecamp.com/123/buckets/456/todos/789 --hours 0:45 --date yesterday --description "Review"`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - recordingID, err := timesheetRecordingIDArg(args[0]) + recordingID, urlAccountID, err := timesheetRecordingIDArg(args[0]) if err != nil { return err } @@ -126,6 +130,9 @@ project. } app := appctx.FromContext(cmd.Context()) + if err := timesheetURLAccount(app, args[0], urlAccountID); err != nil { + return err + } if err := ensureAccount(cmd, app); err != nil { return err } @@ -175,7 +182,7 @@ cleared through the API, only replaced. basecamp timesheet entry update 1069480283 --date 2026-10-26 --description "Client call"`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - entryID, err := timesheetEntryIDArg(args[0]) + entryID, urlAccountID, err := timesheetEntryIDArg(args[0]) if err != nil { return err } @@ -209,6 +216,9 @@ cleared through the API, only replaced. } app := appctx.FromContext(cmd.Context()) + if err := timesheetURLAccount(app, args[0], urlAccountID); err != nil { + return err + } if err := ensureAccount(cmd, app); err != nil { return err } @@ -255,7 +265,7 @@ who may not remove the entry are refused. basecamp timesheet entry delete 1069480283 --json --force`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - entryID, err := timesheetEntryIDArg(args[0]) + entryID, urlAccountID, err := timesheetEntryIDArg(args[0]) if err != nil { return err } @@ -267,6 +277,9 @@ who may not remove the entry are refused. } app := appctx.FromContext(cmd.Context()) + if err := timesheetURLAccount(app, args[0], urlAccountID); err != nil { + return err + } if err := ensureAccount(cmd, app); err != nil { return err } @@ -304,56 +317,84 @@ who may not remove the entry are refused. } // timesheetEntryURL matches an entry's own URL: its app URL -// (projects/:p/timesheet/entries/:id) and its API URL in either form. -var timesheetEntryURL = regexp.MustCompile(`^https?://[^/]+/\d+/(?:(?:projects|buckets)/\d+/timesheet/entries|timesheet_entries)/(\d+)(?:\.json)?/?(?:[?#].*)?$`) +// (projects/:p/timesheet/entries/:id) and its API URL in either form. The +// first group is the account, the second the entry. +var timesheetEntryURL = regexp.MustCompile(`^https?://[^/]+/(\d+)/(?:(?:projects|buckets)/\d+/timesheet/entries|timesheet_entries)/(\d+)(?:\.json)?/?(?:[?#].*)?$`) // timesheetEntryIDArg resolves the positional the per-entry verbs -// take: a bare id or the entry's own URL. Any other URL is refused rather than -// read as an entry id — a to-do's URL names the item the time is logged -// against, and `timesheet entry delete` must never aim at one of those. -func timesheetEntryIDArg(arg string) (int64, error) { +// take: a bare id or the entry's own URL, returning the URL's account too. Any +// other URL is refused rather than read as an entry id — a to-do's URL names +// the item the time is logged against, and `timesheet entry delete` must never +// aim at one of those. +func timesheetEntryIDArg(arg string) (id int64, urlAccountID string, err error) { raw := strings.TrimSpace(arg) if strings.Contains(raw, "://") { m := timesheetEntryURL.FindStringSubmatch(raw) if m == nil { - return 0, output.ErrUsageHint( + return 0, "", output.ErrUsageHint( fmt.Sprintf("%q does not name a timesheet entry", arg), "Pass the entry id, or its URL (…/timesheet/entries/); "+ "basecamp timesheet item lists the entries on an item") } - raw = m[1] + urlAccountID, raw = m[1], m[2] } - id, err := strconv.ParseInt(raw, 10, 64) - if err != nil || id <= 0 { - return 0, output.ErrUsageHint( + id, perr := strconv.ParseInt(raw, 10, 64) + if perr != nil || id <= 0 { + return 0, "", output.ErrUsageHint( fmt.Sprintf("%q is not a timesheet entry id or URL", arg), "Pass the numeric entry id, or paste its Basecamp URL") } - return id, nil + return id, urlAccountID, nil } // timesheetRecordingIDArg resolves create's : the item the -// time is logged against. A project's timesheet URL is refused: it carries the -// project's id, not the id of the project's timesheet recording. -func timesheetRecordingIDArg(arg string) (int64, error) { - if parsed := urlarg.Parse(arg); parsed != nil && (parsed.Type == "timesheet" || parsed.IsCollection) { - return 0, output.ErrUsageHint( - fmt.Sprintf("%q does not name one item", arg), - "Pass the id or URL of the to-do, message, card or other item to log time against. "+ - "For a project-level entry, pass the project timesheet's id: the parent id of a "+ - "project-level entry in basecamp timesheet project") +// time is logged against, and the URL's account. A URL that names no single +// recording is refused — a project's or a project timesheet's URL carries the +// project's id, which extractID would otherwise fall back to. +func timesheetRecordingIDArg(arg string) (id int64, urlAccountID string, err error) { + if parsed := urlarg.Parse(arg); parsed != nil { + if parsed.RecordingID == "" || parsed.IsCollection || parsed.Type == "timesheet" { + return 0, "", output.ErrUsageHint( + fmt.Sprintf("%q does not name one item", arg), + "Pass the id or URL of the to-do, message, card or other item to log time against. "+ + "For a project-level entry, pass the project timesheet's id: the parent id of a "+ + "project-level entry in basecamp timesheet project") + } + urlAccountID = parsed.AccountID } idStr, err := extractID(arg) if err != nil { - return 0, err + return 0, "", err } - id, err := strconv.ParseInt(idStr, 10, 64) - if err != nil || id <= 0 { - return 0, output.ErrUsageHint( + id, perr := strconv.ParseInt(idStr, 10, 64) + if perr != nil || id <= 0 { + return 0, "", output.ErrUsageHint( fmt.Sprintf("%q is not an item id or Basecamp URL", arg), "Pass the numeric id of the item to log time against, or paste its Basecamp URL") } - return id, nil + return id, urlAccountID, nil +} + +// timesheetURLAccount holds a pasted URL to the account it names, before any +// request. The URL router is host-agnostic, so a URL must be on a trusted +// Basecamp host; its account is adopted when none is configured and must +// match the configured one otherwise, rather than acting on a same-numbered +// entry or item in another account. Bare ids skip both checks. +func timesheetURLAccount(app *appctx.App, arg, urlAccountID string) error { + if !strings.Contains(arg, "://") { + return nil + } + if !hostutil.IsTrustedBasecampHost(strings.TrimSpace(arg), app.Config.BaseURL) { + return output.ErrUsage("refusing untrusted host in URL — expected a Basecamp URL") + } + switch { + case urlAccountID == "": + case app.Config.AccountID == "": + app.Config.AccountID = urlAccountID + case urlAccountID != app.Config.AccountID: + return output.ErrUsage(fmt.Sprintf("URL account %s does not match the configured account %s", urlAccountID, app.Config.AccountID)) + } + return nil } // timesheetHours is what bc3 accepts for hours: a decimal, or hours and diff --git a/internal/commands/timesheet_entries_test.go b/internal/commands/timesheet_entries_test.go index 22f99fc2b..dea0a2c4b 100644 --- a/internal/commands/timesheet_entries_test.go +++ b/internal/commands/timesheet_entries_test.go @@ -130,6 +130,9 @@ func TestTimesheetEntryCreateRefusesBadInput(t *testing.T) { // A project's timesheet URL carries the project id, not the // timesheet recording's. {"entry", "create", "https://3.basecamp.com/99999/projects/89/timesheet", "--hours", "1"}, + // Neither does a project's URL, or a listing's. + {"entry", "create", "https://3.basecamp.com/99999/projects/89", "--hours", "1"}, + {"entry", "create", "https://3.basecamp.com/99999/buckets/89/todolists", "--hours", "1"}, } { transport, _, _, err := runTimesheet(t, nil, args...) requireBookmarksUsageError(t, err) @@ -236,3 +239,28 @@ func TestTimesheetEntryDeleteReportsARefusal(t *testing.T) { "entry", "delete", "1069480283", "--force") require.Error(t, err) } + +// A pasted URL acts only on the account it names, on a Basecamp host: a +// look-alike host, or a URL from another account than the configured one, is +// refused before any request, on every verb. +func TestTimesheetEntryVerbsHoldAURLToItsHostAndAccount(t *testing.T) { + for _, tc := range []struct { + name string + args []string + }{ + {"show untrusted host", []string{"entry", "show", "https://evil.example/99999/timesheet_entries/1069480283.json"}}, + {"update untrusted host", []string{"entry", "update", "https://evil.example/99999/projects/89/timesheet/entries/1069480283", "--hours", "1"}}, + {"delete untrusted host", []string{"entry", "delete", "https://evil.example/99999/timesheet_entries/1069480283", "--force"}}, + {"create untrusted host", []string{"entry", "create", "https://evil.example/99999/buckets/89/todos/789", "--hours", "1"}}, + {"show other account", []string{"entry", "show", "https://3.basecamp.com/11111/projects/89/timesheet/entries/1069480283"}}, + {"update other account", []string{"entry", "update", "https://3.basecampapi.com/11111/timesheet_entries/1069480283.json", "--hours", "1"}}, + {"delete other account", []string{"entry", "delete", "https://3.basecamp.com/11111/projects/89/timesheet/entries/1069480283", "--force"}}, + {"create other account", []string{"entry", "create", "https://3.basecamp.com/11111/buckets/89/todos/789", "--hours", "1"}}, + } { + t.Run(tc.name, func(t *testing.T) { + transport, _, _, err := runTimesheet(t, nil, tc.args...) + requireBookmarksUsageError(t, err) + assert.Empty(t, transport.recorded()) + }) + } +} From 26bc68b2e90ab747507a5c2b72fd63108d58fce6 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 28 Oct 2026 00:16:06 -0500 Subject: [PATCH 3/6] Give machine readers a runnable timesheet delete, and a serial smoke lifecycle The delete breadcrumb carries --force when machine output is reading it, as chat delete's does. The smoke lifecycle runs its tests in order, and its timesheets-off gaps are allowlisted like the read-only ones. --- e2e/smoke/.qa-allowlist | 4 ++++ e2e/smoke/smoke_timesheet_write.bats | 5 ++++- internal/commands/timesheet_entries.go | 23 +++++++++++++++----- internal/commands/timesheet_entries_test.go | 24 +++++++++++++++++++++ 4 files changed, 50 insertions(+), 6 deletions(-) diff --git a/e2e/smoke/.qa-allowlist b/e2e/smoke/.qa-allowlist index 952ab0c12..6fba7c5a0 100644 --- a/e2e/smoke/.qa-allowlist +++ b/e2e/smoke/.qa-allowlist @@ -19,6 +19,10 @@ test_reports_schedule_returns_schedule_entries # 400 on some environments — sm test_timesheet_report_returns_timesheet_data # timesheets not enabled — smoke test_timesheet_project_returns_project_timesheet # timesheets not enabled — smoke test_timesheet_item_returns_timesheet_item_detail # timesheets not enabled — smoke +test_timesheet_entry_create_logs_time_against_a_todo # timesheets not enabled — smoke +test_timesheet_entry_show_returns_entry_detail # depends on timesheet entry create — smoke +test_timesheet_entry_update_changes_the_hours # depends on timesheet entry create — smoke +test_timesheet_entry_delete_removes_the_entry # depends on timesheet entry create — smoke # Lineup API may not exist on all environments test_lineup_create_creates_a_lineup_marker # API not available on some servers — smoke diff --git a/e2e/smoke/smoke_timesheet_write.bats b/e2e/smoke/smoke_timesheet_write.bats index b1e246f97..44f15d503 100644 --- a/e2e/smoke/smoke_timesheet_write.bats +++ b/e2e/smoke/smoke_timesheet_write.bats @@ -4,11 +4,14 @@ load smoke_helper setup_file() { + # Each test depends on the one before it: create writes the entry id the + # rest read, and delete must come last. + export BATS_NO_PARALLELIZE_WITHIN_FILE=true ensure_token || return 1 ensure_todo || return 1 } -@test "timesheet entry create logs time against a to-do" { +@test "timesheet entry create logs time against a todo" { run_smoke basecamp timesheet entry create "$QA_TODO" --hours 0.25 --description "Smoke entry $(date +%s)" --json # Timesheets may not be enabled on all accounts (403 forbidden) [[ "$status" -eq 4 ]] && mark_unverifiable "Timesheets not enabled in this account" diff --git a/internal/commands/timesheet_entries.go b/internal/commands/timesheet_entries.go index f631c9789..7fbdb2592 100644 --- a/internal/commands/timesheet_entries.go +++ b/internal/commands/timesheet_entries.go @@ -83,7 +83,7 @@ time is for, and the item it is logged against. return app.OK(entry, output.WithSummary(timesheetEntrySummary("Timesheet entry", entry)), - output.WithBreadcrumbs(timesheetEntryBreadcrumbs(entry)...), + output.WithBreadcrumbs(timesheetEntryBreadcrumbs(cmd, entry)...), ) }, } @@ -155,7 +155,7 @@ project. return app.OK(entry, output.WithSummary(timesheetEntrySummary("Logged timesheet entry", entry)), - output.WithBreadcrumbs(timesheetEntryBreadcrumbs(entry)...), + output.WithBreadcrumbs(timesheetEntryBreadcrumbs(cmd, entry)...), ) }, } @@ -236,7 +236,7 @@ cleared through the API, only replaced. return app.OK(entry, output.WithSummary(timesheetEntrySummary("Updated timesheet entry", entry)), - output.WithBreadcrumbs(timesheetEntryBreadcrumbs(entry)...), + output.WithBreadcrumbs(timesheetEntryBreadcrumbs(cmd, entry)...), ) }, } @@ -461,7 +461,7 @@ func timesheetEntrySummary(prefix string, entry *basecamp.TimesheetEntry) string // timesheetEntryBreadcrumbs are the follow-ups for one entry: change or delete // it, and see the rest of the time on its item. -func timesheetEntryBreadcrumbs(entry *basecamp.TimesheetEntry) []output.Breadcrumb { +func timesheetEntryBreadcrumbs(cmd *cobra.Command, entry *basecamp.TimesheetEntry) []output.Breadcrumb { crumbs := []output.Breadcrumb{ { Action: "update", @@ -470,7 +470,7 @@ func timesheetEntryBreadcrumbs(entry *basecamp.TimesheetEntry) []output.Breadcru }, { Action: "delete", - Cmd: fmt.Sprintf("basecamp timesheet entry delete %d", entry.ID), + Cmd: timesheetEntryDeleteCmd(cmd, entry.ID), Description: "Delete entry", }, } @@ -483,3 +483,16 @@ func timesheetEntryBreadcrumbs(entry *basecamp.TimesheetEntry) []output.Breadcru } return crumbs } + +// timesheetEntryDeleteCmd builds the delete breadcrumb for whoever will read +// it, as deleteLineCmd does for chat lines: machine output cannot answer the +// confirmation, so it gets --force; a person gets the bare command and is +// asked, with any flag holding this invocation in a human format carried +// along so the pasted command renders the same way. +func timesheetEntryDeleteCmd(cmd *cobra.Command, entryID int64) string { + c := fmt.Sprintf("basecamp timesheet entry delete %d", entryID) + if machineReadsThisOutput(cmd) { + return c + " --force" + } + return c + humanFormatOverride(appctx.FromContext(cmd.Context())) +} diff --git a/internal/commands/timesheet_entries_test.go b/internal/commands/timesheet_entries_test.go index dea0a2c4b..9f351766a 100644 --- a/internal/commands/timesheet_entries_test.go +++ b/internal/commands/timesheet_entries_test.go @@ -264,3 +264,27 @@ func TestTimesheetEntryVerbsHoldAURLToItsHostAndAccount(t *testing.T) { }) } } + +// The delete breadcrumb is something its reader can run: machine output cannot +// answer a confirmation, so the JSON a test renders carries --force. +func TestTimesheetEntryDeleteBreadcrumbCarriesForceForMachineOutput(t *testing.T) { + app, _, out := setupPersonalFeedApp(t, + stubRoute{method: http.MethodGet, path: timesheetEntryPath(1069480283), status: http.StatusOK, body: timesheetEntryFixture}) + app.Flags.Hints = true // breadcrumbs are stripped from the envelope without this + require.NoError(t, executeRecordingCommand(NewTimesheetCmd(), app, "entry", "show", "1069480283")) + + var envelope struct { + Breadcrumbs []struct { + Action string `json:"action"` + Cmd string `json:"cmd"` + } `json:"breadcrumbs"` + } + require.NoError(t, json.Unmarshal(out.Bytes(), &envelope), out.String()) + var deleteCmd string + for _, b := range envelope.Breadcrumbs { + if b.Action == "delete" { + deleteCmd = b.Cmd + } + } + assert.Equal(t, "basecamp timesheet entry delete 1069480283 --force", deleteCmd) +} From c7cba822cfb9e8e00816aec97aff0ee11a62e764 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 28 Oct 2026 00:24:11 -0500 Subject: [PATCH 4/6] Take only an id or a trusted item URL for timesheet entries Anything but a bare id is a URL that has to be http(s) on a trusted Basecamp host. create takes only the item kinds bc3 logs time against, so a person's or project's URL can't send its id out as a recording's. URL accounts compare numerically, and the follow-up commands carry an account adopted from the URL. --- internal/commands/timesheet_entries.go | 101 ++++++++++++++------ internal/commands/timesheet_entries_test.go | 44 +++++++++ 2 files changed, 114 insertions(+), 31 deletions(-) diff --git a/internal/commands/timesheet_entries.go b/internal/commands/timesheet_entries.go index 7fbdb2592..0b583b8e5 100644 --- a/internal/commands/timesheet_entries.go +++ b/internal/commands/timesheet_entries.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "regexp" + "slices" "strconv" "strings" "time" @@ -69,6 +70,7 @@ time is for, and the item it is logged against. } app := appctx.FromContext(cmd.Context()) + scope := newFollowUpScope(app) if err := timesheetURLAccount(app, args[0], urlAccountID); err != nil { return err } @@ -83,7 +85,7 @@ time is for, and the item it is logged against. return app.OK(entry, output.WithSummary(timesheetEntrySummary("Timesheet entry", entry)), - output.WithBreadcrumbs(timesheetEntryBreadcrumbs(cmd, entry)...), + output.WithBreadcrumbs(timesheetEntryBreadcrumbs(cmd, entry, scope.args(app.Config.AccountID))...), ) }, } @@ -130,6 +132,7 @@ project. } app := appctx.FromContext(cmd.Context()) + scope := newFollowUpScope(app) if err := timesheetURLAccount(app, args[0], urlAccountID); err != nil { return err } @@ -155,7 +158,7 @@ project. return app.OK(entry, output.WithSummary(timesheetEntrySummary("Logged timesheet entry", entry)), - output.WithBreadcrumbs(timesheetEntryBreadcrumbs(cmd, entry)...), + output.WithBreadcrumbs(timesheetEntryBreadcrumbs(cmd, entry, scope.args(app.Config.AccountID))...), ) }, } @@ -216,6 +219,7 @@ cleared through the API, only replaced. } app := appctx.FromContext(cmd.Context()) + scope := newFollowUpScope(app) if err := timesheetURLAccount(app, args[0], urlAccountID); err != nil { return err } @@ -236,7 +240,7 @@ cleared through the API, only replaced. return app.OK(entry, output.WithSummary(timesheetEntrySummary("Updated timesheet entry", entry)), - output.WithBreadcrumbs(timesheetEntryBreadcrumbs(cmd, entry)...), + output.WithBreadcrumbs(timesheetEntryBreadcrumbs(cmd, entry, scope.args(app.Config.AccountID))...), ) }, } @@ -277,6 +281,7 @@ who may not remove the entry are refused. } app := appctx.FromContext(cmd.Context()) + scope := newFollowUpScope(app) if err := timesheetURLAccount(app, args[0], urlAccountID); err != nil { return err } @@ -304,7 +309,7 @@ who may not remove the entry are refused. output.WithSummary(fmt.Sprintf("Deleted timesheet entry #%d", entryID)), output.WithBreadcrumbs(output.Breadcrumb{ Action: "report", - Cmd: "basecamp timesheet report", + Cmd: "basecamp timesheet report" + scope.args(app.Config.AccountID), Description: "View timesheet report", }), ) @@ -347,26 +352,37 @@ func timesheetEntryIDArg(arg string) (id int64, urlAccountID string, err error) return id, urlAccountID, nil } -// timesheetRecordingIDArg resolves create's : the item the -// time is logged against, and the URL's account. A URL that names no single -// recording is refused — a project's or a project timesheet's URL carries the -// project's id, which extractID would otherwise fall back to. +// timesheetKinds are the URL types that name an item bc3 logs time against, +// as its API docs list them, or a generic recordings/ URL, which is as good +// as its bare id. bc3 still decides each case; the list keeps a URL for +// something else (a person, a project, a Campfire, a path the router only +// matched by shape) from sending its id out as a recording's. +var timesheetKinds = []string{ + "cards", "documents", "messages", "recordings", "schedule_entries", "todos", "uploads", +} + +// timesheetRecordingIDArg resolves create's : a bare id, or +// the http(s) URL of one item of a timesheet kind, returning the URL's account +// too. Anything else is refused rather than read as a recording id. func timesheetRecordingIDArg(arg string) (id int64, urlAccountID string, err error) { - if parsed := urlarg.Parse(arg); parsed != nil { - if parsed.RecordingID == "" || parsed.IsCollection || parsed.Type == "timesheet" { + raw := strings.TrimSpace(arg) + if !isAllDigits(raw) { + parsed := urlarg.Parse(raw) + switch { + case !hasHTTPScheme(raw) || parsed == nil || parsed.IsCollection || parsed.RecordingID == "" || + !slices.Contains(timesheetKinds, parsed.Type): return 0, "", output.ErrUsageHint( - fmt.Sprintf("%q does not name one item", arg), - "Pass the id or URL of the to-do, message, card or other item to log time against. "+ + fmt.Sprintf("%q does not name an item to log time against", arg), + "Pass the id or URL of the to-do, message, card, document, upload or schedule entry. "+ "For a project-level entry, pass the project timesheet's id: the parent id of a "+ "project-level entry in basecamp timesheet project") + case parsed.OccurrenceDate != "": + return 0, "", output.ErrUsageHint("That URL names one occurrence of a recurring event", + "Pass the occurrence's own id, which "+showOccurrence(raw)+" prints, or the event's id") } - urlAccountID = parsed.AccountID - } - idStr, err := extractID(arg) - if err != nil { - return 0, "", err + raw, urlAccountID = parsed.RecordingID, parsed.AccountID } - id, perr := strconv.ParseInt(idStr, 10, 64) + id, perr := strconv.ParseInt(raw, 10, 64) if perr != nil || id <= 0 { return 0, "", output.ErrUsageHint( fmt.Sprintf("%q is not an item id or Basecamp URL", arg), @@ -375,23 +391,42 @@ func timesheetRecordingIDArg(arg string) (id int64, urlAccountID string, err err return id, urlAccountID, nil } +func isAllDigits(s string) bool { + if s == "" { + return false + } + for _, r := range s { + if r < '0' || r > '9' { + return false + } + } + return true +} + +func hasHTTPScheme(s string) bool { + lower := strings.ToLower(s) + return strings.HasPrefix(lower, "https://") || strings.HasPrefix(lower, "http://") +} + // timesheetURLAccount holds a pasted URL to the account it names, before any -// request. The URL router is host-agnostic, so a URL must be on a trusted -// Basecamp host; its account is adopted when none is configured and must -// match the configured one otherwise, rather than acting on a same-numbered -// entry or item in another account. Bare ids skip both checks. +// request. Any argument that is not a bare id is a URL here, and the URL +// router is host-agnostic, so it must be on a trusted Basecamp host; its +// account is adopted when none is configured and must equal the configured one +// otherwise (numerically, as accountIDsEqual compares), rather than acting on +// a same-numbered entry or item in another account. func timesheetURLAccount(app *appctx.App, arg, urlAccountID string) error { - if !strings.Contains(arg, "://") { + raw := strings.TrimSpace(arg) + if isAllDigits(raw) { return nil } - if !hostutil.IsTrustedBasecampHost(strings.TrimSpace(arg), app.Config.BaseURL) { + if !hostutil.IsTrustedBasecampHost(raw, app.Config.BaseURL) { return output.ErrUsage("refusing untrusted host in URL — expected a Basecamp URL") } switch { case urlAccountID == "": case app.Config.AccountID == "": app.Config.AccountID = urlAccountID - case urlAccountID != app.Config.AccountID: + case !accountIDsEqual(urlAccountID, app.Config.AccountID): return output.ErrUsage(fmt.Sprintf("URL account %s does not match the configured account %s", urlAccountID, app.Config.AccountID)) } return nil @@ -461,23 +496,27 @@ func timesheetEntrySummary(prefix string, entry *basecamp.TimesheetEntry) string // timesheetEntryBreadcrumbs are the follow-ups for one entry: change or delete // it, and see the rest of the time on its item. -func timesheetEntryBreadcrumbs(cmd *cobra.Command, entry *basecamp.TimesheetEntry) []output.Breadcrumb { +// +// scopeArgs carries the profile and an account that would not come back on its +// own (adopted from a URL, given with --account), so a pasted follow-up acts +// where this one did. +func timesheetEntryBreadcrumbs(cmd *cobra.Command, entry *basecamp.TimesheetEntry, scopeArgs string) []output.Breadcrumb { crumbs := []output.Breadcrumb{ { Action: "update", - Cmd: fmt.Sprintf("basecamp timesheet entry update %d --hours ", entry.ID), + Cmd: fmt.Sprintf("basecamp timesheet entry update %d --hours %s", entry.ID, scopeArgs), Description: "Change entry", }, { Action: "delete", - Cmd: timesheetEntryDeleteCmd(cmd, entry.ID), + Cmd: timesheetEntryDeleteCmd(cmd, entry.ID, scopeArgs), Description: "Delete entry", }, } if entry.Parent != nil && entry.Parent.ID != 0 { crumbs = append(crumbs, output.Breadcrumb{ Action: "item", - Cmd: fmt.Sprintf("basecamp timesheet item %d", entry.Parent.ID), + Cmd: fmt.Sprintf("basecamp timesheet item %d%s", entry.Parent.ID, scopeArgs), Description: "View the item's timesheet", }) } @@ -489,8 +528,8 @@ func timesheetEntryBreadcrumbs(cmd *cobra.Command, entry *basecamp.TimesheetEntr // confirmation, so it gets --force; a person gets the bare command and is // asked, with any flag holding this invocation in a human format carried // along so the pasted command renders the same way. -func timesheetEntryDeleteCmd(cmd *cobra.Command, entryID int64) string { - c := fmt.Sprintf("basecamp timesheet entry delete %d", entryID) +func timesheetEntryDeleteCmd(cmd *cobra.Command, entryID int64, scopeArgs string) string { + c := fmt.Sprintf("basecamp timesheet entry delete %d%s", entryID, scopeArgs) if machineReadsThisOutput(cmd) { return c + " --force" } diff --git a/internal/commands/timesheet_entries_test.go b/internal/commands/timesheet_entries_test.go index 9f351766a..3315d6b90 100644 --- a/internal/commands/timesheet_entries_test.go +++ b/internal/commands/timesheet_entries_test.go @@ -133,6 +133,11 @@ func TestTimesheetEntryCreateRefusesBadInput(t *testing.T) { // Neither does a project's URL, or a listing's. {"entry", "create", "https://3.basecamp.com/99999/projects/89", "--hours", "1"}, {"entry", "create", "https://3.basecamp.com/99999/buckets/89/todolists", "--hours", "1"}, + // A person's URL parses to an id, but not a recording's. + {"entry", "create", "https://3.basecamp.com/99999/people/789", "--hours", "1"}, + // Without a scheme there is no host to trust. + {"entry", "create", "//evil.example/11111/buckets/89/todos/789", "--hours", "1"}, + {"entry", "create", "3.basecamp.com/99999/buckets/89/todos/789", "--hours", "1"}, } { transport, _, _, err := runTimesheet(t, nil, args...) requireBookmarksUsageError(t, err) @@ -256,6 +261,8 @@ func TestTimesheetEntryVerbsHoldAURLToItsHostAndAccount(t *testing.T) { {"update other account", []string{"entry", "update", "https://3.basecampapi.com/11111/timesheet_entries/1069480283.json", "--hours", "1"}}, {"delete other account", []string{"entry", "delete", "https://3.basecamp.com/11111/projects/89/timesheet/entries/1069480283", "--force"}}, {"create other account", []string{"entry", "create", "https://3.basecamp.com/11111/buckets/89/todos/789", "--hours", "1"}}, + {"show scheme-less", []string{"entry", "show", "//evil.example/11111/timesheet_entries/1069480283"}}, + {"delete scheme-less", []string{"entry", "delete", "3.basecamp.com/99999/timesheet_entries/1069480283", "--force"}}, } { t.Run(tc.name, func(t *testing.T) { transport, _, _, err := runTimesheet(t, nil, tc.args...) @@ -288,3 +295,40 @@ func TestTimesheetEntryDeleteBreadcrumbCarriesForceForMachineOutput(t *testing.T } assert.Equal(t, "basecamp timesheet entry delete 1069480283 --force", deleteCmd) } + +// Account ids compare as numbers, so a zero-padded URL account is the +// configured one. +func TestTimesheetEntryAcceptsAZeroPaddedAccountInAURL(t *testing.T) { + transport, _, _, err := runTimesheet(t, + []stubRoute{{method: http.MethodGet, path: timesheetEntryPath(1069480283), status: http.StatusOK, body: timesheetEntryFixture}}, + "entry", "show", "https://3.basecamp.com/099999/projects/89/timesheet/entries/1069480283") + require.NoError(t, err) + assert.Equal(t, timesheetEntryPath(1069480283), transport.last(t).Path) +} + +// An account adopted from a pasted URL lasts only for this run, so the +// follow-ups name it; otherwise a pasted update or delete would act in +// whatever account the next run resolves. +func TestTimesheetEntryBreadcrumbsCarryAnAccountAdoptedFromTheURL(t *testing.T) { + app, _, out := setupPersonalFeedApp(t, + stubRoute{method: http.MethodGet, path: timesheetEntryPath(1069480283), status: http.StatusOK, body: timesheetEntryFixture}) + app.Config.AccountID = "" + app.Flags.Hints = true + require.NoError(t, executeRecordingCommand(NewTimesheetCmd(), app, "entry", "show", + "https://3.basecamp.com/99999/projects/89/timesheet/entries/1069480283")) + + var envelope struct { + Breadcrumbs []struct { + Action string `json:"action"` + Cmd string `json:"cmd"` + } `json:"breadcrumbs"` + } + require.NoError(t, json.Unmarshal(out.Bytes(), &envelope), out.String()) + got := map[string]string{} + for _, b := range envelope.Breadcrumbs { + got[b.Action] = b.Cmd + } + assert.Equal(t, "basecamp timesheet entry update 1069480283 --hours --account 99999", got["update"]) + assert.Equal(t, "basecamp timesheet entry delete 1069480283 --account 99999 --force", got["delete"]) + assert.Equal(t, "basecamp timesheet item 789 --account 99999", got["item"]) +} From badc84cb8d6845fd539622eebb6fdf25aa17b137 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 28 Oct 2026 01:12:28 -0500 Subject: [PATCH 5/6] Match a timesheet URL's parsed path, not its raw text A path written after a # or ? is not the URL's path, so entry URLs are parsed first and only their path is matched, and create routes only the scheme, host and path. URLs carrying userinfo are refused. --- internal/commands/timesheet_entries.go | 38 ++++++++++++++++----- internal/commands/timesheet_entries_test.go | 5 +++ 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/internal/commands/timesheet_entries.go b/internal/commands/timesheet_entries.go index 0b583b8e5..1e9c4b953 100644 --- a/internal/commands/timesheet_entries.go +++ b/internal/commands/timesheet_entries.go @@ -3,6 +3,7 @@ package commands import ( "errors" "fmt" + "net/url" "regexp" "slices" "strconv" @@ -321,10 +322,10 @@ who may not remove the entry are refused. return cmd } -// timesheetEntryURL matches an entry's own URL: its app URL +// timesheetEntryRoute matches the path of an entry's own URL: its app URL // (projects/:p/timesheet/entries/:id) and its API URL in either form. The // first group is the account, the second the entry. -var timesheetEntryURL = regexp.MustCompile(`^https?://[^/]+/(\d+)/(?:(?:projects|buckets)/\d+/timesheet/entries|timesheet_entries)/(\d+)(?:\.json)?/?(?:[?#].*)?$`) +var timesheetEntryRoute = regexp.MustCompile(`^/(\d+)/(?:(?:projects|buckets)/\d+/timesheet/entries|timesheet_entries)/(\d+)(?:\.json)?/?$`) // timesheetEntryIDArg resolves the positional the per-entry verbs // take: a bare id or the entry's own URL, returning the URL's account too. Any @@ -333,8 +334,11 @@ var timesheetEntryURL = regexp.MustCompile(`^https?://[^/]+/(\d+)/(?:(?:projects // aim at one of those. func timesheetEntryIDArg(arg string) (id int64, urlAccountID string, err error) { raw := strings.TrimSpace(arg) - if strings.Contains(raw, "://") { - m := timesheetEntryURL.FindStringSubmatch(raw) + if !isAllDigits(raw) { + var m []string + if u := pastedURL(raw); u != nil { + m = timesheetEntryRoute.FindStringSubmatch(u.Path) + } if m == nil { return 0, "", output.ErrUsageHint( fmt.Sprintf("%q does not name a timesheet entry", arg), @@ -367,9 +371,15 @@ var timesheetKinds = []string{ func timesheetRecordingIDArg(arg string) (id int64, urlAccountID string, err error) { raw := strings.TrimSpace(arg) if !isAllDigits(raw) { - parsed := urlarg.Parse(raw) + // Route only the URL's own scheme, host and path, so nothing in a query + // or fragment can be read as the item. No timesheet kind is named by a + // fragment: a comment's #__recording_ cannot hold time. + var parsed *urlarg.Parsed + if u := pastedURL(raw); u != nil { + parsed = urlarg.Parse(u.Scheme + "://" + u.Host + u.Path) + } switch { - case !hasHTTPScheme(raw) || parsed == nil || parsed.IsCollection || parsed.RecordingID == "" || + case parsed == nil || parsed.IsCollection || parsed.RecordingID == "" || !slices.Contains(timesheetKinds, parsed.Type): return 0, "", output.ErrUsageHint( fmt.Sprintf("%q does not name an item to log time against", arg), @@ -403,9 +413,19 @@ func isAllDigits(s string) bool { return true } -func hasHTTPScheme(s string) bool { - lower := strings.ToLower(s) - return strings.HasPrefix(lower, "https://") || strings.HasPrefix(lower, "http://") +// pastedURL parses an http(s) URL with a host, or returns nil. Callers match +// its Path alone: a regexp over the raw string would read text after a ? or # +// as path segments. +func pastedURL(s string) *url.URL { + u, err := url.Parse(s) + if err != nil || u.Host == "" || u.User != nil { + return nil + } + switch strings.ToLower(u.Scheme) { + case "http", "https": + return u + } + return nil } // timesheetURLAccount holds a pasted URL to the account it names, before any diff --git a/internal/commands/timesheet_entries_test.go b/internal/commands/timesheet_entries_test.go index 3315d6b90..77c02fcc7 100644 --- a/internal/commands/timesheet_entries_test.go +++ b/internal/commands/timesheet_entries_test.go @@ -263,6 +263,11 @@ func TestTimesheetEntryVerbsHoldAURLToItsHostAndAccount(t *testing.T) { {"create other account", []string{"entry", "create", "https://3.basecamp.com/11111/buckets/89/todos/789", "--hours", "1"}}, {"show scheme-less", []string{"entry", "show", "//evil.example/11111/timesheet_entries/1069480283"}}, {"delete scheme-less", []string{"entry", "delete", "3.basecamp.com/99999/timesheet_entries/1069480283", "--force"}}, + // A path written after a fragment or query is not the URL's path. + {"update path in fragment", []string{"entry", "update", "https://3.basecamp.com#x/99999/timesheet_entries/1069480283", "--hours", "1"}}, + {"delete path in query", []string{"entry", "delete", "https://3.basecamp.com/?x=/99999/timesheet_entries/1069480283", "--force"}}, + {"create path in fragment", []string{"entry", "create", "https://3.basecamp.com#x/99999/buckets/89/todos/789", "--hours", "1"}}, + {"create userinfo", []string{"entry", "create", "https://evil.example@3.basecamp.com/99999/buckets/89/todos/789", "--hours", "1"}}, } { t.Run(tc.name, func(t *testing.T) { transport, _, _, err := runTimesheet(t, nil, tc.args...) From 87ddf418a2e464e8f1817f69c01da23b81f01a0a Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 28 Oct 2026 01:21:10 -0500 Subject: [PATCH 6/6] Name whose time a timesheet entry is in its summary Styled and Markdown output skip the nested person, so the summary carries the person's name and id. --- internal/commands/timesheet_entries.go | 5 +++++ internal/commands/timesheet_entries_test.go | 6 +++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/internal/commands/timesheet_entries.go b/internal/commands/timesheet_entries.go index 1e9c4b953..3b74899e5 100644 --- a/internal/commands/timesheet_entries.go +++ b/internal/commands/timesheet_entries.go @@ -508,6 +508,11 @@ func timesheetPersonID(cmd *cobra.Command, app *appctx.App, person string) (int6 func timesheetEntrySummary(prefix string, entry *basecamp.TimesheetEntry) string { summary := fmt.Sprintf("%s #%d: %sh on %s", prefix, entry.ID, entry.Hours, entry.Date) + // The generic renderers skip nested objects, so whose time it is would be + // missing from styled and Markdown output without this. + if entry.Person != nil && entry.Person.ID != 0 { + summary += fmt.Sprintf(" by %s (#%d)", entry.Person.Name, entry.Person.ID) + } if entry.Parent != nil && entry.Parent.Title != "" { summary += " for " + entry.Parent.Title } diff --git a/internal/commands/timesheet_entries_test.go b/internal/commands/timesheet_entries_test.go index 77c02fcc7..ef101a4dc 100644 --- a/internal/commands/timesheet_entries_test.go +++ b/internal/commands/timesheet_entries_test.go @@ -55,7 +55,7 @@ func TestTimesheetEntryShowGetsTheEntry(t *testing.T) { call := transport.last(t) assert.Equal(t, http.MethodGet, call.Method) assert.Equal(t, timesheetEntryPath(1069480283), call.Path) - assert.Equal(t, "Timesheet entry #1069480283: 1.5h on 2026-10-26 for Plan the offsite", summary) + assert.Equal(t, "Timesheet entry #1069480283: 1.5h on 2026-10-26 by Victor Cooper (#7) for Plan the offsite", summary) var entry map[string]any require.NoError(t, json.Unmarshal(data, &entry)) @@ -89,7 +89,7 @@ func TestTimesheetEntryCreatePostsDateHoursDescriptionAndPerson(t *testing.T) { assert.Equal(t, map[string]any{ "date": "2026-10-26", "hours": "1:30", "description": "Review", "person_id": float64(7), }, decodeSubtaskBody(t, call)) - assert.Equal(t, "Logged timesheet entry #1069480283: 1.5h on 2026-10-26 for Plan the offsite", summary) + assert.Equal(t, "Logged timesheet entry #1069480283: 1.5h on 2026-10-26 by Victor Cooper (#7) for Plan the offsite", summary) } // --date defaults to today, and unset optional fields are not sent. @@ -155,7 +155,7 @@ func TestTimesheetEntryUpdateSendsOnlyWhatChanged(t *testing.T) { assert.Equal(t, http.MethodPut, call.Method) assert.Equal(t, timesheetEntryPath(1069480283), call.Path) assert.Equal(t, map[string]any{"hours": "2.5"}, decodeSubtaskBody(t, call)) - assert.Equal(t, "Updated timesheet entry #1069480283: 1.5h on 2026-10-26 for Plan the offsite", summary) + assert.Equal(t, "Updated timesheet entry #1069480283: 1.5h on 2026-10-26 by Victor Cooper (#7) for Plan the offsite", summary) } func TestTimesheetEntryUpdateSendsEveryField(t *testing.T) {