Fix search timeout, sort vocabulary, and metadata handling - #557
Conversation
Review carefully before merging. Consider a major version bump. |
There was a problem hiding this comment.
Pull request overview
This PR fixes basecamp search behavior to avoid unbounded pagination timeouts, align --sort with current API vocabulary, and handle search metadata schema drift more gracefully while the pinned SDK lacks search filter fields.
Changes:
- Cap default search results at 20 unless
--allis used; validate--limit(> 0) and forbid--all+--limit. - Normalize
--sortvalues tobest_match(relevance) orrecency, rejecting unknown values. - Treat empty search metadata as a successful response with a notice (instead of a fatal error), and update docs/tests accordingly.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| skills/basecamp/SKILL.md | Updates the skill docs/examples for capped search defaults, new sort vocabulary, and metadata caveats. |
| internal/commands/search.go | Implements bounded default limit, sort normalization, explicit --project/--in rejection, and graceful metadata handling. |
| internal/commands/search_test.go | Adds regression tests for default cap vs --all, sort mappings, --limit validation, project rejection, and metadata schema drift. |
| e2e/smoke/smoke_misc_read.bats | Updates smoke test to expect search metadata success even when metadata is empty (schema drift). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6317039 to
731f11b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 731f11bd9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
731f11b to
fbeece9
Compare
fbeece9 to
e20689e
Compare
e20689e to
3aa3516
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
internal/commands/search.go:447
- The metadata summary uses unconditionally plural "types", which produces ungrammatical output for singular counts (e.g., "1 recording types"). Consider pluralizing based on the count so the summary reads correctly for 0/1/N.
recordingOptions := countSelectableTypes(metadata.RecordingSearchTypes)
fileOptions := countSelectableTypes(metadata.FileSearchTypes)
summary := fmt.Sprintf("Search filters: %d recording types, %d file types", recordingOptions, fileOptions)
3aa3516 to
9807123
Compare
9807123 to
c386d62
Compare
Bound the default search result set and wire the full filter surface into basecamp search. #470: --limit defaulted to 0, which the SDK treats as 'fetch every page' and follows Link-header pagination unbounded, hanging 90s+ on a broad query. Apply a default cap of 20 (--all restores unbounded; --all + --limit is a usage error). #546: sort vocabulary and the previously-ignored project/type/metadata filters. --sort normalizes to relevance (best_match) or recency. New filter flags: --project/--in scope to a project (resolved to bucket_ids[]) --type filter by content type (todo, card, ping, chat, ...) --creator filter by creator (name, email, ID, or me) --since bound to a time range (BC5-only) --file-type filter attachments (image, audio, video, pdf) --exclude-chat drop chat/campfire results Search uses a different type vocabulary than recordings (upload -> Attachment, ping -> Circle, check-in -> Question, event -> Schedule::Entry, folder -> Vault, chat -> Chat::Transcript, forward -> Inbox::Forward), so --type resolves through a dedicated table and rejects unknown values rather than passing them through: BC3 silently discards an unrecognized type and returns unfiltered results. --file-type normalizes casing to BC3's case-sensitive Blob::TYPES membership so 'image' does not silently disable the filter. Each filter is sent in both the plural (BucketIds/TypeNames/CreatorIds) and deprecated singular forms so BC5 honors the plural while older clients fall back to the singular. search metadata now presents the real recording/file search types and default labels; the summary counts only selectable (non-default) options — the key:null 'Everything'/'All files' default is excluded. Empty metadata is a graceful success, not an error. The SearchOptions/SearchMetadata types come from the SDK. Bump the pin to the current SDK tip (a98f52fd3ddd), which hardens the generated array query params (bucket_ids[]/type_names[]/creator_ids[]) to omit empty slices on the wire (basecamp-sdk #407/#410); the public SearchService wrapper the CLI uses was already safe, so this is defense in depth. No new SDK service methods, no CLI surface change. Update .surface, SKILL.md, API-COVERAGE.md, and the search-metadata smoke test.
c386d62 to
26d0419
Compare
Fixes #470
Fixes #546
Problem
basecamp search "<query>" --jsonhangs indefinitely (tested to 90s), no results, no error.--projectis silently ignored, andbasecamp search metadatafails / reports the wrong shape.Root causes
Timeout (basecamp search timing out #470).
--limitdefaults to0; the SDK treats0as "fetch ALL" and follows Link-header pagination across every page unbounded (followPagination). A broad or blank search paginates for 90s+. Affects all output formats."Identical unrelated results" (search ignores query semantics and project filter #546) — fixed upstream. The primary cause was old SDK GET requests carrying
Content-Type: application/json, which made BC3 discardqand every filter (SDK Add lineup list command #324, closed; BC3 #12361). The current SDK sets that header only when the request has a body, so bodyless GETs like search no longer trip it, and BC3 defaults blanksorttobest_match. The residual CLI defect was stale sort docs/vocabulary; unbounded pagination (cause 1) amplified any broad query into the timeout.--project/filters ignored (search ignores query semantics and project filter #546). The global--project/--inflag was accepted but never read;--type/--creator/--since/--file-typedid not exist. The SDK'sSearchOptionsnow exposes the filter fields.search metadatashape (search ignores query semantics and project filter #546)./searches/metadata.jsonreturns recording/file search types (askey/valuepairs) plusdefault_*filter labels; the CLI needed to present the real fields and count them correctly.SDK
The
SearchOptions.{BucketIds,TypeNames,CreatorIds,FileType,ExcludeChat,Since}(plus deprecated singulars) and the realSearchMetadata(RecordingSearchTypes/FileSearchTypesaskey/valuepairs plus fiveDefault*Labelfields) ship in the SDK. This PR bumps the pin to the current SDK tip (a98f52fd3ddd), which additionally hardens the generated array query params (bucket_ids[]/type_names[]/creator_ids[]) to omit empty slices on the wire (basecamp-sdk #407/#410) — the publicSearchServicewrapper the CLI uses was already safe, so that guard is defense in depth. The wrapper owns bracket wire-encoding, so the CLI just sets the option fields. No new SDK service methods and no CLI surface change came with the bump.Changes
--allnor an explicit--limitis given.--all→ unbounded; explicit--limitmust be> 0;--all+--limittogether is a usage error. Regression tests prove a bare search returns 20 in one HTTP request while--alltraverses every page.--sortnormalizes torelevance(→best_match, pinned explicitly) orrecency(with deprecatedcreated_at/updated_ataliases mapped in); unknown values are rejected.--project/--in— resolved viaapp.Names.ResolveProject(ID or name) tobucket_ids[]. Only an explicit flag scopes; ambient config project stays ignored (search is account-wide).--type— friendly aliases and canonical Keys through a dedicated table (todo,card→Kanban::Card,file/upload→Attachment,ping→Circle,check-in→Question,event→Schedule::Entry,folder→Vault,chat→Chat::Transcript,forward→Inbox::Forward,client→Client::Correspondence, …). Search uses a different type vocabulary than recordings, so this does not reusenormalizeRecordingType. Unknown values are rejected: BC3 silently discards an unrecognized type and returns unfiltered results.--creator— resolved viaapp.Names.ResolvePerson(name/email/ID/me).--file-type— case-normalized to BC3's case-sensitiveBlob::TYPES(image→Image, …), soimagedoes not silently disable the filter. Unknown values rejected.--since—last_7_days/last_30_days/last_90_days/last_12_months/forever(BC5-only; no BC4 equivalent). Unknown values rejected.--exclude-chat.BucketIds/TypeNames/CreatorIds) and deprecated singular forms, so BC5 honors the plural while older clients fall back to the singular.search metadatapresents the real recording/file search types and default labels; the summary counts only selectable (non-default) options — thekey:null"Everything"/"All files" default is excluded. Empty metadata is a graceful success, not an error..surface,skills/basecamp/SKILL.md,API-COVERAGE.md, and the search-metadata smoke test updated.Verification
bin/cifully green (fmt, vet, lint, unit + e2e BATS, naming, surface snapshot, skill-drift, SDK provenance, go mod tidy).grep 'replace' go.mod→ none. Tests cover project/creator resolution (bucket_ids[]+bucket_id,creator_ids[]+creator_id), the--type/--file-type/--sincevocabularies (dual plural+singular wire forms, invalid values rejected before any request), and the real metadata shape (non-default counts).