From 58c5f17b6126e89130643b0329958c166bb7a316 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 18 Jul 2026 13:47:17 -0700 Subject: [PATCH 1/3] Bump SDK to d15f023f: merge-safe Update, oauth pointer endpoints Pins github.com/basecamp/basecamp-sdk/go at v0.7.4-0.20260718061625-d15f023f64dc (basecamp/basecamp-sdk#375), which makes TodosService.Update merge-safe (GET -> overlay -> full PUT), adds TodosService.Edit/Replace, and round-trips completion subscribers on the public Todo model. Edit and Replace are alternate write semantics for the already-covered todo update endpoint - no new CLI command, no API-COVERAGE change. Compatibility fallout absorbed here so this commit is green standalone: - OAuth discovery (SDK #369): Config.AuthorizationEndpoint, DeviceAuthorizationEndpoint, and RegistrationEndpoint became *string and discovery now requires an issuer bound code-point-exact to the base URL. Login asserts authorization-code capability immediately after discovery - before scope handling and Dynamic Client Registration - so a device-only server fails cleanly without DCR side effects. buildAuthURL keeps a defensive nil check for other callers. Test discovery fixtures gain the required issuer field. - Credential-origin guard: the SDK now refuses to attach credentials to requests targeting a different origin than BaseURL. Mock-transport tests previously built clients with an empty BaseURL, producing path-only URLs the guard rejects; they now set a test BaseURL. - Merge-safe Update: sparse UpdateTodoRequest callers (assign batch, card steps, TUI hub) silently become merge-safe - the SDK GETs full state before the PUT, at the cost of one extra GET per update. Assign batch fixtures gain the now-required content field, and the lazy resolution test pins the full order: validation GET -> person resolution -> SDK merge GET -> PUT. - todos update: subscriber clearing now routes through the raw-PUT clear branch. The sparse-Update branch cleared subscribers by omission, but merge-safe Update always sends completion_subscriber_ids, so omission no longer clears. Interim plumbing; collapsed onto Edit next. Also fixes scripts/bump-sdk.sh reading api.synced_at: the SDK provenance file exposes .bc3.date, not .bc3.synced_at, so the field landed empty. --- go.mod | 2 +- go.sum | 4 +- internal/auth/auth.go | 37 ++++++++--- internal/auth/auth_test.go | 85 +++++++++++++++++++++---- internal/commands/assign_test.go | 31 +++++---- internal/commands/boost_test.go | 2 +- internal/commands/cards_test.go | 4 +- internal/commands/chat_test.go | 14 ++-- internal/commands/comment_test.go | 2 +- internal/commands/doctor_test.go | 2 +- internal/commands/helpers_test.go | 2 +- internal/commands/hillcharts_test.go | 2 +- internal/commands/messages_test.go | 4 +- internal/commands/people_test.go | 4 +- internal/commands/profile_test.go | 2 +- internal/commands/projects_test.go | 2 +- internal/commands/quickstart_test.go | 2 +- internal/commands/recordings_test.go | 2 +- internal/commands/search_test.go | 2 +- internal/commands/show_test.go | 2 +- internal/commands/todos.go | 7 +- internal/commands/todos_test.go | 24 +++---- internal/tui/resolve/account_test.go | 2 +- internal/tui/resolve/dock_test.go | 2 +- internal/tui/workspace/data/hub_test.go | 2 +- internal/version/sdk-provenance.json | 10 +-- scripts/bump-sdk.sh | 4 +- 27 files changed, 173 insertions(+), 85 deletions(-) diff --git a/go.mod b/go.mod index 9c34285b9..b99b4a60d 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,7 @@ require ( charm.land/bubbles/v2 v2.1.1 charm.land/bubbletea/v2 v2.0.8 charm.land/lipgloss/v2 v2.0.5 - github.com/basecamp/basecamp-sdk/go v0.7.4-0.20260629111348-cc8e9772e729 + github.com/basecamp/basecamp-sdk/go v0.7.4-0.20260718061625-d15f023f64dc github.com/basecamp/cli v0.2.1 github.com/charmbracelet/bubbles v1.0.0 github.com/charmbracelet/glamour v1.0.0 diff --git a/go.sum b/go.sum index 57b6e6447..5a4493001 100644 --- a/go.sum +++ b/go.sum @@ -23,8 +23,8 @@ github.com/aymanbagabas/go-udiff v0.4.1 h1:OEIrQ8maEeDBXQDoGCbbTTXYJMYRCRO1fnodZ github.com/aymanbagabas/go-udiff v0.4.1/go.mod h1:0L9PGwj20lrtmEMeyw4WKJ/TMyDtvAoK9bf2u/mNo3w= github.com/aymerick/douceur v0.2.0 h1:Mv+mAeH1Q+n9Fr+oyamOlAkUNPWPlA8PPGR0QAaYuPk= github.com/aymerick/douceur v0.2.0/go.mod h1:wlT5vV2O3h55X9m7iVYN0TBM0NH/MmbLnd30/FjWUq4= -github.com/basecamp/basecamp-sdk/go v0.7.4-0.20260629111348-cc8e9772e729 h1:bYGNjaxnlTTDUzn7SNgBGawMFkQ0UZB1Kfw3WJWi8eY= -github.com/basecamp/basecamp-sdk/go v0.7.4-0.20260629111348-cc8e9772e729/go.mod h1:39n0Qo1z9IWCnIIJGgTu19OukfnFbvJk27c2+4KzDMI= +github.com/basecamp/basecamp-sdk/go v0.7.4-0.20260718061625-d15f023f64dc h1:QGxZGLVQktaOpJDyKhj7/AY0oVqcBkL8jeFxlSn5zhs= +github.com/basecamp/basecamp-sdk/go v0.7.4-0.20260718061625-d15f023f64dc/go.mod h1:8VLJY4RwlazhxmA5rb37e6fOC0U6vwg3yjeFyT5xBW0= 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/auth/auth.go b/internal/auth/auth.go index e9f2b4b3f..ee114bafe 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -406,6 +406,13 @@ func (m *Manager) Login(ctx context.Context, opts LoginOptions) (*LoginResult, e return nil, err } + // Device-only authorization servers omit the authorization endpoint. + // Assert authorization-code capability before scope handling and client + // registration so an unsupported flow can't trigger DCR side effects. + if oauthCfg.AuthorizationEndpoint == nil || *oauthCfg.AuthorizationEndpoint == "" { + return nil, output.ErrAuth("OAuth server does not advertise an authorization endpoint (authorization-code flow unsupported)") + } + // Apply provider-aware scope rules effectiveScope := opts.Scope if oauthType == "launchpad" { @@ -541,17 +548,26 @@ func (m *Manager) discoverOAuth(ctx context.Context, log func(string)) (*oauth.C if lpErr != nil { return nil, "", lpErr } + authzEndpoint := lpURL + "/authorization/new" fallbackCfg := &oauth.Config{ - AuthorizationEndpoint: lpURL + "/authorization/new", + AuthorizationEndpoint: &authzEndpoint, TokenEndpoint: lpURL + "/authorization/token", } - log(fmt.Sprintf("Authenticating via launchpad (%s)", fallbackCfg.AuthorizationEndpoint)) + log(fmt.Sprintf("Authenticating via launchpad (%s)", authzEndpoint)) return fallbackCfg, "launchpad", nil } - log(fmt.Sprintf("Authenticating via bc3 (%s)", cfg.AuthorizationEndpoint)) + log(fmt.Sprintf("Authenticating via bc3 (%s)", strOrEmpty(cfg.AuthorizationEndpoint))) return cfg, "bc3", nil } +// strOrEmpty returns the value of p, or "" when p is nil. +func strOrEmpty(p *string) string { + if p == nil { + return "" + } + return *p +} + func (m *Manager) launchpadURL() (string, error) { if u := os.Getenv("BASECAMP_LAUNCHPAD_URL"); u != "" { if err := hostutil.RequireSecureURL(u); err != nil { @@ -573,10 +589,10 @@ func (m *Manager) loadClientCredentials(ctx context.Context, oauthCfg *oauth.Con } // Register new client via DCR - if oauthCfg.RegistrationEndpoint == "" { + if oauthCfg.RegistrationEndpoint == nil || *oauthCfg.RegistrationEndpoint == "" { return nil, output.ErrAuth("OAuth server does not support Dynamic Client Registration") } - return m.registerBC3Client(ctx, oauthCfg.RegistrationEndpoint, opts) + return m.registerBC3Client(ctx, *oauthCfg.RegistrationEndpoint, opts) } // Launchpad: resolve client credentials from env vars @@ -792,9 +808,14 @@ func (m *Manager) saveBC3Client(creds *ClientCredentials) error { } func (m *Manager) buildAuthURL(cfg *oauth.Config, oauthType, scope, state, codeChallenge, clientID string, opts *LoginOptions) (string, error) { - u, err := url.Parse(cfg.AuthorizationEndpoint) + // Login asserts this before any side effects; kept as a defensive check + // for other callers. + if cfg.AuthorizationEndpoint == nil || *cfg.AuthorizationEndpoint == "" { + return "", output.ErrAuth("OAuth server does not advertise an authorization endpoint (authorization-code flow unsupported)") + } + u, err := url.Parse(*cfg.AuthorizationEndpoint) if err != nil { - return "", output.ErrAuth(fmt.Sprintf("invalid authorization endpoint %q: %v", cfg.AuthorizationEndpoint, err)) + return "", output.ErrAuth(fmt.Sprintf("invalid authorization endpoint %q: %v", *cfg.AuthorizationEndpoint, err)) } // The authorization endpoint comes from the server-controlled discovery @@ -802,7 +823,7 @@ func (m *Manager) buildAuthURL(cfg *oauth.Config, oauthType, scope, state, codeC // open). Restrict it to https (or http on loopback for local development) // so a hostile discovery doc can't hand the OS a file:// (or other) URL. if !isSecureEndpointURL(u) { - return "", output.ErrAuth(fmt.Sprintf("invalid authorization endpoint %q: must be an absolute https URL (or http on loopback)", cfg.AuthorizationEndpoint)) + return "", output.ErrAuth(fmt.Sprintf("invalid authorization endpoint %q: must be an absolute https URL (or http on loopback)", *cfg.AuthorizationEndpoint)) } q := u.Query() diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 336a79d1e..647156980 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -60,6 +60,11 @@ func (sl *syncLogger) snapshot() []string { return cp } +// strPtr returns a pointer to s, for oauth.Config optional endpoint fields. +func strPtr(s string) *string { + return &s +} + // newTestStore creates a file-backed credential store for testing. func newTestStore(t *testing.T, dir string) *Store { t.Helper() @@ -341,9 +346,9 @@ func TestCredentialsJSON(t *testing.T) { func TestOAuthConfigJSON(t *testing.T) { cfg := &oauth.Config{ Issuer: "https://issuer.example.com", - AuthorizationEndpoint: "https://auth.example.com/authorize", + AuthorizationEndpoint: strPtr("https://auth.example.com/authorize"), TokenEndpoint: "https://auth.example.com/token", - RegistrationEndpoint: "https://auth.example.com/register", + RegistrationEndpoint: strPtr("https://auth.example.com/register"), ScopesSupported: []string{"read", "write"}, } @@ -584,7 +589,7 @@ func TestResolveClientCredentials(t *testing.T) { func TestBuildAuthURL_UsesResolvedRedirectURI(t *testing.T) { m := &Manager{cfg: config.Default(), httpClient: http.DefaultClient} oauthCfg := &oauth.Config{ - AuthorizationEndpoint: "https://auth.example.com/authorize", + AuthorizationEndpoint: strPtr("https://auth.example.com/authorize"), } opts := &LoginOptions{RedirectURI: "http://localhost:9999/my-callback"} @@ -608,7 +613,7 @@ func TestBuildAuthURL_RejectsUnsafeScheme(t *testing.T) { } for _, endpoint := range accepted { t.Run("accepts "+endpoint, func(t *testing.T) { - oauthCfg := &oauth.Config{AuthorizationEndpoint: endpoint} + oauthCfg := &oauth.Config{AuthorizationEndpoint: strPtr(endpoint)} _, err := m.buildAuthURL(oauthCfg, "bc3", "read", "state", "challenge", "cid", opts) require.NoError(t, err) }) @@ -627,7 +632,7 @@ func TestBuildAuthURL_RejectsUnsafeScheme(t *testing.T) { } for _, endpoint := range rejected { t.Run("rejects "+endpoint, func(t *testing.T) { - oauthCfg := &oauth.Config{AuthorizationEndpoint: endpoint} + oauthCfg := &oauth.Config{AuthorizationEndpoint: strPtr(endpoint)} _, err := m.buildAuthURL(oauthCfg, "bc3", "read", "state", "challenge", "cid", opts) require.Error(t, err) }) @@ -649,7 +654,7 @@ func TestBuildAuthURL_RejectsMalformedEndpoint(t *testing.T) { } for _, endpoint := range malformed { t.Run("rejects "+endpoint, func(t *testing.T) { - oauthCfg := &oauth.Config{AuthorizationEndpoint: endpoint} + oauthCfg := &oauth.Config{AuthorizationEndpoint: strPtr(endpoint)} _, err := m.buildAuthURL(oauthCfg, "bc3", "read", "state", "challenge", "cid", opts) require.Error(t, err) assert.Contains(t, err.Error(), "invalid authorization endpoint") @@ -1074,7 +1079,7 @@ func TestLoadClientCredentials_BC3_CustomRedirect_SkipsStoredClient(t *testing.T storedCreds := &ClientCredentials{ClientID: "stored-id", ClientSecret: "stored-secret"} require.NoError(t, m.saveBC3Client(storedCreds)) - oauthCfg := &oauth.Config{RegistrationEndpoint: srv.URL + "/register"} + oauthCfg := &oauth.Config{RegistrationEndpoint: strPtr(srv.URL + "/register")} // Custom redirect: should skip stored client and do DCR opts := &LoginOptions{RedirectURI: "http://localhost:7777/cb"} @@ -1204,10 +1209,11 @@ func TestLoginRemoteMode(t *testing.T) { w.Header().Set("Content-Type", "application/json") base := "http://" + r.Host fmt.Fprintf(w, `{ + "issuer": "%s", "authorization_endpoint": "%s/authorize", "token_endpoint": "%s/token", "registration_endpoint": "%s/register" - }`, base, base, base) + }`, base, base, base, base) case "/register": w.Header().Set("Content-Type", "application/json") fmt.Fprint(w, `{"client_id":"test-client","client_secret":"test-secret"}`) @@ -1291,10 +1297,11 @@ func TestLoginRemoteMode_PromptWording(t *testing.T) { w.Header().Set("Content-Type", "application/json") base := "http://" + r.Host fmt.Fprintf(w, `{ + "issuer": "%s", "authorization_endpoint": "%s/authorize", "token_endpoint": "%s/token", "registration_endpoint": "%s/register" - }`, base, base, base) + }`, base, base, base, base) case "/register": w.Header().Set("Content-Type", "application/json") fmt.Fprint(w, `{"client_id":"test-client","client_secret":"test-secret"}`) @@ -1367,10 +1374,11 @@ func TestLoginRemoteMode_StateMismatch(t *testing.T) { w.Header().Set("Content-Type", "application/json") base := "http://" + r.Host fmt.Fprintf(w, `{ + "issuer": "%s", "authorization_endpoint": "%s/authorize", "token_endpoint": "%s/token", "registration_endpoint": "%s/register" - }`, base, base, base) + }`, base, base, base, base) case "/register": w.Header().Set("Content-Type", "application/json") fmt.Fprint(w, `{"client_id":"test-client"}`) @@ -1428,10 +1436,11 @@ func TestLoginRemoteMode_EmptyInput(t *testing.T) { w.Header().Set("Content-Type", "application/json") base := "http://" + r.Host fmt.Fprintf(w, `{ + "issuer": "%s", "authorization_endpoint": "%s/authorize", "token_endpoint": "%s/token", "registration_endpoint": "%s/register" - }`, base, base, base) + }`, base, base, base, base) case "/register": w.Header().Set("Content-Type", "application/json") fmt.Fprint(w, `{"client_id":"test-client"}`) @@ -1487,10 +1496,11 @@ func TestLoginRemoteMode_CustomRedirectURI(t *testing.T) { w.Header().Set("Content-Type", "application/json") base := "http://" + r.Host fmt.Fprintf(w, `{ + "issuer": "%s", "authorization_endpoint": "%s/authorize", "token_endpoint": "%s/token", "registration_endpoint": "%s/register" - }`, base, base, base) + }`, base, base, base, base) case "/register": w.Header().Set("Content-Type", "application/json") fmt.Fprint(w, `{"client_id":"test-client"}`) @@ -1707,10 +1717,11 @@ func TestLoginBC3DefaultsToRead(t *testing.T) { w.Header().Set("Content-Type", "application/json") base := "http://" + r.Host fmt.Fprintf(w, `{ + "issuer": "%s", "authorization_endpoint": "%s/authorize", "token_endpoint": "%s/token", "registration_endpoint": "%s/register" - }`, base, base, base) + }`, base, base, base, base) case "/register": w.Header().Set("Content-Type", "application/json") fmt.Fprint(w, `{"client_id":"test-client"}`) @@ -2049,6 +2060,54 @@ func TestRefreshLocked_EmptyOAuthTypeDefaultsToLaunchpad(t *testing.T) { assert.Contains(t, body, "client_id="+launchpadClientID) } +// TestLoginDeviceOnlyServerFailsBeforeRegistration asserts the +// authorization-code capability check fires immediately after discovery: a +// device-only server (no authorization_endpoint) fails Login with an auth +// error before Dynamic Client Registration is attempted. +func TestLoginDeviceOnlyServerFailsBeforeRegistration(t *testing.T) { + var mu sync.Mutex + var requests []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + requests = append(requests, r.URL.Path) + mu.Unlock() + switch r.URL.Path { + case "/.well-known/oauth-authorization-server": + w.Header().Set("Content-Type", "application/json") + base := "http://" + r.Host + fmt.Fprintf(w, `{ + "issuer": "%s", + "token_endpoint": "%s/token", + "device_authorization_endpoint": "%s/device", + "registration_endpoint": "%s/register", + "grant_types_supported": ["urn:ietf:params:oauth:grant-type:device_code"] + }`, base, base, base, base) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + tmpDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmpDir) + + cfg := &config.Config{BaseURL: srv.URL} + m := NewManager(cfg, srv.Client()) + m.store = newTestStore(t, tmpDir) + + _, err := m.Login(context.Background(), LoginOptions{Logger: func(string) {}}) + require.Error(t, err) + assert.Contains(t, err.Error(), "does not advertise an authorization endpoint") + + var outErr *output.Error + require.ErrorAs(t, err, &outErr) + assert.Equal(t, output.CodeAuth, outErr.Code) + + mu.Lock() + defer mu.Unlock() + assert.NotContains(t, requests, "/register", "capability check must fire before Dynamic Client Registration") +} + func TestLoginRejectsInvalidScope(t *testing.T) { tmpDir := t.TempDir() cfg := &config.Config{BaseURL: "https://3.basecampapi.com"} diff --git a/internal/commands/assign_test.go b/internal/commands/assign_test.go index 1c455d882..99ba67ff1 100644 --- a/internal/commands/assign_test.go +++ b/internal/commands/assign_test.go @@ -344,7 +344,7 @@ func setupAssignGuardTestApp(t *testing.T) *appctx.App { } authMgr := auth.NewManager(cfg, nil) - sdkClient := basecamp.NewClient(&basecamp.Config{}, &todosTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &todosTestTokenProvider{}, basecamp.WithTransport(assignGuardTransport{}), basecamp.WithMaxRetries(1), ) @@ -454,7 +454,7 @@ func setupStepPathTestApp(t *testing.T, transport *stepPathTransport) *appctx.Ap } authMgr := auth.NewManager(cfg, nil) - sdkClient := basecamp.NewClient(&basecamp.Config{}, &todosTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &todosTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) @@ -545,7 +545,7 @@ func (t *assignBatchTransport) RoundTrip(req *http.Request) (*http.Response, err for id, valid := range t.validTodoIDs { if strings.Contains(path, "/todos/"+id) { if valid { - body := fmt.Sprintf(`{"id": %s, "title": "Test Todo %s", "assignees": []}`, id, id) + body := fmt.Sprintf(`{"id": %s, "title": "Test Todo %s", "content": "Test Todo %s", "assignees": []}`, id, id, id) return &http.Response{StatusCode: 200, Body: io.NopCloser(strings.NewReader(body)), Header: header}, nil } break @@ -578,7 +578,7 @@ func setupAssignBatchTestApp(t *testing.T, transport *assignBatchTransport) (*ap } authMgr := auth.NewManager(cfg, nil) - sdkClient := basecamp.NewClient(&basecamp.Config{}, &todosTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &todosTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) @@ -666,23 +666,30 @@ func TestAssignBatchLazyResolution(t *testing.T) { err := executeAssignCommand(cmd, app, "111", "222", "--to", "me", "-p", "123") require.NoError(t, err) - // Verify request ordering: todo/111 (404), todo/222 (200), THEN profile (me resolution), THEN update + // Verify request ordering: todo/111 (404), todo/222 validation GET, + // THEN profile (me resolution), THEN the SDK's merge GET, THEN update. transport.mu.Lock() log := transport.requestLog transport.mu.Unlock() - todoGetIdx, profileIdx := -1, -1 + var todoGets []int + profileIdx, putIdx := -1, -1 for i, entry := range log { - if strings.Contains(entry, "/todos/222") && strings.HasPrefix(entry, "GET") { - todoGetIdx = i - } - if strings.Contains(entry, "/my/profile.json") { + switch { + case strings.HasPrefix(entry, "GET") && strings.Contains(entry, "/todos/222"): + todoGets = append(todoGets, i) + case strings.Contains(entry, "/my/profile.json"): profileIdx = i + case strings.HasPrefix(entry, "PUT") && strings.Contains(entry, "/todos/222"): + putIdx = i } } - require.NotEqual(t, -1, todoGetIdx, "expected GET /todos/222 in request log") + require.Len(t, todoGets, 2, "expected a validation GET and an SDK merge GET for /todos/222, got: %v", log) require.NotEqual(t, -1, profileIdx, "expected GET /my/profile.json in request log") - assert.Greater(t, profileIdx, todoGetIdx, "person resolution should happen after the valid todo is fetched") + require.NotEqual(t, -1, putIdx, "expected PUT for /todos/222 in request log") + assert.Greater(t, profileIdx, todoGets[0], "person resolution should happen after the valid todo is fetched") + assert.Greater(t, todoGets[1], profileIdx, "SDK merge GET should follow person resolution") + assert.Greater(t, putIdx, todoGets[1], "update PUT should follow the SDK merge GET") } func TestAssignBatchAllFailNeverResolvesAssignee(t *testing.T) { diff --git a/internal/commands/boost_test.go b/internal/commands/boost_test.go index 1c1947815..0d5720b54 100644 --- a/internal/commands/boost_test.go +++ b/internal/commands/boost_test.go @@ -97,7 +97,7 @@ func newBoostTestApp(transport http.RoundTripper) (*appctx.App, *bytes.Buffer) { ProjectID: "123", } - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, &boostTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), diff --git a/internal/commands/cards_test.go b/internal/commands/cards_test.go index 159b283f0..a1ae130d8 100644 --- a/internal/commands/cards_test.go +++ b/internal/commands/cards_test.go @@ -87,7 +87,7 @@ func setupTestApp(t *testing.T) (*appctx.App, *bytes.Buffer) { // Create SDK client with mock token provider and no-network transport // The transport prevents real HTTP calls - fails instantly instead of timing out authMgr := auth.NewManager(cfg, nil) - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, &testTokenProvider{}, basecamp.WithTransport(noNetworkTransport{}), basecamp.WithMaxRetries(1), // Disable retries for instant failure @@ -1542,7 +1542,7 @@ func setupCardsMockApp(t *testing.T, transport http.RoundTripper) *appctx.App { ProjectID: "123", } - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, &testTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), diff --git a/internal/commands/chat_test.go b/internal/commands/chat_test.go index b0ab955dc..27c2e8dea 100644 --- a/internal/commands/chat_test.go +++ b/internal/commands/chat_test.go @@ -129,7 +129,7 @@ func newChatDeleteTestApp(transport http.RoundTripper) (*appctx.App, *bytes.Buff ProjectID: "123", } - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, &chatTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), @@ -182,7 +182,7 @@ func TestChatPostContentIsPlainText(t *testing.T) { ProjectID: "123", } - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, &chatTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), @@ -238,7 +238,7 @@ func TestChatPostContentTypeSentInPayload(t *testing.T) { ProjectID: "123", } - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, &chatTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), @@ -282,7 +282,7 @@ func TestChatPostDefaultOmitsContentType(t *testing.T) { ProjectID: "123", } - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, &chatTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), @@ -373,7 +373,7 @@ func newTestAppWithTransport(t *testing.T, transport http.RoundTripper) (*appctx ProjectID: "123", } - sdkClient := basecamp.NewClient(&basecamp.Config{}, &chatTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &chatTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) @@ -807,7 +807,7 @@ func TestChatPostViaSubcommandWithRoomFlag(t *testing.T) { ProjectID: "123", } - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, &chatTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), @@ -1356,7 +1356,7 @@ func TestChatUploadStyledOutputIncludesFileSize(t *testing.T) { AccountID: "99999", ProjectID: "123", } - sdkClient := basecamp.NewClient(&basecamp.Config{}, &chatTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &chatTestTokenProvider{}, basecamp.WithTransport(&mockChatUploadTransport{}), basecamp.WithMaxRetries(1), ) diff --git a/internal/commands/comment_test.go b/internal/commands/comment_test.go index defce5cad..51952ca67 100644 --- a/internal/commands/comment_test.go +++ b/internal/commands/comment_test.go @@ -155,7 +155,7 @@ func setupCommentsWriteTestApp(t *testing.T, transport http.RoundTripper) (*appc t.Helper() app, buf := setupTestApp(t) - sdkClient := basecamp.NewClient(&basecamp.Config{}, &testTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &testTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) diff --git a/internal/commands/doctor_test.go b/internal/commands/doctor_test.go index 2c45378cb..e246d007f 100644 --- a/internal/commands/doctor_test.go +++ b/internal/commands/doctor_test.go @@ -343,7 +343,7 @@ func setupDoctorTestApp(t *testing.T, accountID string) (*appctx.App, *bytes.Buf authMgr := auth.NewManager(cfg, nil) - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, nil, basecamp.WithMaxRetries(1), ) diff --git a/internal/commands/helpers_test.go b/internal/commands/helpers_test.go index 80cb5a596..5e860b68f 100644 --- a/internal/commands/helpers_test.go +++ b/internal/commands/helpers_test.go @@ -256,7 +256,7 @@ func (t *dockTestTransport) RoundTrip(req *http.Request) (*http.Response, error) func newDockTestApp(t *testing.T, transport http.RoundTripper) *appctx.App { t.Helper() t.Setenv("BASECAMP_NO_KEYRING", "1") - sdk := basecamp.NewClient(&basecamp.Config{}, dockTestTokenProvider{}, + sdk := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, dockTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) diff --git a/internal/commands/hillcharts_test.go b/internal/commands/hillcharts_test.go index fb5aa5ee5..0ec8be53e 100644 --- a/internal/commands/hillcharts_test.go +++ b/internal/commands/hillcharts_test.go @@ -164,7 +164,7 @@ func setupHillchartsMockApp(t *testing.T, transport http.RoundTripper) (*appctx. AccountID: "99999", } - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, &hillchartsTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), diff --git a/internal/commands/messages_test.go b/internal/commands/messages_test.go index de70abb0f..ec0ba65f2 100644 --- a/internal/commands/messages_test.go +++ b/internal/commands/messages_test.go @@ -53,7 +53,7 @@ func setupMessagesTestApp(t *testing.T) (*appctx.App, *bytes.Buffer) { // Create SDK client with mock token provider and no-network transport // The transport prevents real HTTP calls - fails instantly instead of timing out authMgr := auth.NewManager(cfg, nil) - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, &messagesTestTokenProvider{}, basecamp.WithTransport(messagesNoNetworkTransport{}), basecamp.WithMaxRetries(1), // Disable retries for instant failure @@ -393,7 +393,7 @@ func setupMessagesMockApp(t *testing.T, transport http.RoundTripper) (*appctx.Ap ProjectID: "123", } - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, &messagesTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), diff --git a/internal/commands/people_test.go b/internal/commands/people_test.go index 5cc505416..306f05a4b 100644 --- a/internal/commands/people_test.go +++ b/internal/commands/people_test.go @@ -57,7 +57,7 @@ func setupPeopleTestApp(t *testing.T) (*appctx.App, *bytes.Buffer) { // Create auth manager without any stored credentials authMgr := auth.NewManager(cfg, nil) - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, &peopleTestTokenProvider{}, basecamp.WithTransport(peopleNoNetworkTransport{}), basecamp.WithMaxRetries(1), // Disable retries for instant failure @@ -167,7 +167,7 @@ func setupAuthenticatedTestApp(t *testing.T, accountID string, launchpadResponse // Create auth manager authMgr := auth.NewManager(cfg, nil) - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} // Use default transport to allow HTTP requests to the mock server sdkClient := basecamp.NewClient(sdkCfg, &peopleTestTokenProvider{}, basecamp.WithMaxRetries(1), diff --git a/internal/commands/profile_test.go b/internal/commands/profile_test.go index 63827efbc..9b0fe6c6f 100644 --- a/internal/commands/profile_test.go +++ b/internal/commands/profile_test.go @@ -47,7 +47,7 @@ func setupProfileTestApp(t *testing.T, cfg *config.Config) (*appctx.App, *bytes. buf := &bytes.Buffer{} authMgr := auth.NewManager(cfg, nil) - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, nil, basecamp.WithMaxRetries(1)) nameResolver := names.NewResolver(sdkClient, authMgr, cfg.AccountID) diff --git a/internal/commands/projects_test.go b/internal/commands/projects_test.go index 6ad1a02d0..00e188e42 100644 --- a/internal/commands/projects_test.go +++ b/internal/commands/projects_test.go @@ -81,7 +81,7 @@ func setupProjectsMockApp(t *testing.T, transport http.RoundTripper) (*appctx.Ap t.Setenv("BASECAMP_NO_KEYRING", "1") cfg := &config.Config{AccountID: "99999"} - sdkClient := basecamp.NewClient(&basecamp.Config{}, &testTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &testTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) diff --git a/internal/commands/quickstart_test.go b/internal/commands/quickstart_test.go index 565879927..4055905de 100644 --- a/internal/commands/quickstart_test.go +++ b/internal/commands/quickstart_test.go @@ -48,7 +48,7 @@ func setupQuickstartTestApp(t *testing.T, accountID, projectID string) (*appctx. authMgr := auth.NewManager(cfg, nil) - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, &quickstartTestTokenProvider{}, basecamp.WithTransport(quickstartNoNetworkTransport{}), basecamp.WithMaxRetries(1), diff --git a/internal/commands/recordings_test.go b/internal/commands/recordings_test.go index de9a4d2d4..4d4572b84 100644 --- a/internal/commands/recordings_test.go +++ b/internal/commands/recordings_test.go @@ -30,7 +30,7 @@ func setupRecordingsTestApp(t *testing.T) (*appctx.App, *bytes.Buffer) { } authMgr := auth.NewManager(cfg, nil) - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, &todosTestTokenProvider{}, basecamp.WithTransport(todosNoNetworkTransport{}), basecamp.WithMaxRetries(1), diff --git a/internal/commands/search_test.go b/internal/commands/search_test.go index c9f2aa6c4..85c4b89d2 100644 --- a/internal/commands/search_test.go +++ b/internal/commands/search_test.go @@ -81,7 +81,7 @@ func setupSearchTestApp(t *testing.T, transport http.RoundTripper) (*appctx.App, } authMgr := auth.NewManager(cfg, nil) - sdkClient := basecamp.NewClient(&basecamp.Config{}, &todosTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &todosTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) diff --git a/internal/commands/show_test.go b/internal/commands/show_test.go index 2eda58708..5be339757 100644 --- a/internal/commands/show_test.go +++ b/internal/commands/show_test.go @@ -181,7 +181,7 @@ func showTestApp(t *testing.T, transport http.RoundTripper) *appctx.App { t.Setenv("BASECAMP_NO_KEYRING", "1") cfg := &config.Config{AccountID: "99999"} authMgr := auth.NewManager(cfg, nil) - sdkClient := basecamp.NewClient(&basecamp.Config{}, &showTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &showTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) diff --git a/internal/commands/todos.go b/internal/commands/todos.go index 487f9ef52..66daa0bf6 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -1000,10 +1000,11 @@ Set or clear the people notified when the todo is completed: clearDue := noDue || (cmd.Flags().Changed("due") && strings.TrimSpace(due) == "") clearStarts := noStartsOn || (cmd.Flags().Changed("starts-on") && strings.TrimSpace(startsOn) == "") clearDescription := noDescription || (cmd.Flags().Changed("description") && strings.TrimSpace(description) == "") - needsClear := clearDue || clearStarts || clearDescription - // Subscriber clearing works by omission in both branches, so it - // doesn't force the raw-PUT clear branch. + // Subscriber clearing must use the raw-PUT branch: the SDK's + // merge-safe Update always sends completion_subscriber_ids, so a + // sparse update can no longer clear by omission. clearSubscribers := noNotifyOnCompletion || (cmd.Flags().Changed("notify-on-completion") && strings.TrimSpace(notifyOnCompletion) == "") + needsClear := clearDue || clearStarts || clearDescription || clearSubscribers // Clearing due while setting starts is contradictory (Basecamp enforces starts <= due) if clearDue && strings.TrimSpace(startsOn) != "" { diff --git a/internal/commands/todos_test.go b/internal/commands/todos_test.go index 4243c6cc3..2a36edab0 100644 --- a/internal/commands/todos_test.go +++ b/internal/commands/todos_test.go @@ -55,7 +55,7 @@ func setupTodosTestApp(t *testing.T) (*appctx.App, *bytes.Buffer) { // Create SDK client with mock token provider and no-network transport // The transport prevents real HTTP calls - fails instantly instead of timing out authMgr := auth.NewManager(cfg, nil) - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, &todosTestTokenProvider{}, basecamp.WithTransport(todosNoNetworkTransport{}), basecamp.WithMaxRetries(1), // Disable retries for instant failure @@ -383,7 +383,7 @@ func TestTodosCreateContentIsPlainText(t *testing.T) { TodolistID: "456", } - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, &todosTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), @@ -542,7 +542,7 @@ func setupMultiTodosetApp(t *testing.T) *appctx.App { } authMgr := auth.NewManager(cfg, nil) - sdkClient := basecamp.NewClient(&basecamp.Config{}, &todosTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &todosTestTokenProvider{}, basecamp.WithTransport(multiTodosetTransport{}), basecamp.WithMaxRetries(1), ) @@ -635,7 +635,7 @@ func setupTodos404App(t *testing.T) *appctx.App { cfg := &config.Config{AccountID: "99999"} authMgr := auth.NewManager(cfg, nil) - sdkClient := basecamp.NewClient(&basecamp.Config{}, &todosTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &todosTestTokenProvider{}, basecamp.WithTransport(todos404Transport{}), ) @@ -751,7 +751,7 @@ func setupScopedTodosetApp(t *testing.T, transport *scopedTodosetTransport) *app } authMgr := auth.NewManager(cfg, nil) - sdkClient := basecamp.NewClient(&basecamp.Config{}, &todosTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &todosTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) @@ -1009,7 +1009,7 @@ func setupGroupTodoApp(t *testing.T, transport http.RoundTripper) (*appctx.App, } authMgr := auth.NewManager(cfg, nil) - sdkClient := basecamp.NewClient(&basecamp.Config{}, &todosTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &todosTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) @@ -1215,7 +1215,7 @@ func setupStatusTestApp(t *testing.T, transport *statusCapturingTransport) (*app } authMgr := auth.NewManager(cfg, nil) - sdkClient := basecamp.NewClient(&basecamp.Config{}, &todosTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &todosTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) @@ -1465,7 +1465,7 @@ func TestSweepCommentContentIsHTML(t *testing.T) { ProjectID: "123", } - sdkClient := basecamp.NewClient(&basecamp.Config{}, &todosTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &todosTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) @@ -1507,7 +1507,7 @@ func TestSweepCommentLocalImageErrors(t *testing.T) { ProjectID: "123", } - sdkClient := basecamp.NewClient(&basecamp.Config{}, &todosTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &todosTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) @@ -1630,7 +1630,7 @@ func setupTodoUpdateApp(t *testing.T, transport http.RoundTripper) *appctx.App { AccountID: "99999", } - sdkClient := basecamp.NewClient(&basecamp.Config{}, &todosTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &todosTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) @@ -2229,7 +2229,7 @@ func setupAssigneeTodoApp(t *testing.T, transport http.RoundTripper) (*appctx.Ap } authMgr := auth.NewManager(cfg, nil) - sdkClient := basecamp.NewClient(&basecamp.Config{}, &todosTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &todosTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) @@ -2457,7 +2457,7 @@ func setupListlessTodoApp(t *testing.T, transport http.RoundTripper) (*appctx.Ap cfg := &config.Config{AccountID: "99999", ProjectID: "123"} authMgr := auth.NewManager(cfg, nil) - sdkClient := basecamp.NewClient(&basecamp.Config{}, &todosTestTokenProvider{}, + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &todosTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) diff --git a/internal/tui/resolve/account_test.go b/internal/tui/resolve/account_test.go index 14b0e51aa..b91660363 100644 --- a/internal/tui/resolve/account_test.go +++ b/internal/tui/resolve/account_test.go @@ -97,7 +97,7 @@ func TestFetchAccounts_LaunchpadToken(t *testing.T) { cfg := &config.Config{BaseURL: "https://3.basecampapi.com"} authMgr := auth.NewManager(cfg, nil) - sdkCfg := &basecamp.Config{} + sdkCfg := &basecamp.Config{BaseURL: "https://3.basecampapi.com"} sdkClient := basecamp.NewClient(sdkCfg, accountTestTokenProvider{token: "some-launchpad-token"}, basecamp.WithMaxRetries(1), ) diff --git a/internal/tui/resolve/dock_test.go b/internal/tui/resolve/dock_test.go index 24ff2e0cf..86200fd08 100644 --- a/internal/tui/resolve/dock_test.go +++ b/internal/tui/resolve/dock_test.go @@ -40,7 +40,7 @@ func (testTokenProvider) AccessToken(_ context.Context) (string, error) { } func newTestResolver(transport http.RoundTripper, flags *Flags) *Resolver { - sdk := basecamp.NewClient(&basecamp.Config{}, testTokenProvider{}, + sdk := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, testTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) diff --git a/internal/tui/workspace/data/hub_test.go b/internal/tui/workspace/data/hub_test.go index 1035c727c..f5e2c978f 100644 --- a/internal/tui/workspace/data/hub_test.go +++ b/internal/tui/workspace/data/hub_test.go @@ -76,7 +76,7 @@ func TestHubCreateCheckinAnswerDefaultsDateToToday(t *testing.T) { }) transport := &mockHubCheckinsTransport{} - sdk := basecamp.NewClient(&basecamp.Config{}, hubCheckinsTestTokenProvider{}, + sdk := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, hubCheckinsTestTokenProvider{}, basecamp.WithTransport(transport), basecamp.WithMaxRetries(1), ) diff --git a/internal/version/sdk-provenance.json b/internal/version/sdk-provenance.json index 6b6ec2add..f226d070b 100644 --- a/internal/version/sdk-provenance.json +++ b/internal/version/sdk-provenance.json @@ -1,13 +1,13 @@ { "sdk": { "module": "github.com/basecamp/basecamp-sdk/go", - "version": "v0.7.4-0.20260629111348-cc8e9772e729", - "revision": "cc8e9772e729", - "updated_at": "2026-06-29T11:13:48Z" + "version": "v0.7.4-0.20260718061625-d15f023f64dc", + "revision": "d15f023f64dc", + "updated_at": "2026-07-18T06:16:25Z" }, "api": { "repo": "basecamp/bc3", - "revision": "056a356ee0d3b018362adbc8b44703df0567adbf", - "synced_at": "" + "revision": "e52453ff35a6292473eb095a8568649aa132fef9", + "synced_at": "2026-07-17" } } diff --git a/scripts/bump-sdk.sh b/scripts/bump-sdk.sh index bed619e68..3859bb383 100755 --- a/scripts/bump-sdk.sh +++ b/scripts/bump-sdk.sh @@ -130,7 +130,7 @@ LOCAL_API_PROVENANCE="../basecamp-sdk/spec/api-provenance.json" if [[ -f "${LOCAL_API_PROVENANCE}" ]]; then echo " Reading API provenance from local SDK repo" API_REVISION=$(jq -r '.bc3.revision // ""' "${LOCAL_API_PROVENANCE}" 2>/dev/null || echo "") - API_SYNCED_AT=$(jq -r '.bc3.synced_at // ""' "${LOCAL_API_PROVENANCE}" 2>/dev/null || echo "") + API_SYNCED_AT=$(jq -r '.bc3.synced_at // .bc3.date // ""' "${LOCAL_API_PROVENANCE}" 2>/dev/null || echo "") elif command -v gh >/dev/null 2>&1 && [[ -n "${COMMIT}" ]]; then # Try remote via GitHub API echo " Fetching API provenance from GitHub (ref: ${COMMIT})" @@ -143,7 +143,7 @@ elif command -v gh >/dev/null 2>&1 && [[ -n "${COMMIT}" ]]; then fi if [[ -n "${API_JSON}" ]]; then API_REVISION=$(echo "${API_JSON}" | jq -r '.bc3.revision // ""' 2>/dev/null || echo "") - API_SYNCED_AT=$(echo "${API_JSON}" | jq -r '.bc3.synced_at // ""' 2>/dev/null || echo "") + API_SYNCED_AT=$(echo "${API_JSON}" | jq -r '.bc3.synced_at // .bc3.date // ""' 2>/dev/null || echo "") else echo " Could not fetch API provenance (file may not exist yet)" fi From a8b9aeb4da804dde5c2547eb4abbaa982327e865 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 18 Jul 2026 13:59:38 -0700 Subject: [PATCH 2/3] Collapse todos update onto TodosService.Edit (#541) Replaces the two divergent update branches from #540 - the raw-PUT clear branch and the typed merge branch, each fed by a raw flat-route preservation GET - with a single TodosService.Edit call: the SDK GETs full state (completion subscribers included, per basecamp/basecamp-sdk#375), hands the closure the complete writable representation, and PUTs the whole thing back. Exactly one typed GET and one typed PUT; the clear branch's post-PUT re-Get is gone. Fail-closed posture split: field presence is now the server/SDK contract (BC3 always serializes completion_subscribers and the SDK model pins nil-vs-empty), so the missing-key guard is gone - but the CLI still refuses to write back preserved subscriber IDs it can't trust; the Edit closure aborts before the PUT on a missing or invalid id. Accepted wire deltas, defined as clears by the Edit contract: - description clear is now sent as "description": "" (was omitted) - subscriber clear is now sent as "completion_subscriber_ids": [] (was omitted) Date clears remain omission (fullBody sends dates only when non-empty). Behavior improvement: --due/--starts-on values are validated with time.Parse before any HTTP. dateparse passes unrecognized input through as-is, so garbage (or an impossible date like 2026-99-99) previously reached the server; it now fails as a usage error with zero requests. Image uploads for --description move inside the Edit closure, after Edit's GET has confirmed the todo exists - a missing todo can no longer orphan uploaded attachments. --- internal/commands/todos.go | 273 ++++++++------------------------ internal/commands/todos_test.go | 167 ++++++++++++------- 2 files changed, 177 insertions(+), 263 deletions(-) diff --git a/internal/commands/todos.go b/internal/commands/todos.go index 66daa0bf6..7458a6b8c 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -1000,11 +1000,7 @@ Set or clear the people notified when the todo is completed: clearDue := noDue || (cmd.Flags().Changed("due") && strings.TrimSpace(due) == "") clearStarts := noStartsOn || (cmd.Flags().Changed("starts-on") && strings.TrimSpace(startsOn) == "") clearDescription := noDescription || (cmd.Flags().Changed("description") && strings.TrimSpace(description) == "") - // Subscriber clearing must use the raw-PUT branch: the SDK's - // merge-safe Update always sends completion_subscriber_ids, so a - // sparse update can no longer clear by omission. clearSubscribers := noNotifyOnCompletion || (cmd.Flags().Changed("notify-on-completion") && strings.TrimSpace(notifyOnCompletion) == "") - needsClear := clearDue || clearStarts || clearDescription || clearSubscribers // Clearing due while setting starts is contradictory (Basecamp enforces starts <= due) if clearDue && strings.TrimSpace(startsOn) != "" { @@ -1028,7 +1024,7 @@ Set or clear the people notified when the todo is completed: strings.TrimSpace(due) == "" && strings.TrimSpace(startsOn) == "" && !assigneeChanged && !subscribersChanged && (!cmd.Flags().Changed("notify") || !notify) && - !needsClear && !clearSubscribers { + !clearDue && !clearStarts && !clearDescription && !clearSubscribers { return noChanges(cmd) } @@ -1048,195 +1044,95 @@ Set or clear the people notified when the todo is completed: return output.ErrUsage("Invalid todo ID") } - var todo *basecamp.Todo + // Pre-Edit validation and resolution — no todo HTTP happens here. + // Image uploads are deferred into the Edit closure so a missing + // todo can't orphan uploaded attachments. + var descHTML string + if !clearDescription && description != "" { + descHTML = richtext.MarkdownToHTML(description) + } - if needsClear { - // The BC3 API clears fields by omission: include all fields you - // want to keep, omit those you want to clear. The SDK's typed - // UpdateTodoRequest always includes all fields, so we use a raw - // PUT with a hand-built body instead. - existingTodo, err := app.Account().Todos().Get(cmd.Context(), todoID) - if err != nil { - return convertSDKError(err) - } - if existingTodo.Bucket == nil { - return fmt.Errorf("todo %d has no associated project", todoID) + var parsedDue string + if !clearDue && strings.TrimSpace(due) != "" { + parsedDue = dateparse.Parse(due) + if _, err := time.Parse("2006-01-02", parsedDue); err != nil { + return output.ErrUsage(fmt.Sprintf("Invalid due date: %q", due)) } - - // Start with content (required). User-provided title overrides. - body := map[string]any{} - if effectiveTitle != "" { - body["content"] = effectiveTitle - } else { - body["content"] = existingTodo.Content + } + var parsedStarts string + if !clearStarts && !clearDue && strings.TrimSpace(startsOn) != "" { + parsedStarts = dateparse.Parse(startsOn) + if _, err := time.Parse("2006-01-02", parsedStarts); err != nil { + return output.ErrUsage(fmt.Sprintf("Invalid start date: %q", startsOn)) } + } - // Description: omit to clear, include new or existing to preserve. - if !clearDescription { - if description != "" { - descHTML := richtext.MarkdownToHTML(description) - descHTML, err = resolveLocalImages(cmd, app, descHTML) - if err != nil { - return err - } - body["description"] = descHTML - } else { - body["description"] = existingTodo.Description - } + var assigneeIDs []int64 + if assigneeChanged { + if assigneeIDs, err = resolveAssigneeIDs(cmd.Context(), app, assignee); err != nil { + return err } - - // Due date: omit to clear, include new or existing to preserve. - // Clearing due also clears starts (Basecamp enforces starts <= due). - if !clearDue { - if strings.TrimSpace(due) != "" { - if parsed := dateparse.Parse(due); parsed != "" { - body["due_on"] = parsed - } - } else if existingTodo.DueOn != "" { - body["due_on"] = existingTodo.DueOn - } + } + var subscriberIDs []int64 + if subscribersChanged { + if subscriberIDs, err = resolveCompletionSubscriberIDs(cmd.Context(), app, notifyOnCompletion); err != nil { + return err } + } - // Start date: omit to clear, include new or existing to preserve. - // Also omitted when clearing due (see above). - if !clearStarts && !clearDue { - if strings.TrimSpace(startsOn) != "" { - if parsed := dateparse.Parse(startsOn); parsed != "" { - body["starts_on"] = parsed + todo, err := app.Account().Todos().Edit(cmd.Context(), todoID, func(f *basecamp.TodoFields) error { + // Fail closed on unverifiable preserved subscriber state + // (#538): field presence is the server/SDK contract, but the + // CLI still refuses to write back subscriber IDs it can't + // trust. + if !subscribersChanged && !clearSubscribers { + for _, id := range f.CompletionSubscriberIDs { + if id <= 0 { + return fmt.Errorf("cannot verify current completion subscribers for todo %d: subscriber with missing or invalid id", todoID) } - } else if existingTodo.StartsOn != "" { - body["starts_on"] = existingTodo.StartsOn - } - } - - // Assignees: preserve existing unless explicitly changed. - if assigneeChanged { - assigneeIDs, err := resolveAssigneeIDs(cmd.Context(), app, assignee) - if err != nil { - return err - } - body["assignee_ids"] = assigneeIDs - } else if len(existingTodo.Assignees) > 0 { - ids := make([]int64, len(existingTodo.Assignees)) - for i, a := range existingTodo.Assignees { - ids[i] = a.ID - } - body["assignee_ids"] = ids - } - - // Completion subscribers: the SDK's Todo model drops them, so - // existingTodo can't supply them — set explicitly, clear by - // omission, or preserve via a raw read that must succeed - // before any PUT (fail closed). - if subscribersChanged { - subscriberIDs, err := resolveCompletionSubscriberIDs(cmd.Context(), app, notifyOnCompletion) - if err != nil { - return err } - body["completion_subscriber_ids"] = subscriberIDs - } else if !clearSubscribers { - subscriberIDs, err := completionSubscriberIDs(cmd.Context(), app, todoID) - if err != nil { - return err - } - if len(subscriberIDs) > 0 { - body["completion_subscriber_ids"] = subscriberIDs - } - } - - if cmd.Flags().Changed("notify") && notify { - body["notify"] = true - } - - path := fmt.Sprintf("/buckets/%d/todos/%d.json", existingTodo.Bucket.ID, todoID) - _, err = app.Account().Put(cmd.Context(), path, body) - if err != nil { - return convertSDKError(err) } - - todo, err = app.Account().Todos().Get(cmd.Context(), todoID) - if err != nil { - return convertSDKError(err) - } - } else { - // Fetch existing todo so we can preserve fields the user - // didn't change. The BC3 API clears fields by omission, - // so a partial PUT would wipe untouched fields. - existingTodo, err := app.Account().Todos().Get(cmd.Context(), todoID) - if err != nil { - return convertSDKError(err) - } - - req := &basecamp.UpdateTodoRequest{ - Content: existingTodo.Content, - Description: existingTodo.Description, - DueOn: existingTodo.DueOn, - StartsOn: existingTodo.StartsOn, - } - if len(existingTodo.Assignees) > 0 { - ids := make([]int64, len(existingTodo.Assignees)) - for i, a := range existingTodo.Assignees { - ids[i] = a.ID - } - req.AssigneeIDs = ids - } - - // Override with user-provided values. if effectiveTitle != "" { - req.Content = effectiveTitle + f.Content = effectiveTitle } - if description != "" { - descHTML := richtext.MarkdownToHTML(description) - descHTML, err = resolveLocalImages(cmd, app, descHTML) + if clearDescription { + f.Description = "" + } else if descHTML != "" { + // Uploads happen here, after Edit's GET confirmed the + // todo exists. + resolved, err := resolveLocalImages(cmd, app, descHTML) if err != nil { return err } - req.Description = descHTML + f.Description = resolved } - if strings.TrimSpace(due) != "" { - if parsed := dateparse.Parse(due); parsed != "" { - req.DueOn = parsed - } + // Clearing due also clears starts (Basecamp enforces + // starts <= due). + if clearDue { + f.DueOn, f.StartsOn = "", "" + } else if parsedDue != "" { + f.DueOn = parsedDue } - if strings.TrimSpace(startsOn) != "" { - if parsed := dateparse.Parse(startsOn); parsed != "" { - req.StartsOn = parsed - } + if clearStarts { + f.StartsOn = "" + } else if !clearDue && parsedStarts != "" { + f.StartsOn = parsedStarts } if assigneeChanged { - assigneeIDs, err := resolveAssigneeIDs(cmd.Context(), app, assignee) - if err != nil { - return err - } - req.AssigneeIDs = assigneeIDs + f.AssigneeIDs = assigneeIDs } - // Completion subscribers: the SDK's Todo model drops them, so - // existingTodo can't supply them — set explicitly, clear by - // omission (nil), or preserve via a raw read that must succeed - // before any PUT (fail closed). if subscribersChanged { - subscriberIDs, err := resolveCompletionSubscriberIDs(cmd.Context(), app, notifyOnCompletion) - if err != nil { - return err - } - req.CompletionSubscriberIDs = subscriberIDs - } else if !clearSubscribers { - subscriberIDs, err := completionSubscriberIDs(cmd.Context(), app, todoID) - if err != nil { - return err - } - if len(subscriberIDs) > 0 { - req.CompletionSubscriberIDs = subscriberIDs - } + f.CompletionSubscriberIDs = subscriberIDs + } else if clearSubscribers { + f.CompletionSubscriberIDs = []int64{} } if cmd.Flags().Changed("notify") && notify { - req.Notify = true - } - - todo, err = app.Account().Todos().Update(cmd.Context(), todoID, req) - if err != nil { - return convertSDKError(err) + f.Notify = true } + return nil + }) + if err != nil { + return convertSDKError(err) } return app.OK(todo, @@ -1286,43 +1182,6 @@ func resolveCompletionSubscriberIDs(ctx context.Context, app *appctx.App, input return resolvePersonRoleIDs(ctx, app, input, "Completion subscriber") } -// completionSubscriberIDs reads the current completion subscriber ids via a raw -// GET of /todos/{id}.json. The SDK's Todo model drops completion_subscribers in -// todoFromGenerated (basecamp/basecamp-sdk#355), so a raw read is the only way -// to preserve them across the replace-semantics PUT (#538). Delete once the SDK -// round-trips the field. -// -// Fails closed: a missing or null completion_subscribers key, a malformed -// response, or a failed GET all return an error — proceeding without knowing -// the current subscribers would silently clear them. -func completionSubscriberIDs(ctx context.Context, app *appctx.App, todoID int64) ([]int64, error) { - resp, err := app.Account().Get(ctx, fmt.Sprintf("/todos/%d.json", todoID)) - if err != nil { - return nil, fmt.Errorf("failed to read current completion subscribers for todo %d: %w", todoID, convertSDKError(err)) - } - - var raw struct { - CompletionSubscribers *[]struct { - ID int64 `json:"id"` - } `json:"completion_subscribers"` - } - if err := resp.UnmarshalData(&raw); err != nil { - return nil, fmt.Errorf("failed to parse completion subscribers for todo %d: %w", todoID, err) - } - if raw.CompletionSubscribers == nil { - return nil, fmt.Errorf("cannot verify current completion subscribers for todo %d: response missing completion_subscribers", todoID) - } - - ids := make([]int64, len(*raw.CompletionSubscribers)) - for i, s := range *raw.CompletionSubscribers { - if s.ID <= 0 { - return nil, fmt.Errorf("cannot verify current completion subscribers for todo %d: subscriber with missing or invalid id", todoID) - } - ids[i] = s.ID - } - return ids, nil -} - func newTodosCompleteCmd() *cobra.Command { cmd := &cobra.Command{ Use: "complete ...", diff --git a/internal/commands/todos_test.go b/internal/commands/todos_test.go index 2a36edab0..bfcd5b17b 100644 --- a/internal/commands/todos_test.go +++ b/internal/commands/todos_test.go @@ -1537,16 +1537,15 @@ func TestSweepCommentLocalImageErrors(t *testing.T) { // mockTodoUpdateTransport handles GET and PUT for todo update tests. It // records every request as "METHOD path" so tests can assert which routes -// were hit and in what order. The raw preservation GET (flat route -// /todos/{id}.json, distinct from the typed generated route /todos/{id}) +// were hit and in what order. The typed todo GET (Edit's read-before-write) // serves a configurable body/status so tests can exercise the fail-closed // contract. type mockTodoUpdateTransport struct { capturedBody []byte requests []string - rawGetStatus int // 0 → 200 - rawGetBody string // "" → default with completion_subscribers [7, 8] + todoGetStatus int // 0 → 200 + todoGetBody string // "" → default with completion_subscribers [7, 8] } func (t *mockTodoUpdateTransport) RoundTrip(req *http.Request) (*http.Response, error) { @@ -1555,16 +1554,15 @@ func (t *mockTodoUpdateTransport) RoundTrip(req *http.Request) (*http.Response, header := make(http.Header) header.Set("Content-Type", "application/json") - if req.Method == "GET" && req.URL.Path == "/99999/todos/999.json" { - // Raw preservation GET of the flat account-scoped /todos/{id}.json - // route — exact match so bucket-scoped paths can never satisfy it. - status := t.rawGetStatus + if req.Method == "GET" && req.URL.Path == "/99999/todos/999" { + // Typed todo GET — exact match so no other route can satisfy it. + status := t.todoGetStatus if status == 0 { status = 200 } - body := t.rawGetBody + body := t.todoGetBody if body == "" { - body = `{"id": 999, "completion_subscribers": [{"id": 7}, {"id": 8}]}` + body = `{"id": 999, "title": "Test", "content": "Test todo", "status": "active", "completed": false, "description": "Existing desc", "due_on": "2026-04-01", "starts_on": "2026-03-25", "bucket": {"id": 456, "name": "Test Project", "type": "Project"}, "assignees": [{"id": 42, "name": "Test User"}], "completion_subscribers": [{"id": 7}, {"id": 8}]}` } return &http.Response{ StatusCode: status, @@ -1582,15 +1580,6 @@ func (t *mockTodoUpdateTransport) RoundTrip(req *http.Request) (*http.Response, }, nil } - if req.Method == "GET" { - mockTodo := `{"id": 999, "title": "Test", "content": "Test todo", "status": "active", "completed": false, "description": "Existing desc", "due_on": "2026-04-01", "starts_on": "2026-03-25", "bucket": {"id": 456, "name": "Test Project", "type": "Project"}, "assignees": [{"id": 42, "name": "Test User"}]}` - return &http.Response{ - StatusCode: 200, - Body: io.NopCloser(strings.NewReader(mockTodo)), - Header: header, - }, nil - } - if req.Method == "PUT" { if req.Body != nil { body, err := io.ReadAll(req.Body) @@ -1608,7 +1597,7 @@ func (t *mockTodoUpdateTransport) RoundTrip(req *http.Request) (*http.Response, }, nil } - return nil, errors.New("unexpected request") + return nil, fmt.Errorf("unexpected request: %s %s", req.Method, req.URL.Path) } // hasRequest reports whether any recorded request matches the given method @@ -1746,6 +1735,8 @@ func TestTodosUpdateLocalImageErrors(t *testing.T) { err := executeTodosCommand(cmd, app, "update", "999", "--description", "![alt](./missing.png)") require.Error(t, err) assert.Contains(t, err.Error(), "missing.png") + assert.False(t, transport.hasRequest("PUT", "/"), + "upload failure inside the Edit closure must abort before the PUT, got: %v", transport.requests) } func TestTodosUpdateNoDueOmitsField(t *testing.T) { @@ -1772,7 +1763,7 @@ func TestTodosUpdateNoDueOmitsField(t *testing.T) { assert.False(t, startsExists, "starts_on must also be omitted when clearing due") } -func TestTodosUpdateNoDescriptionOmitsField(t *testing.T) { +func TestTodosUpdateNoDescriptionSendsEmpty(t *testing.T) { transport := &mockTodoUpdateTransport{} app := setupTodoUpdateApp(t, transport) @@ -1785,8 +1776,9 @@ func TestTodosUpdateNoDescriptionOmitsField(t *testing.T) { err = json.Unmarshal(transport.capturedBody, &body) require.NoError(t, err) - _, exists := body["description"] - assert.False(t, exists, "description must be omitted to clear") + desc, exists := body["description"] + require.True(t, exists, "description must be present as empty string to clear") + assert.Equal(t, "", desc) // Other fields preserved assert.Equal(t, "2026-04-01", body["due_on"]) @@ -1848,7 +1840,7 @@ func TestTodosUpdateEmptyStartsOnClearsField(t *testing.T) { assert.False(t, exists, "starts_on must be omitted when --starts-on is empty") } -func TestTodosUpdateEmptyDescriptionClearsField(t *testing.T) { +func TestTodosUpdateEmptyDescriptionSendsEmpty(t *testing.T) { transport := &mockTodoUpdateTransport{} app := setupTodoUpdateApp(t, transport) @@ -1861,8 +1853,9 @@ func TestTodosUpdateEmptyDescriptionClearsField(t *testing.T) { err = json.Unmarshal(transport.capturedBody, &body) require.NoError(t, err) - _, exists := body["description"] - assert.False(t, exists, "description must be omitted when --description is empty") + desc, exists := body["description"] + require.True(t, exists, "description must be present as empty string when --description is empty") + assert.Equal(t, "", desc) } func TestTodosUpdateConflictingNoDueAndDue(t *testing.T) { @@ -2002,7 +1995,10 @@ func TestTodosUpdateTitlePreservesExistingFields(t *testing.T) { "completion subscribers must be preserved") } -func TestTodosUpdatePreservationReadUsesFlatRoute(t *testing.T) { +func TestTodosUpdateSingleTypedRoundTrip(t *testing.T) { + // The collapse onto Edit means exactly one typed GET followed by one + // typed PUT — no raw flat-route preservation read, no raw bucket PUT, + // no post-PUT re-Get. transport := &mockTodoUpdateTransport{} app := setupTodoUpdateApp(t, transport) @@ -2010,14 +2006,10 @@ func TestTodosUpdatePreservationReadUsesFlatRoute(t *testing.T) { err := executeTodosCommand(cmd, app, "update", "999", "New title") require.NoError(t, err) - assert.True(t, transport.hasRequest("GET", "/todos/999.json"), - "preservation read must hit the flat /todos/{id}.json route, got: %v", transport.requests) - for _, r := range transport.requests { - assert.NotContains(t, r, "/buckets/", "no bucket-scoped request expected") - } + assert.Equal(t, []string{"GET /99999/todos/999", "PUT /99999/todos/999"}, transport.requests) } -func TestTodosUpdateExplicitSubscribersSkipsPreservationRead(t *testing.T) { +func TestTodosUpdateExplicitSubscribersSendsResolvedIDs(t *testing.T) { transport := &mockTodoUpdateTransport{} app := setupTodoUpdateApp(t, transport) @@ -2031,8 +2023,6 @@ func TestTodosUpdateExplicitSubscribersSkipsPreservationRead(t *testing.T) { require.NoError(t, err) assert.Equal(t, []any{float64(42)}, body["completion_subscriber_ids"]) - assert.False(t, transport.hasRequest("GET", "/todos/999.json"), - "explicit --notify-on-completion must not trigger a preservation read, got: %v", transport.requests) } func TestTodosUpdateNoNotifyOnCompletionClearsSubscribers(t *testing.T) { @@ -2048,10 +2038,9 @@ func TestTodosUpdateNoNotifyOnCompletionClearsSubscribers(t *testing.T) { err = json.Unmarshal(transport.capturedBody, &body) require.NoError(t, err) - _, exists := body["completion_subscriber_ids"] - assert.False(t, exists, "completion_subscriber_ids must be omitted to clear") - assert.False(t, transport.hasRequest("GET", "/todos/999.json"), - "clearing subscribers must not trigger a preservation read, got: %v", transport.requests) + ids, exists := body["completion_subscriber_ids"] + require.True(t, exists, "completion_subscriber_ids must be present as an empty list to clear") + assert.Equal(t, []any{}, ids) } func TestTodosUpdateNoDuePreservesSubscribers(t *testing.T) { @@ -2072,42 +2061,108 @@ func TestTodosUpdateNoDuePreservesSubscribers(t *testing.T) { } func TestTodosUpdateSubscriberReadFailsClosed(t *testing.T) { - // A preservation read that fails — or succeeds without an unambiguous - // completion_subscribers key — must abort the update before any PUT. + // Edit's read failing — or succeeding with subscriber state the CLI + // can't trust — must abort the update before any PUT. A missing + // completion_subscribers key is deliberately not a case here: field + // presence is the server/SDK contract (BC3 always serializes it and + // the SDK model pins nil-vs-empty); the CLI guard covers invalid IDs. cases := map[string]struct { - rawGetStatus int - rawGetBody string + todoGetStatus int + todoGetBody string + wantSubscriber bool // error mentions completion subscribers (CLI closure guard) }{ - "missing key": {rawGetBody: `{"id": 999}`}, - "malformed json": {rawGetBody: `{not json`}, - "http error": {rawGetStatus: 500, rawGetBody: `{}`}, - "invalid subscriber id": {rawGetBody: `{"id": 999, "completion_subscribers": [{"name": "No ID"}]}`}, + "http error": {todoGetStatus: 500, todoGetBody: `{}`}, + "malformed json": {todoGetBody: `{not json`}, + "invalid subscriber id": {todoGetBody: `{"id": 999, "content": "Test todo", "completion_subscribers": [{"name": "No ID"}]}`, wantSubscriber: true}, } - branches := map[string][]string{ - "merge branch": {"update", "999", "New title"}, - "clear branch": {"update", "999", "--no-due"}, + argSets := map[string][]string{ + "title update": {"update", "999", "New title"}, + "clear due": {"update", "999", "--no-due"}, } - for branchName, cmdArgs := range branches { + for argsName, cmdArgs := range argSets { for caseName, tc := range cases { - t.Run(branchName+"/"+caseName, func(t *testing.T) { + t.Run(argsName+"/"+caseName, func(t *testing.T) { transport := &mockTodoUpdateTransport{ - rawGetStatus: tc.rawGetStatus, - rawGetBody: tc.rawGetBody, + todoGetStatus: tc.todoGetStatus, + todoGetBody: tc.todoGetBody, } app := setupTodoUpdateApp(t, transport) cmd := NewTodosCmd() err := executeTodosCommand(cmd, app, cmdArgs...) require.Error(t, err) - assert.Contains(t, err.Error(), "completion subscribers") + if tc.wantSubscriber { + assert.Contains(t, err.Error(), "completion subscribers") + } assert.False(t, transport.hasRequest("PUT", "/"), - "no PUT may occur when the preservation read fails, got: %v", transport.requests) + "no PUT may occur when the Edit read fails, got: %v", transport.requests) }) } } } +func TestTodosUpdateInvalidDateFailsWithoutHTTP(t *testing.T) { + // Date validation happens before any HTTP: dateparse passes + // unrecognized input through as-is, so without pre-validation garbage + // (or an impossible calendar date) would reach the server. + cases := map[string][]string{ + "due unrecognized": {"update", "999", "--due", "garbage"}, + "due impossible": {"update", "999", "--due", "2026-99-99"}, + "starts-on unrecognized": {"update", "999", "--starts-on", "garbage"}, + "starts-on impossible": {"update", "999", "--starts-on", "2026-99-99"}, + } + + for name, cmdArgs := range cases { + t.Run(name, func(t *testing.T) { + transport := &mockTodoUpdateTransport{} + app := setupTodoUpdateApp(t, transport) + + cmd := NewTodosCmd() + err := executeTodosCommand(cmd, app, cmdArgs...) + require.Error(t, err) + assert.Contains(t, err.Error(), "Invalid") + + var outErr *output.Error + require.ErrorAs(t, err, &outErr) + assert.Equal(t, output.CodeUsage, outErr.Code) + + assert.Empty(t, transport.requests, "invalid date must fail before any HTTP") + }) + } +} + +func TestTodosUpdateNotifySendsNotify(t *testing.T) { + t.Run("with --notify", func(t *testing.T) { + transport := &mockTodoUpdateTransport{} + app := setupTodoUpdateApp(t, transport) + + cmd := NewTodosCmd() + err := executeTodosCommand(cmd, app, "update", "999", "New title", "--notify") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBody) + + var body map[string]any + require.NoError(t, json.Unmarshal(transport.capturedBody, &body)) + assert.Equal(t, true, body["notify"]) + }) + + t.Run("without --notify", func(t *testing.T) { + transport := &mockTodoUpdateTransport{} + app := setupTodoUpdateApp(t, transport) + + cmd := NewTodosCmd() + err := executeTodosCommand(cmd, app, "update", "999", "New title") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBody) + + var body map[string]any + require.NoError(t, json.Unmarshal(transport.capturedBody, &body)) + _, exists := body["notify"] + assert.False(t, exists, "notify must be absent unless requested") + }) +} + func TestTodosUpdateSubscriberErrorsUseSubscriberWording(t *testing.T) { // Resolution failures for --notify-on-completion must talk about // completion subscribers, not assignees. From d55eedda53c758fcd97de2aff8cec2907b4bb955 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 19 Jul 2026 11:13:41 -0700 Subject: [PATCH 3/3] Normalize base URL before OAuth discovery The bumped SDK binds the discovered issuer to the requested base URL code-point exact (RFC 8414), so a configured BaseURL with a trailing slash (https://host/) mismatched an issuer of https://host and incorrectly fell back to Launchpad. Normalize with the existing config.NormalizeBaseURL before Discover, matching credentialKey and the authorization-endpoint builders. Regression test drives discoverOAuth with BaseURL srv.URL + "/" and asserts BC3 discovery succeeds without Launchpad fallback; verified to fail without the fix. --- internal/auth/auth.go | 8 ++++++-- internal/auth/auth_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index ee114bafe..0aa75856d 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -539,10 +539,14 @@ func (m *Manager) Logout() error { } func (m *Manager) discoverOAuth(ctx context.Context, log func(string)) (*oauth.Config, string, error) { + // The SDK binds the discovered issuer to this string code-point exact + // (RFC 8414), so a trailing slash in the configured base URL would + // mismatch the server's issuer and incorrectly fall back to Launchpad. + baseURL := config.NormalizeBaseURL(m.cfg.BaseURL) discoverer := oauth.NewDiscoverer(m.httpClient) - cfg, err := discoverer.Discover(ctx, m.cfg.BaseURL) + cfg, err := discoverer.Discover(ctx, baseURL) if err != nil { - log(fmt.Sprintf("warning: OAuth discovery failed for %s, using Launchpad fallback", m.cfg.BaseURL)) + log(fmt.Sprintf("warning: OAuth discovery failed for %s, using Launchpad fallback", baseURL)) // Fallback to Launchpad lpURL, lpErr := m.launchpadURL() if lpErr != nil { diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 647156980..8e3556f3e 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -437,6 +437,40 @@ func TestDiscoverOAuth_PropagatesInsecureLaunchpadError(t *testing.T) { assert.Contains(t, err.Error(), "BASECAMP_LAUNCHPAD_URL") } +// TestDiscoverOAuth_NormalizesTrailingSlash guards the issuer binding: the +// SDK binds the discovered issuer to the requested base URL code-point exact +// (RFC 8414), so a configured BaseURL with a trailing slash must be +// normalized before discovery — otherwise BC3 discovery mismatches the +// server's issuer and incorrectly falls back to Launchpad. +func TestDiscoverOAuth_NormalizesTrailingSlash(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/.well-known/oauth-authorization-server" { + http.NotFound(w, r) + return + } + w.Header().Set("Content-Type", "application/json") + base := "http://" + r.Host + fmt.Fprintf(w, `{ + "issuer": "%s", + "authorization_endpoint": "%s/authorize", + "token_endpoint": "%s/token" + }`, base, base, base) + })) + defer srv.Close() + + cfg := config.Default() + cfg.BaseURL = srv.URL + "/" + + m := &Manager{cfg: cfg, httpClient: srv.Client()} + + noop := func(string) {} + oauthCfg, oauthType, err := m.discoverOAuth(context.Background(), noop) + require.NoError(t, err) + assert.Equal(t, "bc3", oauthType, "trailing-slash BaseURL must not fall back to Launchpad") + require.NotNil(t, oauthCfg.AuthorizationEndpoint) + assert.Equal(t, srv.URL+"/authorize", *oauthCfg.AuthorizationEndpoint) +} + func TestResolveOAuthCallback(t *testing.T) { tests := []struct { name string