From 0610ccf17fd98d7616364d73717d92722e89bf24 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Wed, 28 Oct 2026 15:32:35 -0400 Subject: [PATCH 1/5] Bump basecamp-sdk to v0.32.0, for move and copy that wait Syncs the vendored MCP model to the tag (no model changes) and refreshes the Nix vendorHash. --- go.mod | 2 +- go.sum | 4 ++-- internal/mcpserver/model/PROVENANCE.json | 4 ++-- internal/version/sdk-provenance.json | 6 +++--- nix/package.nix | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 3496ffbd4..6aefec586 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,7 @@ require ( charm.land/bubbles/v2 v2.2.1 charm.land/bubbletea/v2 v2.1.0 charm.land/lipgloss/v2 v2.0.6 - github.com/basecamp/basecamp-sdk/go v0.31.0 + github.com/basecamp/basecamp-sdk/go v0.32.0 github.com/basecamp/cli v0.2.2-0.20261023171439-c9bce8206df9 github.com/basecamp/mcp v0.0.0-20260828100356-2d6f44b51e9d github.com/basecamp/surfguard/go v0.1.0 diff --git a/go.sum b/go.sum index 26d423ea9..ec9182111 100644 --- a/go.sum +++ b/go.sum @@ -89,8 +89,8 @@ github.com/aymerick/douceur v0.2.0 h1:Mv+mAeH1Q+n9Fr+oyamOlAkUNPWPlA8PPGR0QAaYuP github.com/aymerick/douceur v0.2.0/go.mod h1:wlT5vV2O3h55X9m7iVYN0TBM0NH/MmbLnd30/FjWUq4= github.com/basecamp/actioncable-go v1.2.0 h1:t0MxlfMTcmaGXZ5ztgKRs7ZiY1MwXtBRwSSSAh1ofVg= github.com/basecamp/actioncable-go v1.2.0/go.mod h1:ezaV5z1GXQAsqyejqTs6wCFl2D8Wj+COLQkHc/kwoRs= -github.com/basecamp/basecamp-sdk/go v0.31.0 h1:+gAcD0hMiEt4VWP4VOSOr/U3crjVa6dfT/NjmPN2JDo= -github.com/basecamp/basecamp-sdk/go v0.31.0/go.mod h1:mUm/MQGMyg1E1cddyxdG7tQHiq0NsYee2zUeAUT5VjU= +github.com/basecamp/basecamp-sdk/go v0.32.0 h1:HWRxJGhlaxzh7f/2+cxax664dN5BgjfM+Our3rEWuZU= +github.com/basecamp/basecamp-sdk/go v0.32.0/go.mod h1:mUm/MQGMyg1E1cddyxdG7tQHiq0NsYee2zUeAUT5VjU= github.com/basecamp/cli v0.2.2-0.20261023171439-c9bce8206df9 h1:3NfGs+zx23Tp6cf+97+UewrJHlzZMLzmvEXOmOJyQAk= github.com/basecamp/cli v0.2.2-0.20261023171439-c9bce8206df9/go.mod h1:iTBTaWvsPEFIcZfkxQHEfISyJ6sZ7036K6bNx0RY3EE= github.com/basecamp/mcp v0.0.0-20260828100356-2d6f44b51e9d h1:zEQVGq1x1nhKMZ2TudFAcSJ32CHT8richI1vQakIKz4= diff --git a/internal/mcpserver/model/PROVENANCE.json b/internal/mcpserver/model/PROVENANCE.json index f1e5d3018..b58d4348f 100644 --- a/internal/mcpserver/model/PROVENANCE.json +++ b/internal/mcpserver/model/PROVENANCE.json @@ -1,7 +1,7 @@ { "source": "github.com/basecamp/basecamp-sdk", - "commit": "b2e9356f386d7ca5fc87d0ba89ad197ffff9af5c", - "ref": "v0.31.0", + "commit": "13227457da654997d78d3549ab7a0191825fda97", + "ref": "go/v0.32.0", "files": ["behavior-model.json", "openapi.json"], "synced_by": "scripts/sync-mcp-model.sh", "patches": "binary-upload operations dropped because the toolkit refuses their non-JSON bodies (EXCLUDED_OPERATIONS) and the stream-ticket mint dropped by policy (POLICY_EXCLUDED_OPERATIONS); no tag patches applied (PATCHED_TAGS is empty — the export tags every operation) — see the sync script" diff --git a/internal/version/sdk-provenance.json b/internal/version/sdk-provenance.json index bb75db60d..e81547c3a 100644 --- a/internal/version/sdk-provenance.json +++ b/internal/version/sdk-provenance.json @@ -1,9 +1,9 @@ { "sdk": { "module": "github.com/basecamp/basecamp-sdk/go", - "version": "v0.31.0", - "revision": "b2e9356f386d", - "updated_at": "2026-10-28T04:21:17Z" + "version": "v0.32.0", + "revision": "13227457da65", + "updated_at": "2026-10-28T19:25:31Z" }, "api": { "repo": "basecamp/bc3", diff --git a/nix/package.nix b/nix/package.nix index ad5912ef5..4ed9c71bc 100644 --- a/nix/package.nix +++ b/nix/package.nix @@ -8,7 +8,7 @@ buildGoModule.override { go = go_1_26; } (finalAttrs: { src = lib.cleanSource ./..; # To update: set to lib.fakeHash, run `nix build`, use the hash from the error. - vendorHash = "sha256-K/+fC4LChVQRYXOG6xIt/2j4KPnh0/d7nuqAuekTzGw="; + vendorHash = "sha256-HFrhx9UJa/emaHZf4xcoQVglrTUepJRZDRofkZI5+No="; subPackages = [ "cmd/basecamp" ]; From b26504c4e529863895162700a4d347202356a8f8 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Wed, 28 Oct 2026 15:20:42 -0400 Subject: [PATCH 2/5] Serve move and copy that wait for Basecamp in basecamp mcp basecamp_recordings move and copy take a recording and a destination (a project, or a container plus an optional folder), start the filing once through the SDK's MoveAndWait or CopyAndWait, wait for Basecamp to finish, and answer with the new recording read back at its new id. A failure says how far it got and whether calling again is safe. They replace the model's start-only move_recording and copy_recording, which answered with a filing to poll. get_filing moves into basecamp_recordings beside them, as the check for a filing that was still running when the wait ended. --- README.md | 7 +- internal/mcpserver/catalog_test.go | 13 +- internal/mcpserver/composite.go | 43 +- internal/mcpserver/dispatch.go | 21 +- internal/mcpserver/filing.go | 491 ++++++++++++++++++ internal/mcpserver/filing_test.go | 465 +++++++++++++++++ .../mcpserver/testdata/catalog_snapshot.txt | 8 +- 7 files changed, 1023 insertions(+), 25 deletions(-) create mode 100644 internal/mcpserver/filing.go create mode 100644 internal/mcpserver/filing_test.go diff --git a/README.md b/README.md index d82fa9130..1910bde9d 100644 --- a/README.md +++ b/README.md @@ -461,7 +461,7 @@ Sixteen domain tools (`basecamp_projects`, `basecamp_todos`, `basecamp_cards`, account. Each tool takes `{"action": "...", "params": {...}}` and serves per-action schemas through its `describe` action. -Two composite helpers ride on that surface, shared with basecamp-mcp-server +Composite helpers ride on that surface, shared with basecamp-mcp-server through basecamp-sdk so both servers answer the same way. `basecamp_recordings summarize` resolves the pointer an account event feed row carries — bucket id, recording id, and the event or recording type — @@ -471,7 +471,10 @@ it carries no method and no path, where every model operation carries both, and its summary opens with "Synthetic:". `basecamp_messages create_comment` is an ordinary endpoint with a synthetic parameter: `mentions`, a list of person ids expanded client-side into the attachment markup Basecamp reads as -a mention. +a mention. `basecamp_recordings move` and `copy` file a recording into +another container, in its project or another one, as the web's Move… and +Copy… dialogs do: they wait for Basecamp to finish and return the new +recording, and a failure says whether trying again is safe. ```bash basecamp mcp --read-only # serve only read-only actions diff --git a/internal/mcpserver/catalog_test.go b/internal/mcpserver/catalog_test.go index f1f2975ad..b3d9f8926 100644 --- a/internal/mcpserver/catalog_test.go +++ b/internal/mcpserver/catalog_test.go @@ -51,7 +51,9 @@ func TestCatalogClaimsEveryTag(t *testing.T) { // upload). The stream-ticket mint could ride it and is dropped anyway: its // result is a replayable bearer and a URL embedding it, the dispatcher // returns a result verbatim into a model transcript with no reveal opt-in, -// and nothing on this surface could open the WebSocket it is for. +// and nothing on this surface could open the WebSocket it is for. The +// start-only move and copy are dropped at load instead, where the waiting +// composites take their place. func TestCatalogExcludesUnservedOperations(t *testing.T) { cat := loadForTest(t) excluded := map[string]bool{ @@ -61,6 +63,9 @@ func TestCatalogExcludesUnservedOperations(t *testing.T) { "UpdateProjectWallpaper": true, "UpdateProjectLogo": true, "CreateStreamTicket": true, + // Replaced by the waiting recordings.move and recordings.copy. + "MoveRecording": true, + "CopyRecording": true, } model, composite := 0, 0 for _, d := range cat.Domains { @@ -73,8 +78,8 @@ func TestCatalogExcludesUnservedOperations(t *testing.T) { assert.False(t, excluded[op.ID], "operation %q should be excluded from the vendored model", op.ID) } } - assert.Equal(t, 286, model, "served model operation count") - assert.Equal(t, 1, composite, "served composite operation count") + assert.Equal(t, 284, model, "served model operation count") + assert.Equal(t, 3, composite, "served composite operation count") } // isComposite reports the SDK compositions this package serves itself: no @@ -105,7 +110,7 @@ func TestCatalogMarksCompositesSynthetic(t *testing.T) { assert.NotEmpty(t, op.Doc, "composite %q must document what it composes", op.ID) } } - assert.Equal(t, map[string]bool{"recordings.summarize": true}, seen) + assert.Equal(t, map[string]bool{"recordings.summarize": true, "recordings.move": true, "recordings.copy": true}, seen) } // TestCatalogDeclaresMentionsOnCreateComment pins the other half of the diff --git a/internal/mcpserver/composite.go b/internal/mcpserver/composite.go index 8d1a5c47e..62b62bf73 100644 --- a/internal/mcpserver/composite.go +++ b/internal/mcpserver/composite.go @@ -51,7 +51,7 @@ const ( mentionsParam = "mentions" syntheticSummaryTag = "Synthetic: " recordingsDomainTag = "Recordings" - recordingsDomainDesc = "Recordings: the compact projection of one recording, resolved from the pointer an account event feed row carries." + recordingsDomainDesc = "Recordings: the compact projection of one recording, resolved from the pointer an account event feed row carries; and moving or copying a recording into another container or project, waiting for Basecamp to finish and returning the new recording." ) // maxExactJSONInteger is the largest integer a JSON number carries without @@ -84,6 +84,8 @@ type compositeHandler func(ctx context.Context, api API, op *catalog.Operation, // dispatcher holds when a call arrives. var compositeHandlers = map[string]compositeHandler{ recordingsDomainKey + "." + summarizeAction: handleSummarize, + recordingsDomainKey + "." + moveAction: handleFiling(basecamp.FilingMove), + recordingsDomainKey + "." + copyAction: handleFiling(basecamp.FilingCopy), } // recordingsDomain is the hand-built domain carrying the composite reads. @@ -96,6 +98,8 @@ func recordingsDomain() *catalog.Domain { Tool: recordingsToolName, Blurb: recordingsDomainDesc, Operations: []*catalog.Operation{ + filingOperation(basecamp.FilingCopy), + filingOperation(basecamp.FilingMove), { ID: "SummarizeRecording", Action: summarizeAction, @@ -131,7 +135,8 @@ func recordingsDomain() *catalog.Domain { } // installComposites adds the composite surface to a loaded catalog: the -// recordings domain, and the mentions parameter on messages.create_comment. +// recordings domain, which takes over the model's move, copy and filing +// operations, and the mentions parameter on messages.create_comment. // It runs after rescopeToAccount, which walks model paths the composite // operations deliberately do not have. func installComposites(cat *catalog.Catalog) error { @@ -140,7 +145,11 @@ func installComposites(cat *catalog.Catalog) error { return fmt.Errorf("the model already serves a %q domain; the composite domain would shadow it", recordingsDomainKey) } } - cat.Domains = append(cat.Domains, recordingsDomain()) + recordings := recordingsDomain() + if err := installFilingOperations(cat, recordings); err != nil { + return err + } + cat.Domains = append(cat.Domains, recordings) return installMentionsParam(cat) } @@ -208,16 +217,8 @@ func handleSummarize(ctx context.Context, api API, op *catalog.Operation, params func summarizeRef(op *catalog.Operation, params map[string]any) (basecamp.RecordingRef, error) { var ref basecamp.RecordingRef - // What is accepted is what is advertised: the same body schema describe - // serves, read here rather than restated, so the two cannot drift. - known, _ := op.Body["properties"].(map[string]any) - if len(known) == 0 { - return ref, fmt.Errorf("internal error: action %q declares no parameters", op.Action) - } - for name := range params { - if _, ok := known[name]; !ok { - return ref, fmt.Errorf("unknown parameter %q for action %q (describe the action for its schema)", name, op.Action) - } + if err := refuseUnknownParams(op, params); err != nil { + return ref, err } var err error @@ -239,6 +240,22 @@ func summarizeRef(op *catalog.Operation, params map[string]any) (basecamp.Record return ref, nil } +// refuseUnknownParams holds a composite's parameters to what it advertises: +// the same body schema describe serves, read here rather than restated, so +// the two cannot drift. +func refuseUnknownParams(op *catalog.Operation, params map[string]any) error { + known, _ := op.Body["properties"].(map[string]any) + if len(known) == 0 { + return fmt.Errorf("internal error: action %q declares no parameters", op.Action) + } + for name := range params { + if _, ok := known[name]; !ok { + return fmt.Errorf("unknown parameter %q for action %q (describe the action for its schema)", name, op.Action) + } + } + return nil +} + // errNoTypeGiven is a pointer with neither type. The SDK reaches the same // verdict for a pointer it cannot route, so this wraps that sentinel and is // reported under the same identity: a caller branching on the error's type diff --git a/internal/mcpserver/dispatch.go b/internal/mcpserver/dispatch.go index 3383ec754..06e0c755f 100644 --- a/internal/mcpserver/dispatch.go +++ b/internal/mcpserver/dispatch.go @@ -25,8 +25,8 @@ import ( // // The four verbs serve every model operation but the feed's two poll lanes. // The recordings and comments services serve the composite actions (see -// composite.go), which are SDK compositions rather than single requests and so -// cannot be assembled from a method and a path. The event feed service serves +// composite.go and filing.go), which are SDK compositions rather than single +// requests and so cannot be assembled from a method and a path. The event feed service serves // poll_events and poll_inbox, whose refusals carry the data a consumer resumes // from and whose bodies the verbs above throw away (see feed.go). // @@ -44,6 +44,23 @@ type API interface { Recordings() *basecamp.RecordingsService Comments() *basecamp.CommentsService EventFeed() *basecamp.EventFeedService + + // The typed reads and the project dock that move and copy resolve a + // recording and its destination through (see filing.go). + Projects() *basecamp.ProjectsService + Cards() *basecamp.CardsService + CardColumns() *basecamp.CardColumnsService + Messages() *basecamp.MessagesService + Documents() *basecamp.DocumentsService + Uploads() *basecamp.UploadsService + CloudFiles() *basecamp.CloudFilesService + GoogleDocuments() *basecamp.GoogleDocumentsService + Vaults() *basecamp.VaultsService + Schedules() *basecamp.SchedulesService + Checkins() *basecamp.CheckinsService + Forwards() *basecamp.ForwardsService + Todolists() *basecamp.TodolistsService + Todos() *basecamp.TodosService } // The CLI hands its account-scoped client straight to New. diff --git a/internal/mcpserver/filing.go b/internal/mcpserver/filing.go new file mode 100644 index 000000000..f6b27b9b1 --- /dev/null +++ b/internal/mcpserver/filing.go @@ -0,0 +1,491 @@ +package mcpserver + +import ( + "bytes" + "cmp" + "context" + "encoding/json" + "errors" + "fmt" + "maps" + "slices" + "strings" + + "github.com/modelcontextprotocol/go-sdk/mcp" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/basecamp/mcp/catalog" + "github.com/basecamp/mcp/gateway" +) + +// Filing actions: recordings.move and recordings.copy file a recording into +// another container, in its project or another one, the way the web's Move… +// and Copy… dialogs do. Basecamp files in the background, so each action +// starts the filing once through RecordingsService.MoveAndWait or +// CopyAndWait, waits for Basecamp to finish it, and answers with the new +// recording read back at its new id. They replace the model's start-only +// MoveRecording and CopyRecording, which answered with a filing to poll. +// GetFiling moves into the recordings domain beside them, as the check for a +// filing that had not finished when the wait ended. +const ( + moveAction = "move" + copyAction = "copy" +) + +// filingWait bounds how long move and copy wait for Basecamp to finish: the +// SDK's default minute. Tests shorten it. +var filingWait = basecamp.FilingWaitOptions{} + +// recordingRead is one recording as its typed read returns it: the record +// to answer with, and the fields filing needs from it. +type recordingRead struct { + record json.RawMessage + id int64 + typ string + parent *basecamp.Parent +} + +type recordingReader func(ctx context.Context, api API, id int64) (recordingRead, error) + +// filingReads holds the typed read for each recordable type Basecamp moves +// and copies on its own: every type basecamp.FilingContainerFor accepts. A +// to-do list and a to-do list group are both Todolist, and a folder is a +// Vault; FilingContainerFor tells them apart by the parent the read reports. +var filingReads = map[string]recordingReader{ + "Kanban::Card": reader(API.Cards, (*basecamp.CardsService).Get), + "Kanban::Column": reader(API.CardColumns, (*basecamp.CardColumnsService).Get), + "Message": reader(API.Messages, (*basecamp.MessagesService).Get), + "Document": reader(API.Documents, (*basecamp.DocumentsService).Get), + "Upload": reader(API.Uploads, (*basecamp.UploadsService).Get), + "CloudFile": reader(API.CloudFiles, (*basecamp.CloudFilesService).Get), + "GoogleDocument": reader(API.GoogleDocuments, (*basecamp.GoogleDocumentsService).Get), + "Vault": reader(API.Vaults, (*basecamp.VaultsService).Get), + "Schedule::Entry": reader(API.Schedules, (*basecamp.SchedulesService).GetEntry), + "Question": reader(API.Checkins, (*basecamp.CheckinsService).GetQuestion), + "Inbox::Forward": reader(API.Forwards, (*basecamp.ForwardsService).Get), + "Todolist": reader(API.Todolists, (*basecamp.TodolistsService).Get), + "Todo": reader(API.Todos, (*basecamp.TodosService).Get), +} + +// reader adapts one SDK typed read into a recordingReader. The record goes +// back to the caller as the SDK serializes it, so the answer has the same +// shape as that type's own get action. +func reader[S, T any](service func(API) S, get func(S, context.Context, int64) (*T, error)) recordingReader { + return func(ctx context.Context, api API, id int64) (recordingRead, error) { + record, err := get(service(api), ctx, id) + if err != nil { + return recordingRead{}, err + } + raw, err := json.Marshal(record) + if err != nil { + return recordingRead{}, err + } + var fields struct { + ID int64 `json:"id"` + Type string `json:"type"` + Parent *basecamp.Parent `json:"parent"` + } + if err := json.Unmarshal(raw, &fields); err != nil { + return recordingRead{}, err + } + return recordingRead{record: raw, id: fields.ID, typ: fields.Type, parent: fields.Parent}, nil + } +} + +func filingTypes() []any { + types := make([]any, 0, len(filingReads)) + for _, typ := range slices.Sorted(maps.Keys(filingReads)) { + types = append(types, typ) + } + return types +} + +// containerNouns names each kind of container in messages. +var containerNouns = map[basecamp.FilingContainer]string{ + basecamp.FilingIntoCardTable: "card table", + basecamp.FilingIntoMessageBoard: "message board", + basecamp.FilingIntoVault: "Docs & Files", + basecamp.FilingIntoSchedule: "schedule", + basecamp.FilingIntoQuestionnaire: "Automatic Check-ins", + basecamp.FilingIntoInbox: "Email Forwards", + basecamp.FilingIntoTodoset: "To-dos", + basecamp.FilingIntoTodolist: "to-do list", +} + +func filingOperation(kind basecamp.FilingType) *catalog.Operation { + verb, action, id := "Move", moveAction, "MoveRecordingAndWait" + if kind == basecamp.FilingCopy { + verb, action, id = "Copy", copyAction, "CopyRecordingAndWait" + } + lower := strings.ToLower(verb) + properties := map[string]any{ + "recording_id": idSchema("The recording to " + lower + "."), + "recording_type": map[string]any{ + "type": "string", + "enum": filingTypes(), + "description": "The recording's type as Basecamp spells it (its JSON \"type\"). A to-do list group is a Todolist and a folder is a Vault.", + }, + "project_id": idSchema("The project to " + lower + " it into. Its one enabled tool of the recording's kind is the destination: its card table, message board, Docs & Files, schedule, Automatic Check-ins or Email Forwards, or for a to-do or to-do list group its one to-do list. A project with none, or several, is refused with what to pass instead. Pass project_id or container_id, not both."), + "container_id": idSchema("The container to " + lower + " it into, of the recording's kind: a card table, message board, Docs & Files tool, schedule, Automatic Check-ins, Email Forwards or To-dos tool from a project's dock, or a to-do list for a to-do or to-do list group. A folder is not a container; pass its Docs & Files here and the folder as folder_id."), + "folder_id": idSchema("A folder inside the destination Docs & Files to put it in. Only for documents, uploads, cloud files, Google documents and folders."), + } + doc := verb + "s a recording into another container, in its project or another one, as the web's " + verb + "… dialog does. " + + "Composed in basecamp-sdk (RecordingsService." + verb + "AndWait): it reads the recording, resolves the destination, starts the " + lower + " once, waits up to a minute for Basecamp to finish it, and reads back the new recording. Not backed by any single Basecamp endpoint, so this action carries no method and no path.\n\n" + + "Returns the new recording, as its type's own get action returns it, with source_id naming the recording it came from. A " + lower + " always gives the recording a new id. A card lands in the card table's Triage, or Done if it is completed.\n\n" + + "Comments, card steps, check-in answers and other recordings that go with their parent are not filed on their own, and neither is a whole tool (a project's Docs & Files or card table itself).\n\n" + + "A failure says how far it got in error.type, and error.retry_safe says whether calling this action again is safe. destination_missing, destination_disabled and destination_ambiguous filed nothing and name what to pass instead (destination_disabled carries the tool_id that basecamp_automation enable_tool turns on; destination_ambiguous lists candidates for container_id). filing_refused filed nothing. filing_unsettled was still running when the wait ended: do not " + lower + " again; check it with get_filing and the filing_id it carries. filing_failed was started and reported failed by Basecamp. filed_unread was filed under recording_id but could not be read back." + if kind == basecamp.FilingCopy { + properties["include_comments"] = map[string]any{ + "type": "boolean", + "description": "Copy the recording's comments too. Defaults to false.", + } + doc += " filing_may_have_started is a start that failed without Basecamp refusing it: a copy may exist, and copying again makes another, so look for it in the destination first." + } else { + doc += " filing_may_have_started is a start that failed without Basecamp refusing it; Basecamp refuses a repeated move, so trying again is safe." + } + return &catalog.Operation{ + ID: id, + Action: action, + Tag: recordingsDomainTag, + Summary: syntheticSummaryTag + lower + " a recording into another container or project, wait for Basecamp to finish, and return the new recording.", + Doc: doc, + BodyRequired: true, + Body: map[string]any{ + "type": "object", + "additionalProperties": false, + "required": []any{"recording_id", "recording_type"}, + "properties": properties, + }, + } +} + +// installFilingOperations replaces the model's start-only MoveRecording and +// CopyRecording with the waiting composites, and moves GetFiling into the +// recordings domain beside them. Each must be where the model put it: a +// sync that renamed or dropped one stops the load instead of serving a +// stale twin or losing the check for an unsettled filing. +func installFilingOperations(cat *catalog.Catalog, recordings *catalog.Domain) error { + for _, id := range []string{"MoveRecording", "CopyRecording"} { + if _, err := takeOperation(cat, id); err != nil { + return err + } + } + getFiling, err := takeOperation(cat, "GetFiling") + if err != nil { + return err + } + getFiling.Summary = "Get the state of a move or copy, by the filing_id a move or copy answered with when it had not finished" + recordings.Operations = append(recordings.Operations, getFiling) + slices.SortFunc(recordings.Operations, func(a, b *catalog.Operation) int { return strings.Compare(a.Action, b.Action) }) + return nil +} + +func takeOperation(cat *catalog.Catalog, id string) (*catalog.Operation, error) { + for _, d := range cat.Domains { + for i, op := range d.Operations { + if op.ID == id { + d.Operations = slices.Delete(d.Operations, i, i+1) + return op, nil + } + } + } + return nil, fmt.Errorf("the model serves no %q operation for the filing actions to take over", id) +} + +// filingRequest is one move or copy as the caller named it. +type filingRequest struct { + kind basecamp.FilingType + recordingID int64 + recordingType string + projectID int64 + containerID int64 + folderID int64 + includeComments bool +} + +func handleFiling(kind basecamp.FilingType) compositeHandler { + return func(ctx context.Context, api API, op *catalog.Operation, params map[string]any) (*mcp.CallToolResult, error) { + req, err := parseFilingRequest(kind, op, params) + if err != nil { + return gateway.ErrorResult("%v", err), nil + } + answer, err := fileRecording(ctx, api, req) + if err != nil { + if failure, ok := errors.AsType[*filingError](err); ok { + return failure.result(), nil + } + return gateway.ErrorResult("%v", err), nil + } + return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: string(answer)}}}, nil + } +} + +func parseFilingRequest(kind basecamp.FilingType, op *catalog.Operation, params map[string]any) (filingRequest, error) { + req := filingRequest{kind: kind} + if err := refuseUnknownParams(op, params); err != nil { + return req, err + } + var err error + if req.recordingID, err = requiredID(params, "recording_id"); err != nil { + return req, err + } + if req.recordingType, err = optionalString(params, "recording_type"); err != nil { + return req, err + } + if _, ok := filingReads[req.recordingType]; !ok { + return req, fmt.Errorf("parameter %q must be one of the types Basecamp moves and copies on its own (describe the action for the list), got %q", "recording_type", req.recordingType) + } + for name, field := range map[string]*int64{"project_id": &req.projectID, "container_id": &req.containerID, "folder_id": &req.folderID} { + if _, given := params[name]; given { + if *field, err = requiredID(params, name); err != nil { + return req, err + } + } + } + if (req.projectID == 0) == (req.containerID == 0) { + return req, errors.New("pass exactly one of project_id and container_id to name where it goes") + } + if raw, given := params["include_comments"]; given && raw != nil { + include, ok := raw.(bool) + if !ok { + return req, fmt.Errorf("parameter %q must be a boolean, got %T", "include_comments", raw) + } + req.includeComments = include + } + return req, nil +} + +// fileRecording reads the recording, resolves where it goes, files it once +// and waits, then reads back what Basecamp made. +func fileRecording(ctx context.Context, api API, req filingRequest) (json.RawMessage, error) { + src, err := filingReads[req.recordingType](ctx, api, req.recordingID) + if err != nil { + return nil, err + } + if src.typ != req.recordingType { + return nil, fmt.Errorf("recording %d is a %s, not a %s", req.recordingID, src.typ, req.recordingType) + } + var parentType string + if src.parent != nil { + parentType = src.parent.Type + } + container, ok := basecamp.FilingContainerFor(src.typ, parentType) + if !ok { + return nil, &filingError{ + Type: "not_filable", + Message: fmt.Sprintf("%s %d, whose parent is a %s, is not filed on its own: this action files the recordings inside a tool, not a whole tool such as a project's Docs & Files", src.typ, src.id, cmp.Or(parentType, "project")), + RetrySafe: true, + } + } + if req.folderID != 0 && !container.Folders() { + return nil, fmt.Errorf("folder_id applies only to Docs & Files items; a %s goes into a %s", src.typ, containerNouns[container]) + } + + destinationID := req.containerID + if destinationID == 0 { + if destinationID, err = destinationInProject(ctx, api, container, req.projectID); err != nil { + return nil, err + } + } + + recordings := api.Recordings() + var filing *basecamp.Filing + if req.kind == basecamp.FilingCopy { + filing, err = recordings.CopyAndWait(ctx, src.id, &basecamp.CopyRecordingRequest{ + FilingDestinationID: destinationID, + ParentID: req.folderID, + IncludeComments: req.includeComments, + }, &filingWait) + } else { + filing, err = recordings.MoveAndWait(ctx, src.id, &basecamp.MoveRecordingRequest{ + FilingDestinationID: destinationID, + ParentID: req.folderID, + }, &filingWait) + } + if err != nil { + return nil, explainFilingError(req, container, err) + } + + made := filing.DestinationRecording + read, ok := filingReads[made.Type] + if !ok { + read = filingReads[src.typ] + } + filed, err := read(ctx, api, made.ID) + if err != nil { + return nil, &filingError{ + Type: "filed_unread", + Message: fmt.Sprintf("%s %d as %s %d, but could not read it back: %v", pastTense(req.kind), src.id, made.Type, made.ID, err), + RecordingID: made.ID, + AppURL: made.AppURL, + } + } + return withSourceID(filed.record, src.id) +} + +// destinationInProject finds the one container of the kind in a project: its +// enabled dock tool of that name, or for a to-do list, its To-dos tool's one +// list. Only enabled tools count, as in the web's dialogs. +func destinationInProject(ctx context.Context, api API, container basecamp.FilingContainer, projectID int64) (int64, error) { + project, err := api.Projects().Get(ctx, projectID) + if err != nil { + return 0, err + } + toolKind := container + if container == basecamp.FilingIntoTodolist { + toolKind = basecamp.FilingIntoTodoset + } + var enabled []basecamp.DockItem + var disabled *basecamp.DockItem + for _, tool := range project.Dock { + switch { + case tool.Name != string(toolKind): + case tool.Enabled: + enabled = append(enabled, tool) + case disabled == nil: + disabled = &tool + } + } + noun := containerNouns[toolKind] + switch { + case len(enabled) == 0 && disabled != nil: + return 0, &filingError{ + Type: "destination_disabled", + Message: fmt.Sprintf("Project %s's %s (%s) is turned off, so nothing can be moved or copied into it. Turn it on with basecamp_automation enable_tool and toolId %d, or pick another project", project.Name, noun, disabled.Title, disabled.ID), + RetrySafe: true, + ToolID: disabled.ID, + } + case len(enabled) == 0: + return 0, &filingError{ + Type: "destination_missing", + Message: fmt.Sprintf("Project %s has no %s", project.Name, noun), + RetrySafe: true, + } + } + + // The candidates are what container_id takes: the tools themselves, or + // for a to-do list, the lists in every enabled To-dos tool. + candidates := make([]filingCandidate, 0, len(enabled)) + plural := noun + " tools" + if container == basecamp.FilingIntoTodolist { + plural = "to-do lists" + for _, tool := range enabled { + lists, err := api.Todolists().List(ctx, tool.ID, nil) + if err != nil { + return 0, err + } + for _, list := range lists.Todolists { + candidates = append(candidates, filingCandidate{ID: list.ID, Title: list.Title}) + } + } + } else { + for _, tool := range enabled { + candidates = append(candidates, filingCandidate{ID: tool.ID, Title: tool.Title}) + } + } + switch len(candidates) { + case 0: + return 0, &filingError{ + Type: "destination_missing", + Message: fmt.Sprintf("Project %s has no %s", project.Name, plural), + RetrySafe: true, + } + case 1: + return candidates[0].ID, nil + } + return 0, &filingError{ + Type: "destination_ambiguous", + Message: fmt.Sprintf("Project %s has %d %s; pass one of them as container_id", project.Name, len(candidates), plural), + RetrySafe: true, + Candidates: candidates, + } +} + +// filingError is a move or copy that did not end with a new recording read +// back: how far it got, and whether calling the action again is safe. +type filingError struct { + Type string `json:"type"` + Message string `json:"message"` + RetrySafe bool `json:"retry_safe"` + FilingID int64 `json:"filing_id,omitempty"` + FilingStatus string `json:"filing_status,omitempty"` + RecordingID int64 `json:"recording_id,omitempty"` + AppURL string `json:"app_url,omitempty"` + ToolID int64 `json:"tool_id,omitempty"` + Candidates []filingCandidate `json:"candidates,omitempty"` +} + +type filingCandidate struct { + ID int64 `json:"id"` + Title string `json:"title"` +} + +func (f *filingError) Error() string { return f.Message } + +func (f *filingError) result() *mcp.CallToolResult { + result, _ := gateway.JSONResult(map[string]any{"error": f}) + result.IsError = true + return result +} + +// explainFilingError says how far a failed MoveAndWait or CopyAndWait got. +func explainFilingError(req filingRequest, container basecamp.FilingContainer, err error) error { + verb := strings.ToLower(string(req.kind)) + if refused, ok := errors.AsType[*basecamp.FilingRefusedError](err); ok { + message := fmt.Sprintf("Basecamp refused to %s it, and nothing was filed: %v", verb, refused.Err) + if apiErr, ok := errors.AsType[*basecamp.Error](refused.Err); ok && apiErr.Code == basecamp.CodeNotFound { + message += fmt.Sprintf(". Basecamp answers not found when the destination is not a %s this recording can go into, or is the one it is in now; a folder goes in folder_id", containerNouns[container]) + } + return &filingError{Type: "filing_refused", Message: message, RetrySafe: true} + } + if started, ok := errors.AsType[*basecamp.FilingMayHaveStartedError](err); ok { + if req.kind == basecamp.FilingCopy { + return &filingError{ + Type: "filing_may_have_started", + Message: fmt.Sprintf("The request to copy it failed without Basecamp refusing it (%v). Basecamp may have made the copy, and copying again makes another: look for it in the destination first", started.Err), + } + } + return &filingError{ + Type: "filing_may_have_started", + Message: fmt.Sprintf("The request to move it failed without Basecamp refusing it (%v). Basecamp refuses a repeated move, so trying again either moves it or is refused because this one did", started.Err), + RetrySafe: true, + } + } + if failed, ok := errors.AsType[*basecamp.FilingFailedError](err); ok { + return &filingError{ + Type: "filing_failed", + Message: fmt.Sprintf("Basecamp started to %s it and reported filing %d failed. Read the recording before trying again", verb, failed.Filing.ID), + FilingID: failed.Filing.ID, + FilingStatus: string(failed.Filing.Status), + } + } + if unsettled, ok := errors.AsType[*basecamp.FilingUnsettledError](err); ok { + return &filingError{ + Type: "filing_unsettled", + Message: fmt.Sprintf("Basecamp had not finished filing %d when the wait ended (%v). Do not %s it again; check it with get_filing and filingId %d, which names the new recording once it lands", unsettled.Filing.ID, unsettled.Cause, verb, unsettled.Filing.ID), + FilingID: unsettled.Filing.ID, + FilingStatus: string(unsettled.Filing.Status), + } + } + return err +} + +func pastTense(kind basecamp.FilingType) string { + if kind == basecamp.FilingCopy { + return "Copied" + } + return "Moved" +} + +// withSourceID adds source_id, the recording a filing came from, to the +// recording it made. +func withSourceID(record json.RawMessage, sourceID int64) (json.RawMessage, error) { + fields := map[string]any{} + decoder := json.NewDecoder(bytes.NewReader(record)) + decoder.UseNumber() + if err := decoder.Decode(&fields); err != nil { + return nil, err + } + fields["source_id"] = sourceID + return json.Marshal(fields) +} diff --git a/internal/mcpserver/filing_test.go b/internal/mcpserver/filing_test.go new file mode 100644 index 000000000..6874ec9a9 --- /dev/null +++ b/internal/mcpserver/filing_test.go @@ -0,0 +1,465 @@ +package mcpserver + +import ( + "encoding/json" + "fmt" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "os" + "regexp" + "slices" + "strconv" + "strings" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/modelcontextprotocol/go-sdk/mcp" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/basecamp/mcp/mcptest" +) + +const ( + fakeSourceID = 101 + fakeNewID = 202 + fakeProjectID = 77 + fakeFilingID = 9 + fakeListID = 600 +) + +// fakeBasecamp stands in for Basecamp behind move and copy: one recording of +// a given type and parent, a destination project whose dock the test sets, +// and a filing that runs through the statuses the test lists. +type fakeBasecamp struct { + t *testing.T + recordType string + parentType string + dock []basecamp.DockItem + lists []basecamp.Todolist + // listsIn overrides lists for the To-dos tool with that id. + listsIn map[string][]basecamp.Todolist + // start answers the POST that starts the filing; nil starts it. + start http.HandlerFunc + // statuses are what GetFiling reports, one per poll, the last repeated. + statuses []basecamp.FilingStatus + + mu sync.Mutex + started map[string]any + starts int + polls int +} + +var trailingID = regexp.MustCompile(`/(\d+)(\.json)?$`) + +func (f *fakeBasecamp) ServeHTTP(w http.ResponseWriter, r *http.Request) { + f.mu.Lock() + defer f.mu.Unlock() + path := r.URL.Path + switch { + case r.Method == http.MethodPost && (strings.HasSuffix(path, "/moves.json") || strings.HasSuffix(path, "/copies.json")): + f.starts++ + body, _ := io.ReadAll(r.Body) + f.started = map[string]any{} + require.NoError(f.t, json.Unmarshal(body, &f.started)) + f.started["path"] = path + if f.start != nil { + f.start(w, r) + return + } + f.json(w, http.StatusCreated, f.filing(basecamp.FilingPending)) + case strings.HasPrefix(path, "/999/filings/"): + status := f.statuses[min(f.polls, len(f.statuses)-1)] + f.polls++ + f.json(w, http.StatusOK, f.filing(status)) + case strings.TrimSuffix(path, ".json") == fmt.Sprintf("/999/projects/%d", fakeProjectID): + f.json(w, http.StatusOK, map[string]any{"id": fakeProjectID, "name": "Dest", "dock": f.dock}) + case strings.HasSuffix(path, "/todolists.json"): + todoset := strings.Split(path, "/")[3] + if lists, ok := f.listsIn[todoset]; ok { + f.json(w, http.StatusOK, lists) + return + } + f.json(w, http.StatusOK, f.lists) + case r.Method == http.MethodGet && trailingID.MatchString(path): + id, _ := strconv.Atoi(trailingID.FindStringSubmatch(path)[1]) + if id != fakeSourceID && id != fakeNewID { + f.t.Errorf("unexpected read: %s", path) + w.WriteHeader(http.StatusNotFound) + return + } + f.json(w, http.StatusOK, map[string]any{ + "id": id, + "type": f.recordType, + "title": "Thing", + "parent": map[string]any{"id": 55, "type": f.parentType, "title": "Old home"}, + "bucket": map[string]any{"id": fakeProjectID, "name": "Dest", "type": "Project"}, + }) + default: + f.t.Errorf("unexpected upstream request: %s %s", r.Method, path) + w.WriteHeader(http.StatusNotFound) + } +} + +func (f *fakeBasecamp) filing(status basecamp.FilingStatus) map[string]any { + filing := map[string]any{"id": fakeFilingID, "status": status, "type": "Move", "source_recording_id": fakeSourceID, "url": "x"} + if status == basecamp.FilingCompleted { + filing["destination_recording"] = map[string]any{"id": fakeNewID, "type": f.recordType, "url": "x", "app_url": "https://3.basecamp.com/999/x/202"} + } + return filing +} + +func (f *fakeBasecamp) json(w http.ResponseWriter, status int, body any) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + require.NoError(f.t, json.NewEncoder(w).Encode(body)) +} + +// dockFor is a destination project's dock with one enabled tool of every +// kind, each with its own id. +func dockFor() []basecamp.DockItem { + names := []basecamp.FilingContainer{ + basecamp.FilingIntoCardTable, basecamp.FilingIntoMessageBoard, basecamp.FilingIntoVault, basecamp.FilingIntoSchedule, + basecamp.FilingIntoQuestionnaire, basecamp.FilingIntoInbox, basecamp.FilingIntoTodoset, + } + dock := make([]basecamp.DockItem, len(names)) + for i, name := range names { + dock[i] = basecamp.DockItem{ID: int64(500 + i), Name: string(name), Title: string(name), Enabled: true} + } + return dock +} + +func newFake(t *testing.T, recordType, parentType string) *fakeBasecamp { + return &fakeBasecamp{ + t: t, + recordType: recordType, + parentType: parentType, + dock: dockFor(), + lists: []basecamp.Todolist{{ID: fakeListID, Title: "Only list"}}, + statuses: []basecamp.FilingStatus{basecamp.FilingProcessing, basecamp.FilingCompleted}, + } +} + +func filingSession(t *testing.T, fake *fakeBasecamp, cfg Config) *mcp.ClientSession { + t.Helper() + upstream := httptest.NewServer(fake) + t.Cleanup(upstream.Close) + srv, err := New(newTestAPI(upstream), cfg) + require.NoError(t, err) + return mcptest.Connect(t, srv.BuildMCPServer(slog.New(slog.DiscardHandler))) +} + +func file(t *testing.T, session *mcp.ClientSession, action string, params map[string]any) (map[string]any, bool) { + t.Helper() + text, isError := mcptest.CallText(t, session, "basecamp_recordings", map[string]any{"action": action, "params": params}) + var decoded map[string]any + if err := json.Unmarshal([]byte(text), &decoded); err != nil { + return map[string]any{"text": text}, isError + } + return decoded, isError +} + +func failureOf(t *testing.T, answer map[string]any) map[string]any { + t.Helper() + failure, ok := answer["error"].(map[string]any) + require.True(t, ok, "not a filing failure: %v", answer) + return failure +} + +func dockID(name basecamp.FilingContainer) float64 { + for _, tool := range dockFor() { + if tool.Name == string(name) { + return float64(tool.ID) + } + } + return 0 +} + +// recordableTypes is every recording type a candidate for filing could be, +// as Basecamp spells it in JSON: bc3's recordable models (app/models, each +// including Recordable), the SDK's RecordingType constants, and every +// type-shaped string in the vendored model. FilingContainerFor is asked +// about each of them, with each as a parent, so a type it starts accepting +// shows up here without anyone listing it. +func recordableTypes(t *testing.T) []string { + t.Helper() + seen := map[string]bool{} + for _, typ := range []string{ + "Attachment", "AutolinkedImage", "Chat::Lines::Code", "Chat::Lines::RichText", "Chat::Lines::Text", + "Chat::Lines::Upload", "Chat::Transcript", "Client::Approval", "Client::Approval::Response", "Client::Board", + "Client::Correspondence", "Client::Forward", "Client::Introduction", "Client::Reply", "CloudFile", "Comment", + "Dock", "Document", "Door", "Embed", "Gallery", "Gauge", "Gauge::Needle", "GoogleDocument", "Hill::Dot", + "Hill::Version", "Inbox", "Inbox::Forward", "Inbox::Reply", "Journal", "Journal::Entry", "Kanban::Board", + "Kanban::Boardset", "Kanban::Card", "Kanban::Column", "Kanban::DoneColumn", "Kanban::NotNowColumn", + "Kanban::OnHold", "Kanban::Step", "Kanban::Triage", "Kanban::Wormhole", "Message", "Message::Board", + "Notebook::Note", "Printout", "Question", "Question::Answer", "Questionnaire", "Relay", "Schedule", + "Schedule::Entry", "Schedule::Entry::Occurrence", "Subtask", "Timeline", "Timesheet", "Timesheet::Entry", + "Todo", "Todolist", "Todoset", "Upload", "Vault", + string(basecamp.RecordingTypeComment), string(basecamp.RecordingTypeDocument), string(basecamp.RecordingTypeDoor), + string(basecamp.RecordingTypeKanbanCard), string(basecamp.RecordingTypeKanbanStep), string(basecamp.RecordingTypeMessage), + string(basecamp.RecordingTypeQuestionAnswer), string(basecamp.RecordingTypeScheduleEntry), string(basecamp.RecordingTypeTodo), + string(basecamp.RecordingTypeTodolist), string(basecamp.RecordingTypeUpload), string(basecamp.RecordingTypeVault), + } { + seen[typ] = true + } + model, err := os.ReadFile("model/openapi.json") + require.NoError(t, err) + for _, match := range regexp.MustCompile(`"([A-Z][A-Za-z]+(?:::[A-Z][A-Za-z]+)*)"`).FindAllSubmatch(model, -1) { + seen[string(match[1])] = true + } + types := make([]string, 0, len(seen)) + for typ := range seen { + types = append(types, typ) + } + slices.Sort(types) + return types +} + +// TestFilingServesEveryTypeTheSDKFiles is the parity check: every recording +// type basecamp.FilingContainerFor accepts, under every parent it accepts it +// with, moves and copies through basecamp_recordings into a project's +// container of the kind the SDK names, and answers with the new recording. +// A type the SDK starts filing that this server cannot read, or a container +// kind it cannot resolve from a project, fails here. +func TestFilingServesEveryTypeTheSDKFiles(t *testing.T) { + type route struct { + typ string + container basecamp.FilingContainer + } + types := recordableTypes(t) + routes := map[route]string{} + for _, typ := range types { + for _, parent := range append([]string{""}, types...) { + if container, ok := basecamp.FilingContainerFor(typ, parent); ok { + if _, seen := routes[route{typ, container}]; !seen { + routes[route{typ, container}] = parent + } + } + } + } + require.NotEmpty(t, routes) + t.Logf("%d routes from %d candidate types", len(routes), len(types)) + + served := map[string]bool{} + for r, parent := range routes { + served[r.typ] = true + for _, action := range []string{moveAction, copyAction} { + t.Run(fmt.Sprintf("%s %s under %s", action, r.typ, parent), func(t *testing.T) { + t.Parallel() + fake := newFake(t, r.typ, parent) + fake.statuses = []basecamp.FilingStatus{basecamp.FilingCompleted} + session := filingSession(t, fake, Config{}) + + answer, isError := file(t, session, action, map[string]any{ + "recording_id": fakeSourceID, + "recording_type": r.typ, + "project_id": fakeProjectID, + }) + require.False(t, isError, "%v", answer) + assert.Equal(t, float64(fakeNewID), answer["id"]) + assert.Equal(t, float64(fakeSourceID), answer["source_id"]) + assert.Equal(t, r.typ, answer["type"]) + + want := dockID(r.container) + if r.container == basecamp.FilingIntoTodolist { + want = fakeListID + } + assert.Equal(t, want, fake.started["filing_destination_id"], "filed into the project's %s", r.container) + assert.Equal(t, 1, fake.starts, "started once") + }) + } + } + for typ := range filingReads { + assert.True(t, served[typ], "recording_type advertises %s, which FilingContainerFor never accepts", typ) + } +} + +func TestFilingIntoANamedContainerSkipsTheProject(t *testing.T) { + fake := newFake(t, "Document", "Vault") + session := filingSession(t, fake, Config{}) + + answer, isError := file(t, session, copyAction, map[string]any{ + "recording_id": fakeSourceID, + "recording_type": "Document", + "container_id": "4100", + "folder_id": 4200, + "include_comments": true, + }) + require.False(t, isError, "%v", answer) + assert.Equal(t, float64(4100), fake.started["filing_destination_id"]) + assert.Equal(t, float64(4200), fake.started["parent_id"]) + assert.Equal(t, true, fake.started["include_comments"]) + assert.Contains(t, fake.started["path"], "/copies.json") +} + +func TestFilingRefusesADestinationItCannotPick(t *testing.T) { + t.Run("turned off", func(t *testing.T) { + fake := newFake(t, "Message", "Message::Board") + fake.dock = []basecamp.DockItem{{ID: 900, Name: "message_board", Title: "Message Board", Enabled: false}} + failure := failureOf(t, mustFail(t, filingSession(t, fake, Config{}), moveAction, map[string]any{ + "recording_id": fakeSourceID, "recording_type": "Message", "project_id": fakeProjectID, + })) + assert.Equal(t, "destination_disabled", failure["type"]) + assert.Equal(t, float64(900), failure["tool_id"]) + assert.Equal(t, true, failure["retry_safe"]) + assert.Contains(t, failure["message"], "enable_tool") + assert.Zero(t, fake.starts) + }) + t.Run("several", func(t *testing.T) { + fake := newFake(t, "Kanban::Card", "Kanban::Triage") + fake.dock = []basecamp.DockItem{ + {ID: 901, Name: "kanban_board", Title: "Bugs", Enabled: true}, + {ID: 902, Name: "kanban_board", Title: "Ideas", Enabled: true}, + } + failure := failureOf(t, mustFail(t, filingSession(t, fake, Config{}), copyAction, map[string]any{ + "recording_id": fakeSourceID, "recording_type": "Kanban::Card", "project_id": fakeProjectID, + })) + assert.Equal(t, "destination_ambiguous", failure["type"]) + assert.Equal(t, []any{ + map[string]any{"id": float64(901), "title": "Bugs"}, + map[string]any{"id": float64(902), "title": "Ideas"}, + }, failure["candidates"]) + assert.Zero(t, fake.starts) + }) + t.Run("several to-do lists", func(t *testing.T) { + fake := newFake(t, "Todo", "Todolist") + fake.lists = []basecamp.Todolist{{ID: 601, Title: "A"}, {ID: 602, Title: "B"}} + failure := failureOf(t, mustFail(t, filingSession(t, fake, Config{}), moveAction, map[string]any{ + "recording_id": fakeSourceID, "recording_type": "Todo", "project_id": fakeProjectID, + })) + assert.Equal(t, "destination_ambiguous", failure["type"]) + assert.Len(t, failure["candidates"], 2) + assert.Zero(t, fake.starts) + }) + t.Run("to-do lists across several To-dos tools", func(t *testing.T) { + fake := newFake(t, "Todo", "Todolist") + fake.dock = append(fake.dock, basecamp.DockItem{ID: 599, Name: "todoset", Title: "Second", Enabled: true}) + fake.listsIn = map[string][]basecamp.Todolist{"599": {{ID: 601, Title: "Second list"}}} + failure := failureOf(t, mustFail(t, filingSession(t, fake, Config{}), copyAction, map[string]any{ + "recording_id": fakeSourceID, "recording_type": "Todo", "project_id": fakeProjectID, + })) + assert.Equal(t, "destination_ambiguous", failure["type"]) + assert.Equal(t, []any{ + map[string]any{"id": float64(fakeListID), "title": "Only list"}, + map[string]any{"id": float64(601), "title": "Second list"}, + }, failure["candidates"], "the candidates are lists, which container_id takes, not To-dos tools") + assert.Zero(t, fake.starts) + }) + t.Run("none", func(t *testing.T) { + fake := newFake(t, "Inbox::Forward", "Inbox") + fake.dock = nil + failure := failureOf(t, mustFail(t, filingSession(t, fake, Config{}), moveAction, map[string]any{ + "recording_id": fakeSourceID, "recording_type": "Inbox::Forward", "project_id": fakeProjectID, + })) + assert.Equal(t, "destination_missing", failure["type"]) + assert.Zero(t, fake.starts) + }) +} + +func mustFail(t *testing.T, session *mcp.ClientSession, action string, params map[string]any) map[string]any { + t.Helper() + answer, isError := file(t, session, action, params) + require.True(t, isError, "%v", answer) + return answer +} + +func TestFilingRefusesWhatItCannotFile(t *testing.T) { + cases := map[string]struct { + parentType string + params map[string]any + want string + }{ + "a whole Docs & Files tool": {"Project", map[string]any{"recording_type": "Vault", "project_id": fakeProjectID}, "not filed on its own"}, + "a folder outside Docs & Files": {"Message::Board", map[string]any{"recording_type": "Message", "container_id": 5, "folder_id": 6}, "folder_id applies only"}, + "both destinations": {"Message::Board", map[string]any{"recording_type": "Message", "container_id": 5, "project_id": fakeProjectID}, "exactly one"}, + "no destination": {"Message::Board", map[string]any{"recording_type": "Message"}, "exactly one"}, + "a type that is not filed": {"Message", map[string]any{"recording_type": "Comment", "container_id": 5}, "recording_type"}, + "comments on a move": {"Message::Board", map[string]any{"recording_type": "Message", "container_id": 5, "include_comments": true}, "unknown parameter"}, + } + for name, c := range cases { + t.Run(name, func(t *testing.T) { + typ, _ := c.params["recording_type"].(string) + fake := newFake(t, typ, c.parentType) + params := map[string]any{"recording_id": fakeSourceID} + for k, v := range c.params { + params[k] = v + } + answer := mustFail(t, filingSession(t, fake, Config{}), moveAction, params) + assert.Contains(t, fmt.Sprint(answer), c.want) + assert.Zero(t, fake.starts, "nothing filed") + }) + } +} + +// TestFilingSaysWhetherTryingAgainIsSafe pins how each way a filing can end +// short of a new recording reads to the caller. +func TestFilingSaysWhetherTryingAgainIsSafe(t *testing.T) { + defer func(saved basecamp.FilingWaitOptions) { filingWait = saved }(filingWait) + filingWait.Timeout = 300 * time.Millisecond + + params := map[string]any{"recording_id": fakeSourceID, "recording_type": "Message", "container_id": 5} + + t.Run("refused", func(t *testing.T) { + fake := newFake(t, "Message", "Message::Board") + fake.start = func(w http.ResponseWriter, _ *http.Request) { + fake.json(w, http.StatusUnprocessableEntity, map[string]any{"error": "nope"}) + } + failure := failureOf(t, mustFail(t, filingSession(t, fake, Config{}), copyAction, params)) + assert.Equal(t, "filing_refused", failure["type"]) + assert.Equal(t, true, failure["retry_safe"]) + }) + t.Run("refused as not found", func(t *testing.T) { + fake := newFake(t, "Message", "Message::Board") + fake.start = func(w http.ResponseWriter, _ *http.Request) { + fake.json(w, http.StatusNotFound, map[string]any{"error": "not found"}) + } + failure := failureOf(t, mustFail(t, filingSession(t, fake, Config{}), moveAction, params)) + assert.Equal(t, "filing_refused", failure["type"]) + assert.Contains(t, failure["message"], "not a message board this recording can go into, or is the one it is in now") + }) + t.Run("copy may have started", func(t *testing.T) { + fake := newFake(t, "Message", "Message::Board") + fake.start = func(w http.ResponseWriter, _ *http.Request) { + fake.json(w, http.StatusInternalServerError, map[string]any{"error": "boom"}) + } + failure := failureOf(t, mustFail(t, filingSession(t, fake, Config{}), copyAction, params)) + assert.Equal(t, "filing_may_have_started", failure["type"]) + assert.Equal(t, false, failure["retry_safe"]) + assert.Equal(t, 1, fake.starts, "a copy is never sent twice") + }) + t.Run("failed", func(t *testing.T) { + fake := newFake(t, "Message", "Message::Board") + fake.statuses = []basecamp.FilingStatus{basecamp.FilingFailed} + failure := failureOf(t, mustFail(t, filingSession(t, fake, Config{}), moveAction, params)) + assert.Equal(t, "filing_failed", failure["type"]) + assert.Equal(t, float64(fakeFilingID), failure["filing_id"]) + assert.Equal(t, false, failure["retry_safe"]) + }) + t.Run("unsettled", func(t *testing.T) { + fake := newFake(t, "Message", "Message::Board") + fake.statuses = []basecamp.FilingStatus{basecamp.FilingProcessing} + failure := failureOf(t, mustFail(t, filingSession(t, fake, Config{}), copyAction, params)) + assert.Equal(t, "filing_unsettled", failure["type"]) + assert.Equal(t, float64(fakeFilingID), failure["filing_id"]) + assert.Equal(t, "processing", failure["filing_status"]) + assert.Equal(t, false, failure["retry_safe"]) + assert.Contains(t, failure["message"], "get_filing") + }) +} + +func TestFilingIsAWriteTheReadOnlyServerDrops(t *testing.T) { + fake := newFake(t, "Message", "Message::Board") + session := filingSession(t, fake, Config{ReadOnly: true}) + + answer := mustFail(t, session, moveAction, map[string]any{"recording_id": fakeSourceID, "recording_type": "Message", "container_id": 5}) + assert.NotContains(t, fmt.Sprint(answer), "source_id") + assert.Zero(t, fake.starts) + + text, isError := mcptest.CallText(t, session, "basecamp_recordings", map[string]any{"action": "describe", "params": map[string]any{"action": "get_filing"}}) + assert.False(t, isError, "get_filing stays served read-only: %s", text) +} diff --git a/internal/mcpserver/testdata/catalog_snapshot.txt b/internal/mcpserver/testdata/catalog_snapshot.txt index ade56d087..76c7ba4ed 100644 --- a/internal/mcpserver/testdata/catalog_snapshot.txt +++ b/internal/mcpserver/testdata/catalog_snapshot.txt @@ -241,7 +241,6 @@ Call {"action": "describe", "params": {"action": "NAME"}} for an action's full p ACTIONS (RO = read-only): - archive_recording: Archive a recording; bc3 answers 403 for types it never lets be archived, timesheet entries among them -- copy_recording: Start copying a recording into another project; poll GetFiling for the result - create_answer: Create a new answer for a question - create_lineup_marker: Create a new lineup marker - create_project_from_template: Create a project from a template (asynchronous) @@ -261,7 +260,6 @@ ACTIONS (RO = read-only): - enable_tool: Enable a tool (show it on the project dock) - get_answer (RO): Get a single answer by id - get_answers_by_person (RO, paginated): Get all answers from a specific person for a question -- get_filing (RO): Get the state of a move or copy started by MoveRecording or CopyRecording - get_project_construction (RO): Get the status of a project construction - get_question (RO): Get a single question by id - get_question_reminders (RO, paginated): Get pending check-in reminders for the current user @@ -282,7 +280,6 @@ ACTIONS (RO = read-only): - list_recordings (RO, paginated): List recordings of a given type across projects - list_templates (RO, paginated): List all templates visible to the current user - list_webhooks (RO, paginated): List all webhooks for a project -- move_recording: Start moving a recording into another project; poll GetFiling for the result - move_recording_to_vault: Move a document, upload or vault into another vault in its project, or reorder it in its own vault - pause_question: Pause a check-in question (stops sending reminders) - reposition_tool: Reposition a tool on the project dock @@ -413,11 +410,14 @@ ACTIONS (RO = read-only): - update_my_note: Replace the note's content, recording a new revision server-side == basecamp_recordings -Recordings: the compact projection of one recording, resolved from the pointer an account event feed row carries. +Recordings: the compact projection of one recording, resolved from the pointer an account event feed row carries; and moving or copying a recording into another container or project, waiting for Basecamp to finish and returning the new recording. Gateway tool: call with {"action": "...", "params": {...}}. Call {"action": "describe", "params": {"action": "NAME"}} for an action's full parameter schema. ACTIONS (RO = read-only): +- copy: Synthetic: copy a recording into another container or project, wait for Basecamp to finish, and return the new recording. +- get_filing (RO): Get the state of a move or copy, by the filing_id a move or copy answered with when it had not finished +- move: Synthetic: move a recording into another container or project, wait for Basecamp to finish, and return the new recording. - summarize (RO): Synthetic: resolve a recording pointer into a compact projection. From a739380f4a10d3be96c45beda4e620a3e6fd34a8 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Wed, 28 Oct 2026 15:54:22 -0400 Subject: [PATCH 3/5] File a recurring event as its series, and type a failed read before filing Basecamp answers a recurring event's read with one occurrence, so the type check refused every recurring event. The read now names the series, with the occurrence kept as occurrence_id, as the CLI's own schedule move does. A failed read of the recording or the destination project now answers with error.type (recording_unreadable, destination_unreadable) and retry_safe, like every other failure after the arguments are accepted. --- internal/mcpserver/filing.go | 48 ++++++++++++++++++++++--- internal/mcpserver/filing_test.go | 58 +++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 5 deletions(-) diff --git a/internal/mcpserver/filing.go b/internal/mcpserver/filing.go index f6b27b9b1..4ff1c3eff 100644 --- a/internal/mcpserver/filing.go +++ b/internal/mcpserver/filing.go @@ -60,7 +60,7 @@ var filingReads = map[string]recordingReader{ "CloudFile": reader(API.CloudFiles, (*basecamp.CloudFilesService).Get), "GoogleDocument": reader(API.GoogleDocuments, (*basecamp.GoogleDocumentsService).Get), "Vault": reader(API.Vaults, (*basecamp.VaultsService).Get), - "Schedule::Entry": reader(API.Schedules, (*basecamp.SchedulesService).GetEntry), + "Schedule::Entry": seriesReader(reader(API.Schedules, (*basecamp.SchedulesService).GetEntry)), "Question": reader(API.Checkins, (*basecamp.CheckinsService).GetQuestion), "Inbox::Forward": reader(API.Forwards, (*basecamp.ForwardsService).Get), "Todolist": reader(API.Todolists, (*basecamp.TodolistsService).Get), @@ -92,6 +92,38 @@ func reader[S, T any](service func(API) S, get func(S, context.Context, int64) ( } } +// occurrenceType is what Basecamp answers a recurring event's read with: it +// redirects to one occurrence, whose parent is the event. +const occurrenceType = "Schedule::Entry::Occurrence" + +// seriesReader reads a recurring event as its series, which is what files: the +// read names the event, with the occurrence it answered with kept as +// occurrence_id. An occurrence's parent is the event, not the schedule, so the +// record carries no parent. +func seriesReader(read recordingReader) recordingReader { + return func(ctx context.Context, api API, id int64) (recordingRead, error) { + r, err := read(ctx, api, id) + if err != nil || r.typ != occurrenceType || r.parent == nil { + return r, err + } + fields := map[string]any{} + decoder := json.NewDecoder(bytes.NewReader(r.record)) + decoder.UseNumber() + if err := decoder.Decode(&fields); err != nil { + return recordingRead{}, err + } + fields["occurrence_id"] = fields["id"] + fields["id"] = r.parent.ID + fields["type"] = r.parent.Type + delete(fields, "parent") + raw, err := json.Marshal(fields) + if err != nil { + return recordingRead{}, err + } + return recordingRead{record: raw, id: r.parent.ID, typ: r.parent.Type}, nil + } +} + func filingTypes() []any { types := make([]any, 0, len(filingReads)) for _, typ := range slices.Sorted(maps.Keys(filingReads)) { @@ -133,7 +165,7 @@ func filingOperation(kind basecamp.FilingType) *catalog.Operation { "Composed in basecamp-sdk (RecordingsService." + verb + "AndWait): it reads the recording, resolves the destination, starts the " + lower + " once, waits up to a minute for Basecamp to finish it, and reads back the new recording. Not backed by any single Basecamp endpoint, so this action carries no method and no path.\n\n" + "Returns the new recording, as its type's own get action returns it, with source_id naming the recording it came from. A " + lower + " always gives the recording a new id. A card lands in the card table's Triage, or Done if it is completed.\n\n" + "Comments, card steps, check-in answers and other recordings that go with their parent are not filed on their own, and neither is a whole tool (a project's Docs & Files or card table itself).\n\n" + - "A failure says how far it got in error.type, and error.retry_safe says whether calling this action again is safe. destination_missing, destination_disabled and destination_ambiguous filed nothing and name what to pass instead (destination_disabled carries the tool_id that basecamp_automation enable_tool turns on; destination_ambiguous lists candidates for container_id). filing_refused filed nothing. filing_unsettled was still running when the wait ended: do not " + lower + " again; check it with get_filing and the filing_id it carries. filing_failed was started and reported failed by Basecamp. filed_unread was filed under recording_id but could not be read back." + "A failure says how far it got in error.type, and error.retry_safe says whether calling this action again is safe (a malformed call is refused as plain text instead). recording_unreadable and destination_unreadable are a read before the filing that failed, carrying the read's own error. destination_missing, destination_disabled and destination_ambiguous filed nothing and name what to pass instead (destination_disabled carries the tool_id that basecamp_automation enable_tool turns on; destination_ambiguous lists candidates for container_id). filing_refused filed nothing. filing_unsettled was still running when the wait ended: do not " + lower + " again; check it with get_filing and the filing_id it carries. filing_failed was started and reported failed by Basecamp. filed_unread was filed under recording_id but could not be read back." if kind == basecamp.FilingCopy { properties["include_comments"] = map[string]any{ "type": "boolean", @@ -260,7 +292,7 @@ func parseFilingRequest(kind basecamp.FilingType, op *catalog.Operation, params func fileRecording(ctx context.Context, api API, req filingRequest) (json.RawMessage, error) { src, err := filingReads[req.recordingType](ctx, api, req.recordingID) if err != nil { - return nil, err + return nil, unreadable("recording_unreadable", fmt.Sprintf("Could not read %s %d", req.recordingType, req.recordingID), err) } if src.typ != req.recordingType { return nil, fmt.Errorf("recording %d is a %s, not a %s", req.recordingID, src.typ, req.recordingType) @@ -329,7 +361,7 @@ func fileRecording(ctx context.Context, api API, req filingRequest) (json.RawMes func destinationInProject(ctx context.Context, api API, container basecamp.FilingContainer, projectID int64) (int64, error) { project, err := api.Projects().Get(ctx, projectID) if err != nil { - return 0, err + return 0, unreadable("destination_unreadable", fmt.Sprintf("Could not read project %d", projectID), err) } toolKind := container if container == basecamp.FilingIntoTodolist { @@ -372,7 +404,7 @@ func destinationInProject(ctx context.Context, api API, container basecamp.Filin for _, tool := range enabled { lists, err := api.Todolists().List(ctx, tool.ID, nil) if err != nil { - return 0, err + return 0, unreadable("destination_unreadable", fmt.Sprintf("Could not list the to-do lists in project %s's %s", project.Name, tool.Title), err) } for _, list := range lists.Todolists { candidates = append(candidates, filingCandidate{ID: list.ID, Title: list.Title}) @@ -422,6 +454,12 @@ type filingCandidate struct { func (f *filingError) Error() string { return f.Message } +// unreadable is a read before the filing that failed: nothing was filed, so +// trying again is safe, and the read's own error says whether it can work. +func unreadable(kind, what string, err error) *filingError { + return &filingError{Type: kind, Message: fmt.Sprintf("%s, so nothing was filed: %v", what, err), RetrySafe: true} +} + func (f *filingError) result() *mcp.CallToolResult { result, _ := gateway.JSONResult(map[string]any{"error": f}) result.IsError = true diff --git a/internal/mcpserver/filing_test.go b/internal/mcpserver/filing_test.go index 6874ec9a9..6a3e00973 100644 --- a/internal/mcpserver/filing_test.go +++ b/internal/mcpserver/filing_test.go @@ -42,6 +42,11 @@ type fakeBasecamp struct { parentType string dock []basecamp.DockItem lists []basecamp.Todolist + // recurring answers each read with an occurrence of the event read, as + // Basecamp does for a recurring event. + recurring bool + // unreadable answers reads of these paths with these statuses. + unreadable map[string]int // listsIn overrides lists for the To-dos tool with that id. listsIn map[string][]basecamp.Todolist // start answers the POST that starts the filing; nil starts it. @@ -61,6 +66,10 @@ func (f *fakeBasecamp) ServeHTTP(w http.ResponseWriter, r *http.Request) { f.mu.Lock() defer f.mu.Unlock() path := r.URL.Path + if status, ok := f.unreadable[strings.TrimSuffix(path, ".json")]; ok { + f.json(w, status, map[string]any{"error": "no"}) + return + } switch { case r.Method == http.MethodPost && (strings.HasSuffix(path, "/moves.json") || strings.HasSuffix(path, "/copies.json")): f.starts++ @@ -93,6 +102,15 @@ func (f *fakeBasecamp) ServeHTTP(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNotFound) return } + if f.recurring { + f.json(w, http.StatusOK, map[string]any{ + "id": id + 1, + "type": "Schedule::Entry::Occurrence", + "parent": map[string]any{"id": id, "type": "Schedule::Entry", "title": "Weekly"}, + "bucket": map[string]any{"id": fakeProjectID, "name": "Dest", "type": "Project"}, + }) + return + } f.json(w, http.StatusOK, map[string]any{ "id": id, "type": f.recordType, @@ -463,3 +481,43 @@ func TestFilingIsAWriteTheReadOnlyServerDrops(t *testing.T) { text, isError := mcptest.CallText(t, session, "basecamp_recordings", map[string]any{"action": "describe", "params": map[string]any{"action": "get_filing"}}) assert.False(t, isError, "get_filing stays served read-only: %s", text) } + +func TestFilingFilesARecurringEventAsItsSeries(t *testing.T) { + fake := newFake(t, "Schedule::Entry", "Schedule") + fake.recurring = true + session := filingSession(t, fake, Config{}) + + answer, isError := file(t, session, moveAction, map[string]any{ + "recording_id": fakeSourceID, "recording_type": "Schedule::Entry", "project_id": fakeProjectID, + }) + require.False(t, isError, "%v", answer) + assert.Equal(t, fmt.Sprintf("/999/recordings/%d/moves.json", fakeSourceID), fake.started["path"], "the series is what moves") + assert.Equal(t, float64(fakeNewID), answer["id"]) + assert.Equal(t, float64(fakeNewID+1), answer["occurrence_id"]) + assert.Equal(t, "Schedule::Entry", answer["type"]) + assert.Equal(t, float64(fakeSourceID), answer["source_id"]) + assert.NotContains(t, answer, "parent") +} + +func TestFilingTypesAFailedReadBeforeFiling(t *testing.T) { + t.Run("the recording", func(t *testing.T) { + fake := newFake(t, "Message", "Message::Board") + fake.unreadable = map[string]int{fmt.Sprintf("/999/messages/%d", fakeSourceID): http.StatusNotFound} + failure := failureOf(t, mustFail(t, filingSession(t, fake, Config{}), moveAction, map[string]any{ + "recording_id": fakeSourceID, "recording_type": "Message", "project_id": fakeProjectID, + })) + assert.Equal(t, "recording_unreadable", failure["type"]) + assert.Equal(t, true, failure["retry_safe"]) + assert.Zero(t, fake.starts) + }) + t.Run("the destination project", func(t *testing.T) { + fake := newFake(t, "Message", "Message::Board") + fake.unreadable = map[string]int{fmt.Sprintf("/999/projects/%d", fakeProjectID): http.StatusForbidden} + failure := failureOf(t, mustFail(t, filingSession(t, fake, Config{}), copyAction, map[string]any{ + "recording_id": fakeSourceID, "recording_type": "Message", "project_id": fakeProjectID, + })) + assert.Equal(t, "destination_unreadable", failure["type"]) + assert.Equal(t, true, failure["retry_safe"]) + assert.Zero(t, fake.starts) + }) +} From e846a39db34b5c3230267af1a0bc0f46e4c89a42 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Wed, 28 Oct 2026 15:59:58 -0400 Subject: [PATCH 4/5] Refuse a recording id that now redirects to a moved recording A moved recording's old id reads as its new one. Filing what that read found would move the recording again when a lost answer to a move is retried. The call is now refused as recording_moved, naming the new id, and nothing is filed. --- internal/mcpserver/filing.go | 13 ++++++++++++- internal/mcpserver/filing_test.go | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/internal/mcpserver/filing.go b/internal/mcpserver/filing.go index 4ff1c3eff..8b1f541d4 100644 --- a/internal/mcpserver/filing.go +++ b/internal/mcpserver/filing.go @@ -165,7 +165,7 @@ func filingOperation(kind basecamp.FilingType) *catalog.Operation { "Composed in basecamp-sdk (RecordingsService." + verb + "AndWait): it reads the recording, resolves the destination, starts the " + lower + " once, waits up to a minute for Basecamp to finish it, and reads back the new recording. Not backed by any single Basecamp endpoint, so this action carries no method and no path.\n\n" + "Returns the new recording, as its type's own get action returns it, with source_id naming the recording it came from. A " + lower + " always gives the recording a new id. A card lands in the card table's Triage, or Done if it is completed.\n\n" + "Comments, card steps, check-in answers and other recordings that go with their parent are not filed on their own, and neither is a whole tool (a project's Docs & Files or card table itself).\n\n" + - "A failure says how far it got in error.type, and error.retry_safe says whether calling this action again is safe (a malformed call is refused as plain text instead). recording_unreadable and destination_unreadable are a read before the filing that failed, carrying the read's own error. destination_missing, destination_disabled and destination_ambiguous filed nothing and name what to pass instead (destination_disabled carries the tool_id that basecamp_automation enable_tool turns on; destination_ambiguous lists candidates for container_id). filing_refused filed nothing. filing_unsettled was still running when the wait ended: do not " + lower + " again; check it with get_filing and the filing_id it carries. filing_failed was started and reported failed by Basecamp. filed_unread was filed under recording_id but could not be read back." + "A failure says how far it got in error.type, and error.retry_safe says whether calling this action again is safe (a malformed call is refused as plain text instead). recording_unreadable and destination_unreadable are a read before the filing that failed, carrying the read's own error. recording_moved is an id that now redirects to recording_id, because the recording was moved: nothing was filed, which is how a retried move is refused once the first one landed. destination_missing, destination_disabled and destination_ambiguous filed nothing and name what to pass instead (destination_disabled carries the tool_id that basecamp_automation enable_tool turns on; destination_ambiguous lists candidates for container_id). filing_refused filed nothing. filing_unsettled was still running when the wait ended: do not " + lower + " again; check it with get_filing and the filing_id it carries. filing_failed was started and reported failed by Basecamp. filed_unread was filed under recording_id but could not be read back." if kind == basecamp.FilingCopy { properties["include_comments"] = map[string]any{ "type": "boolean", @@ -294,6 +294,17 @@ func fileRecording(ctx context.Context, api API, req filingRequest) (json.RawMes if err != nil { return nil, unreadable("recording_unreadable", fmt.Sprintf("Could not read %s %d", req.recordingType, req.recordingID), err) } + if src.id != req.recordingID { + // A moved recording's old id redirects to its new one. Filing what + // the redirect found would move a recording again when a lost answer + // to a move is retried, so the caller names it by its new id. + return nil, &filingError{ + Type: "recording_moved", + Message: fmt.Sprintf("Recording %d has been moved and is now %d; nothing was filed. Pass recording_id %d to file it from where it is now", req.recordingID, src.id, src.id), + RetrySafe: true, + RecordingID: src.id, + } + } if src.typ != req.recordingType { return nil, fmt.Errorf("recording %d is a %s, not a %s", req.recordingID, src.typ, req.recordingType) } diff --git a/internal/mcpserver/filing_test.go b/internal/mcpserver/filing_test.go index 6a3e00973..cd2003b1f 100644 --- a/internal/mcpserver/filing_test.go +++ b/internal/mcpserver/filing_test.go @@ -45,6 +45,9 @@ type fakeBasecamp struct { // recurring answers each read with an occurrence of the event read, as // Basecamp does for a recurring event. recurring bool + // movedTo answers a read of the source with the recording at this id, + // as Basecamp's redirect from a moved recording's old id does. + movedTo int // unreadable answers reads of these paths with these statuses. unreadable map[string]int // listsIn overrides lists for the To-dos tool with that id. @@ -102,6 +105,9 @@ func (f *fakeBasecamp) ServeHTTP(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNotFound) return } + if id == fakeSourceID && f.movedTo != 0 { + id = f.movedTo + } if f.recurring { f.json(w, http.StatusOK, map[string]any{ "id": id + 1, @@ -521,3 +527,15 @@ func TestFilingTypesAFailedReadBeforeFiling(t *testing.T) { assert.Zero(t, fake.starts) }) } + +func TestFilingRefusesAnIDThatNowRedirects(t *testing.T) { + fake := newFake(t, "Document", "Vault") + fake.movedTo = fakeNewID + failure := failureOf(t, mustFail(t, filingSession(t, fake, Config{}), moveAction, map[string]any{ + "recording_id": fakeSourceID, "recording_type": "Document", "container_id": 5, + })) + assert.Equal(t, "recording_moved", failure["type"]) + assert.Equal(t, float64(fakeNewID), failure["recording_id"]) + assert.Equal(t, true, failure["retry_safe"]) + assert.Zero(t, fake.starts, "a retried move never files the recording again") +} From 5637b0d867a7fc46fef0ae109ae288a472a365a9 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Wed, 28 Oct 2026 16:20:11 -0400 Subject: [PATCH 5/5] Spell the copy action's describe doc as Copies --- internal/mcpserver/filing.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/mcpserver/filing.go b/internal/mcpserver/filing.go index 8b1f541d4..f063ac359 100644 --- a/internal/mcpserver/filing.go +++ b/internal/mcpserver/filing.go @@ -145,9 +145,9 @@ var containerNouns = map[basecamp.FilingContainer]string{ } func filingOperation(kind basecamp.FilingType) *catalog.Operation { - verb, action, id := "Move", moveAction, "MoveRecordingAndWait" + verb, verbs, action, id := "Move", "Moves", moveAction, "MoveRecordingAndWait" if kind == basecamp.FilingCopy { - verb, action, id = "Copy", copyAction, "CopyRecordingAndWait" + verb, verbs, action, id = "Copy", "Copies", copyAction, "CopyRecordingAndWait" } lower := strings.ToLower(verb) properties := map[string]any{ @@ -161,7 +161,7 @@ func filingOperation(kind basecamp.FilingType) *catalog.Operation { "container_id": idSchema("The container to " + lower + " it into, of the recording's kind: a card table, message board, Docs & Files tool, schedule, Automatic Check-ins, Email Forwards or To-dos tool from a project's dock, or a to-do list for a to-do or to-do list group. A folder is not a container; pass its Docs & Files here and the folder as folder_id."), "folder_id": idSchema("A folder inside the destination Docs & Files to put it in. Only for documents, uploads, cloud files, Google documents and folders."), } - doc := verb + "s a recording into another container, in its project or another one, as the web's " + verb + "… dialog does. " + + doc := verbs + " a recording into another container, in its project or another one, as the web's " + verb + "… dialog does. " + "Composed in basecamp-sdk (RecordingsService." + verb + "AndWait): it reads the recording, resolves the destination, starts the " + lower + " once, waits up to a minute for Basecamp to finish it, and reads back the new recording. Not backed by any single Basecamp endpoint, so this action carries no method and no path.\n\n" + "Returns the new recording, as its type's own get action returns it, with source_id naming the recording it came from. A " + lower + " always gives the recording a new id. A card lands in the card table's Triage, or Done if it is completed.\n\n" + "Comments, card steps, check-in answers and other recordings that go with their parent are not filed on their own, and neither is a whole tool (a project's Docs & Files or card table itself).\n\n" +