diff --git a/e2e/smoke/smoke_misc_read.bats b/e2e/smoke/smoke_misc_read.bats index 998f0989c..2ede67870 100644 --- a/e2e/smoke/smoke_misc_read.bats +++ b/e2e/smoke/smoke_misc_read.bats @@ -28,10 +28,8 @@ setup_file() { @test "search metadata returns metadata" { run_smoke basecamp search metadata --json - # Search metadata requires projects with search enabled - if [[ "$status" -ne 0 ]]; then - mark_unverifiable "Search metadata not available" - return 0 - fi + # Empty metadata is deliberate success now (SDK schema drift, not an error); + # only genuine transport/API failures should fail here. + assert_success assert_json_value '.ok' 'true' } diff --git a/internal/commands/search.go b/internal/commands/search.go index 9c1cb478a..83d8cbe96 100644 --- a/internal/commands/search.go +++ b/internal/commands/search.go @@ -13,6 +13,13 @@ import ( "github.com/basecamp/basecamp-cli/internal/output" ) +// defaultSearchLimit caps results when neither --all nor an explicit --limit is +// given. The SDK treats Limit==0 as "fetch every page" and follows Link-header +// pagination unbounded, which can hang for 90s+ on a broad query (#470); a +// bounded default keeps the common case fast while --all preserves opt-in +// exhaustive fetches. +const defaultSearchLimit = 20 + // NewSearchCmd creates the search command for full-text search. func NewSearchCmd() *cobra.Command { var sortBy string @@ -25,9 +32,10 @@ func NewSearchCmd() *cobra.Command { Long: `Search across all Basecamp content. Uses the Basecamp search API to find content matching your query. -Use 'basecamp search metadata' to see available search scopes.`, +Results are capped at 20 by default; pass --all to fetch every match. +Use 'basecamp search metadata' to inspect the search metadata the API reports.`, Example: ` basecamp search "quarterly goals" - basecamp search "bug report" --sort created_at + basecamp search "bug report" --sort recency basecamp search "design review" --limit 5 basecamp search "meeting notes" --all`, Annotations: map[string]string{"agent_notes": "Use search for keyword queries, use recordings for browsing by type/status without a search term"}, @@ -47,21 +55,49 @@ Use 'basecamp search metadata' to see available search scopes.`, query := args[0] - if all && limit > 0 { + // The global --project/--in flag is accepted but the pinned SDK's + // SearchParams exposes only q/sort — no bucket_ids[]. Reject explicit + // project scoping rather than silently returning unscoped results. + // Ambient config.ProjectID is never rejected; only an explicit flag + // signals search-scoping intent. + // Follow-up (post-SDK-bump that absorbs bucket_ids[], BC3 #12361 merged + // server-side): resolve via app.Names.ResolveProject and pass the bucket + // ID. See spec/api-gaps/search-filter-additions.md in the SDK repo. + if app.Flags.Project != "" { + return output.ErrUsageHint( + "project-scoped search is not supported yet", + "The Basecamp API now accepts bucket_ids[] (BC3 #12361), but no published SDK build exposes it. Re-run the search without --project/--in for now.", + ) + } + + sort, err := normalizeSearchSort(sortBy) + if err != nil { + return err + } + + limitChanged := cmd.Flags().Changed("limit") + if all && limitChanged { return output.ErrUsage("--all and --limit are mutually exclusive") } + effectiveLimit := defaultSearchLimit + switch { + case all: + effectiveLimit = 0 // unbounded: follow pagination to the end + case limitChanged: + if limit <= 0 { + return output.ErrUsage("--limit must be a positive number; use --all to fetch every result") + } + effectiveLimit = limit + } + if err := ensureAccount(cmd, app); err != nil { return err } - // Build search options - opts := &basecamp.SearchOptions{} - if sortBy != "" { - opts.Sort = sortBy - } - if !all && limit > 0 { - opts.Limit = limit + opts := &basecamp.SearchOptions{ + Sort: sort, + Limit: effectiveLimit, } searchResult, err := app.Account().Search().Search(cmd.Context(), query, opts) @@ -98,8 +134,8 @@ Use 'basecamp search metadata' to see available search scopes.`, }, } - cmd.Flags().StringVarP(&sortBy, "sort", "s", "", "Sort by: created_at or updated_at (default: relevance)") - cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Maximum number of results to fetch") + cmd.Flags().StringVarP(&sortBy, "sort", "s", "", "Sort order: relevance (default) or recency") + cmd.Flags().IntVarP(&limit, "limit", "n", 0, "Maximum number of results to fetch (default 20; use --all for every result)") cmd.Flags().BoolVar(&all, "all", false, "Fetch all results (no limit)") cmd.AddCommand(newSearchMetadataCmd()) @@ -107,12 +143,33 @@ Use 'basecamp search metadata' to see available search scopes.`, return cmd } +// normalizeSearchSort maps the user-facing --sort vocabulary onto the values the +// Basecamp search API accepts. Empty/relevance normalizes to best_match (BC3's +// default, pinned explicitly for deterministic output); recency and its +// deprecated created_at/updated_at aliases normalize to recency (newest-first). +// BC3 treats any non-blank, non-best_match sort as created-at descending, so +// recency works today regardless of the search-filter release. Unknown values +// are a usage error. +func normalizeSearchSort(sort string) (string, error) { + switch strings.ToLower(strings.TrimSpace(sort)) { + case "", "relevance", "best_match": + return "best_match", nil + case "recency", "newest", "created_at", "updated_at": + return "recency", nil + default: + return "", output.ErrUsage(fmt.Sprintf("invalid --sort value %q; valid values are relevance or recency", sort)) + } +} + func newSearchMetadataCmd() *cobra.Command { return &cobra.Command{ Use: "metadata", Aliases: []string{"types"}, - Short: "Show available search scopes", - Long: "Display available projects for search scope filtering.", + Short: "Show search metadata reported by the API", + Long: `Display the search metadata returned by the Basecamp API. + +Note: this SDK build surfaces only project scopes; the API also returns +recording/file search types and labels that are not yet modeled.`, RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) return runSearchMetadata(cmd, app) @@ -200,18 +257,16 @@ func runSearchMetadata(cmd *cobra.Command, app *appctx.App) error { if err != nil { return convertSDKError(err) } - - // Handle empty response - if metadata == nil || len(metadata.Projects) == 0 { - return output.ErrUsageHint( - "Search metadata not available", - "No projects available for search filtering", - ) + if metadata == nil { + metadata = &basecamp.SearchMetadata{} } - summary := fmt.Sprintf("Available projects: %d", len(metadata.Projects)) + summary := "Search metadata (limited SDK coverage)" + if len(metadata.Projects) > 0 { + summary = fmt.Sprintf("Search metadata: %d project scopes", len(metadata.Projects)) + } - return app.OK(metadata, + respOpts := []output.ResponseOption{ output.WithSummary(summary), output.WithBreadcrumbs( output.Breadcrumb{ @@ -220,5 +275,18 @@ func runSearchMetadata(cmd *cobra.Command, app *appctx.App) error { Description: "Search content", }, ), - ) + } + + // The pinned SDK models only `projects`, but /searches/metadata.json now also + // returns recording/file search types and labels. An empty projects list is + // SDK schema drift, not an empty account — surface a notice rather than the + // former fatal "Search metadata not available" error (#546). Reserve errors + // for genuine transport/API failures (handled by convertSDKError above). + if len(metadata.Projects) == 0 { + respOpts = append(respOpts, output.WithNotice( + "This SDK build surfaces only project scopes; the Basecamp API also returns recording/file search types and labels that are not yet modeled.", + )) + } + + return app.OK(metadata, respOpts...) } diff --git a/internal/commands/search_test.go b/internal/commands/search_test.go index 85c4b89d2..87c7f0313 100644 --- a/internal/commands/search_test.go +++ b/internal/commands/search_test.go @@ -8,6 +8,8 @@ import ( "fmt" "io" "net/http" + "net/url" + "strconv" "strings" "testing" @@ -24,11 +26,24 @@ import ( "github.com/basecamp/basecamp-cli/internal/output" ) -// searchTransport serves mock search API responses with configurable -// result count and total count (X-Total-Count header). +// searchTransport serves mock search API responses. +// +// In single-page mode (perPage == 0) it returns resultCount results with no +// pagination Link — the shape most tests need. In paginated mode (perPage > 0) +// it serves ?page= pages of perPage results drawn from a pool of `pool` total +// results, advertising a `Link: …; rel="next"` header whenever more results +// remain. Optional pointers capture the number of HTTP requests served and the +// last request's query parameters so tests can assert pagination short-circuits +// and query wiring. type searchTransport struct { resultCount int totalCount int + + perPage int // page size when paginating; 0 = single page of resultCount + pool int // total results available across pages (paginated mode) + + requests *int // captures number of HTTP requests served + lastParams *url.Values // captures the last request's query params } func (s searchTransport) RoundTrip(req *http.Request) (*http.Response, error) { @@ -39,9 +54,39 @@ func (s searchTransport) RoundTrip(req *http.Request) (*http.Response, error) { return nil, errors.New("unexpected request: " + req.URL.Path) } - // Build N search results + query := req.URL.Query() + if s.requests != nil { + *s.requests++ + } + if s.lastParams != nil { + *s.lastParams = query + } + + // Determine which ids this page carries. + start, end := 0, s.resultCount + if s.perPage > 0 { + page := 1 + if p := query.Get("page"); p != "" { + if n, err := strconv.Atoi(p); err == nil && n > 0 { + page = n + } + } + start = (page - 1) * s.perPage + end = start + s.perPage + if end > s.pool { + end = s.pool + } + if end < s.pool { + next := *req.URL + q := next.Query() + q.Set("page", strconv.Itoa(page+1)) + next.RawQuery = q.Encode() + header.Set("Link", fmt.Sprintf("<%s>; rel=\"next\"", next.String())) + } + } + var results []map[string]any - for i := range s.resultCount { + for i := start; i < end; i++ { results = append(results, map[string]any{ "id": i + 1, "status": "active", @@ -71,6 +116,45 @@ func (s searchTransport) RoundTrip(req *http.Request) (*http.Response, error) { }, nil } +// searchMetadataTransport serves /searches/metadata.json with the current BC3 +// response shape: recording/file search types as key/value pairs plus the +// default_* filter labels, but no `projects` key — so the pinned SDK +// deserializes an empty Projects slice. See bc3 +// app/views/api/searches/metadata/index.json.jbuilder. +type searchMetadataTransport struct{} + +func (searchMetadataTransport) RoundTrip(req *http.Request) (*http.Response, error) { + header := make(http.Header) + header.Set("Content-Type", "application/json") + + if !strings.Contains(req.URL.Path, "/searches/metadata.json") { + return nil, errors.New("unexpected request: " + req.URL.Path) + } + + body := []byte(`{ + "recording_search_types": [ + {"key": null, "value": "Everything"}, + {"key": "Todo", "value": "To-dos"} + ], + "file_search_types": [ + {"key": null, "value": "All files"}, + {"key": "image", "value": "images"} + ], + "default_creator_label": "Anyone", + "default_bucket_label": "All projects", + "default_circle_label": "All pings", + "default_file_type_label": "All files", + "default_type_label": "Everything" + }`) + + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(bytes.NewReader(body)), + Header: header, + Request: req, + }, nil +} + func setupSearchTestApp(t *testing.T, transport http.RoundTripper) (*appctx.App, *bytes.Buffer) { t.Helper() t.Setenv("BASECAMP_NO_KEYRING", "1") @@ -144,3 +228,160 @@ func TestSearchAllAndLimitMutuallyExclusive(t *testing.T) { require.True(t, errors.As(err, &e), "expected *output.Error, got %T: %v", err, err) assert.Contains(t, e.Message, "--all and --limit are mutually exclusive") } + +// TestSearchBoundedDefault is the regression for #470: a bare search must apply +// the default cap and short-circuit pagination in a single request, even when +// the first page already advertises a next Link. +func TestSearchBoundedDefault(t *testing.T) { + var requests int + // Page 1 carries 25 results and a next Link; pool of 50 guarantees the Link + // is present so we prove the default cap stops us, not an exhausted pool. + app, buf := setupSearchTestApp(t, searchTransport{ + perPage: 25, + pool: 50, + totalCount: 50, + requests: &requests, + }) + + cmd := NewSearchCmd() + require.NoError(t, executeSearchCommand(cmd, app, "query")) + + var envelope output.Response + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + + results, ok := envelope.Data.([]any) + require.True(t, ok, "expected results array, got %T", envelope.Data) + assert.Len(t, results, defaultSearchLimit) + assert.Equal(t, 1, requests, "default cap must short-circuit pagination in one request") +} + +// TestSearchAllTraversesPages proves --all bypasses the default cap and follows +// pagination to completion. +func TestSearchAllTraversesPages(t *testing.T) { + var requests int + app, buf := setupSearchTestApp(t, searchTransport{ + perPage: 20, + pool: 25, + totalCount: 25, + requests: &requests, + }) + + cmd := NewSearchCmd() + require.NoError(t, executeSearchCommand(cmd, app, "query", "--all")) + + var envelope output.Response + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + + results, ok := envelope.Data.([]any) + require.True(t, ok, "expected results array, got %T", envelope.Data) + assert.Len(t, results, 25) + assert.Equal(t, 2, requests, "--all must traverse every page") +} + +func TestSearchLimitMustBePositive(t *testing.T) { + for _, value := range []string{"0", "-1"} { + app, _ := setupSearchTestApp(t, todosNoNetworkTransport{}) + cmd := NewSearchCmd() + err := executeSearchCommand(cmd, app, "query", "--limit", value) + require.Error(t, err, "--limit %s should be rejected", value) + + var e *output.Error + require.True(t, errors.As(err, &e), "expected *output.Error, got %T: %v", err, err) + assert.Contains(t, e.Message, "must be a positive number") + } +} + +// TestSearchDefaultQueryAndSort proves a bare search sends q= and the +// pinned best_match default sort. +func TestSearchDefaultQueryAndSort(t *testing.T) { + var params url.Values + app, _ := setupSearchTestApp(t, searchTransport{ + resultCount: 3, + totalCount: 3, + lastParams: ¶ms, + }) + + cmd := NewSearchCmd() + require.NoError(t, executeSearchCommand(cmd, app, "meeting notes")) + + assert.Equal(t, "meeting notes", params.Get("q")) + assert.Equal(t, "best_match", params.Get("sort")) +} + +func TestSearchSortMappings(t *testing.T) { + for input, want := range map[string]string{ + "relevance": "best_match", + "best_match": "best_match", + "recency": "recency", + "newest": "recency", + "created_at": "recency", + "updated_at": "recency", + } { + var params url.Values + app, _ := setupSearchTestApp(t, searchTransport{ + resultCount: 1, + totalCount: 1, + lastParams: ¶ms, + }) + + cmd := NewSearchCmd() + require.NoError(t, executeSearchCommand(cmd, app, "query", "--sort", input), "sort %q", input) + assert.Equal(t, want, params.Get("sort"), "sort %q", input) + } +} + +func TestSearchInvalidSortRejected(t *testing.T) { + app, _ := setupSearchTestApp(t, todosNoNetworkTransport{}) + + cmd := NewSearchCmd() + err := executeSearchCommand(cmd, app, "query", "--sort", "bogus") + require.Error(t, err) + + var e *output.Error + require.True(t, errors.As(err, &e), "expected *output.Error, got %T: %v", err, err) + assert.Contains(t, e.Message, "invalid --sort value") +} + +// TestSearchProjectRejected proves an explicit --project (surfaced via +// app.Flags.Project, since the harness can't parse root globals) is rejected +// with the SDK-gap message, while an ambient config project is never rejected. +func TestSearchProjectRejected(t *testing.T) { + app, _ := setupSearchTestApp(t, todosNoNetworkTransport{}) + app.Flags.Project = "123" + + cmd := NewSearchCmd() + err := executeSearchCommand(cmd, app, "query") + require.Error(t, err) + + var e *output.Error + require.True(t, errors.As(err, &e), "expected *output.Error, got %T: %v", err, err) + assert.Contains(t, e.Message, "project-scoped search is not supported yet") +} + +func TestSearchAmbientProjectNotRejected(t *testing.T) { + app, buf := setupSearchTestApp(t, searchTransport{resultCount: 1, totalCount: 1}) + app.Config.ProjectID = "456" // ambient, not an explicit flag + require.Empty(t, app.Flags.Project) + + cmd := NewSearchCmd() + require.NoError(t, executeSearchCommand(cmd, app, "query")) + + var envelope output.Response + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + assert.True(t, envelope.OK) +} + +// TestSearchMetadataEmptyIsGraceful proves empty metadata (SDK schema drift) is +// a successful envelope with a notice, not the former fatal error (#546). +func TestSearchMetadataEmptyIsGraceful(t *testing.T) { + app, buf := setupSearchTestApp(t, searchMetadataTransport{}) + + cmd := NewSearchCmd() + require.NoError(t, executeSearchCommand(cmd, app, "metadata")) + + var envelope output.Response + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + assert.True(t, envelope.OK) + assert.NotContains(t, envelope.Notice, "Search metadata not available") + assert.Contains(t, envelope.Notice, "not yet modeled") +} diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 36f3e696d..48337d725 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -893,9 +893,9 @@ basecamp people remove --project # Remove from project ### Search ```bash -basecamp search "query" --json # Full-text search -basecamp search "query" --sort updated_at --limit 20 -basecamp search metadata --json # Available search scopes +basecamp search "query" --json # Full-text search (capped at 20; --all for every match) +basecamp search "query" --sort recency --limit 20 +basecamp search metadata --json # Search metadata reported by the API (limited SDK coverage) ``` ### Generic Show