diff --git a/internal/commands/files.go b/internal/commands/files.go index 03a94e6a..04f61fcd 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -1208,13 +1208,45 @@ func newFilesUpdateCmd(project *string) *cobra.Command { Short: "Update a document, vault, or upload", Long: `Update a document, vault, or upload. +For documents, updating only --title or only --content preserves the untouched field. + You can pass either an item ID or a Basecamp URL: basecamp files update 789 --title "new title" --in my-project basecamp files update 789 --content "new content" --in my-project`, - Args: cobra.ExactArgs(1), + Annotations: map[string]string{"agent_notes": "Document updates preserve untouched title/content by fetching current state first because BC3 rebuilds documents from permitted params on PUT; explicit clears via --title \"\"/--content \"\" work because the SDK strips empty strings to absent fields, which the controller then nulls. Upload/vault updates do not clear by omission, so empty-valued flags are rejected CLI-side."}, + Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - if strings.TrimSpace(title) == "" && strings.TrimSpace(content) == "" && itemType == "" { - return noChanges(cmd) + titleChanged := cmd.Flags().Changed("title") + contentChanged := cmd.Flags().Changed("content") + titleTrimmed := strings.TrimSpace(title) + contentTrimmed := strings.TrimSpace(content) + itemType = strings.ToLower(strings.TrimSpace(itemType)) + switch itemType { + case "", "document", "doc": + // Explicit "" clears are valid for documents; whitespace-only is not. + hasTitle := titleChanged && (title == "" || titleTrimmed != "") + hasContent := contentChanged && (content == "" || contentTrimmed != "") + if !hasTitle && !hasContent { + return noChanges(cmd) + } + case "vault", "folder": + if contentChanged { + return output.ErrUsage("--content can only be used with --type document or upload") + } + if !titleChanged || titleTrimmed == "" { + return noChanges(cmd) + } + case "upload", "file": + hasTitle := titleChanged && titleTrimmed != "" + hasContent := contentChanged && contentTrimmed != "" + if !hasTitle && !hasContent { + return noChanges(cmd) + } + default: + return output.ErrUsageHint( + fmt.Sprintf("Invalid type: %s", itemType), + "Use: vault, document, or upload", + ) } app := appctx.FromContext(cmd.Context()) @@ -1269,12 +1301,10 @@ You can pass either an item ID or a Basecamp URL: result = vault detectedType = "vault" case "document", "doc": - docHTML := richtext.MarkdownToHTML(content) - docHTML, err = resolveLocalImages(cmd, app, docHTML) + req, err := buildDocumentUpdateRequest(cmd, app, itemID, nil, titleChanged, contentChanged, title, content) if err != nil { - return err + return convertSDKError(err) } - req := &basecamp.UpdateDocumentRequest{Title: title, Content: docHTML} doc, err := app.Account().Documents().Update(cmd.Context(), itemID, req) if err != nil { return convertSDKError(err) @@ -1304,14 +1334,12 @@ You can pass either an item ID or a Basecamp URL: var firstErr error // Try document first (most common update case) - _, err := app.Account().Documents().Get(cmd.Context(), itemID) + existingDoc, err := app.Account().Documents().Get(cmd.Context(), itemID) if err == nil { - docHTML := richtext.MarkdownToHTML(content) - docHTML, resolveErr := resolveLocalImages(cmd, app, docHTML) - if resolveErr != nil { - return resolveErr + req, buildErr := buildDocumentUpdateRequest(cmd, app, itemID, existingDoc, titleChanged, contentChanged, title, content) + if buildErr != nil { + return convertSDKError(buildErr) } - req := &basecamp.UpdateDocumentRequest{Title: title, Content: docHTML} doc, err := app.Account().Documents().Update(cmd.Context(), itemID, req) if err != nil { return convertSDKError(err) @@ -1323,6 +1351,12 @@ You can pass either an item ID or a Basecamp URL: // Try vault _, err = app.Account().Vaults().Get(cmd.Context(), itemID) if err == nil { + if contentChanged { + return output.ErrUsage("detected a folder/vault; use --title to rename it") + } + if !titleChanged || titleTrimmed == "" { + return noChanges(cmd) + } req := &basecamp.UpdateVaultRequest{Title: title} vault, err := app.Account().Vaults().Update(cmd.Context(), itemID, req) if err != nil { @@ -1334,6 +1368,11 @@ You can pass either an item ID or a Basecamp URL: // Try upload _, err = app.Account().Uploads().Get(cmd.Context(), itemID) if err == nil { + hasTitle := titleChanged && titleTrimmed != "" + hasContent := contentChanged && contentTrimmed != "" + if !hasTitle && !hasContent { + return noChanges(cmd) + } req := &basecamp.UpdateUploadRequest{Description: content} if title != "" { req.BaseName = title @@ -1380,6 +1419,49 @@ You can pass either an item ID or a Basecamp URL: return cmd } +func buildDocumentUpdateRequest(cmd *cobra.Command, app *appctx.App, itemID int64, existingDoc *basecamp.Document, titleChanged, contentChanged bool, title, content string) (*basecamp.UpdateDocumentRequest, error) { + // BC3 rebuilds documents from permitted params on PUT, so omitted + // title/content fields are replaced with empty values. Fetch and merge when + // the caller updates only one field so the untouched field is preserved. + // + // Explicit clears via --title "" or --content "" work by composition: the + // SDK strips empty strings to absent JSON fields, and the controller then + // nulls those absent fields during rebuild. The wire-shape assertion in + // TestFilesUpdateDocumentEmptyTitleClearsWhilePreservingContent pins this. + if existingDoc == nil && (!titleChanged || !contentChanged) { + var err error + existingDoc, err = app.Account().Documents().Get(cmd.Context(), itemID) + if err != nil { + return nil, err + } + } + + req := &basecamp.UpdateDocumentRequest{} + if existingDoc != nil { + req.Title = existingDoc.Title + req.Content = existingDoc.Content + } + + if titleChanged { + req.Title = title + } + if contentChanged { + if content == "" { + req.Content = "" + return req, nil + } + docHTML := richtext.MarkdownToHTML(content) + var err error + docHTML, err = resolveLocalImages(cmd, app, docHTML) + if err != nil { + return nil, err + } + req.Content = docHTML + } + + return req, nil +} + func newFilesDownloadCmd(project *string) *cobra.Command { var outDir string diff --git a/internal/commands/files_test.go b/internal/commands/files_test.go index da3acf38..e6fec86c 100644 --- a/internal/commands/files_test.go +++ b/internal/commands/files_test.go @@ -3,7 +3,11 @@ package commands import ( "bytes" "context" + "encoding/json" "errors" + "fmt" + "io" + "net/http" "strings" "testing" @@ -157,3 +161,310 @@ func TestFilesDownloadStdoutStreamsUploadID(t *testing.T) { assert.Equal(t, fileContent, stdout.String(), "upload body should be streamed directly to stdout") } + +type mockFilesUpdateTransport struct { + capturedBody []byte + requests []string +} + +func (t *mockFilesUpdateTransport) RoundTrip(req *http.Request) (*http.Response, error) { + t.requests = append(t.requests, req.Method+" "+req.URL.Path) + + header := make(http.Header) + header.Set("Content-Type", "application/json") + + switch { + case req.Method == http.MethodGet && strings.Contains(req.URL.Path, "/projects.json"): + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(`[{"id":456,"name":"Test Project"}]`)), + Header: header, + }, nil + case req.Method == http.MethodGet && strings.Contains(req.URL.Path, "/documents/999"): + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader( + `{"id":999,"title":"Existing title","content":"
Existing body
","status":"active","bucket":{"id":456,"name":"Test Project","type":"Project"}}`, + )), + Header: header, + }, nil + case req.Method == http.MethodPut && strings.Contains(req.URL.Path, "/documents/999"): + if req.Body != nil { + body, err := io.ReadAll(req.Body) + if err != nil { + return nil, fmt.Errorf("reading request body: %w", err) + } + t.capturedBody = body + _ = req.Body.Close() + } + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader( + `{"id":999,"title":"Updated title","content":"
Existing body
","status":"active","bucket":{"id":456,"name":"Test Project","type":"Project"}}`, + )), + Header: header, + }, nil + default: + return nil, fmt.Errorf("unexpected request: %s %s", req.Method, req.URL.Path) + } +} + +func TestFilesUpdateDocumentTitlePreservesExistingContent(t *testing.T) { + transport := &mockFilesUpdateTransport{} + app := showTestApp(t, transport) + app.Config.ProjectID = "456" + + cmd := NewFilesCmd() + err := executeMessagesCommand(cmd, app, "update", "999", "--type", "document", "--title", "Updated title") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBody) + + var body map[string]any + err = json.Unmarshal(transport.capturedBody, &body) + require.NoError(t, err) + + assert.Equal(t, "Updated title", body["title"]) + assert.Equal(t, "
Existing body
", body["content"]) +} + +func TestFilesUpdateDocumentContentPreservesExistingTitle(t *testing.T) { + transport := &mockFilesUpdateTransport{} + app := showTestApp(t, transport) + app.Config.ProjectID = "456" + + cmd := NewFilesCmd() + err := executeMessagesCommand(cmd, app, "update", "999", "--content", "Updated **body**") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBody) + + var body map[string]any + err = json.Unmarshal(transport.capturedBody, &body) + require.NoError(t, err) + + assert.Equal(t, "Existing title", body["title"]) + assert.Equal(t, "

Updated body

", body["content"]) +} + +func TestFilesUpdateDocumentEmptyTitleClearsWhilePreservingContent(t *testing.T) { + transport := &mockFilesUpdateTransport{} + app := showTestApp(t, transport) + app.Config.ProjectID = "456" + + cmd := NewFilesCmd() + err := executeMessagesCommand(cmd, app, "update", "999", "--type", "document", "--title", "") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBody) + + var body map[string]any + err = json.Unmarshal(transport.capturedBody, &body) + require.NoError(t, err) + + _, hasTitle := body["title"] + assert.False(t, hasTitle) + assert.Equal(t, "
Existing body
", body["content"]) +} + +func TestFilesUpdateDocumentEmptyContentClearsWhilePreservingTitle(t *testing.T) { + transport := &mockFilesUpdateTransport{} + app := showTestApp(t, transport) + app.Config.ProjectID = "456" + + cmd := NewFilesCmd() + err := executeMessagesCommand(cmd, app, "update", "999", "--content", "") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBody) + + var body map[string]any + err = json.Unmarshal(transport.capturedBody, &body) + require.NoError(t, err) + + assert.Equal(t, "Existing title", body["title"]) + _, hasContent := body["content"] + assert.False(t, hasContent) +} + +func TestFilesUpdateTypeWithoutChangesShowsHelp(t *testing.T) { + app, _ := setupMessagesTestApp(t) + app.Config.ProjectID = "456" + + cmd := NewFilesCmd() + err := executeMessagesCommand(cmd, app, "update", "999", "--type", "document") + assert.NoError(t, err) +} + +func TestFilesUpdateVaultRejectsContentFlag(t *testing.T) { + app, _ := setupMessagesTestApp(t) + app.Config.ProjectID = "456" + + cmd := NewFilesCmd() + err := executeMessagesCommand(cmd, app, "update", "999", "--type", "vault", "--content", "desc") + 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, "--content can only be used with --type document or upload") +} + +func TestFilesUpdateVaultWithoutTitleShowsHelp(t *testing.T) { + app, _ := setupMessagesTestApp(t) + app.Config.ProjectID = "456" + + cmd := NewFilesCmd() + err := executeMessagesCommand(cmd, app, "update", "999", "--type", "vault") + assert.NoError(t, err) +} + +type mockFilesAutodetectVaultTransport struct{} + +func (t *mockFilesAutodetectVaultTransport) RoundTrip(req *http.Request) (*http.Response, error) { + header := make(http.Header) + header.Set("Content-Type", "application/json") + + switch { + case req.Method == http.MethodGet && strings.Contains(req.URL.Path, "/projects.json"): + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(`[{"id":456,"name":"Test Project"}]`)), + Header: header, + }, nil + case req.Method == http.MethodGet && strings.Contains(req.URL.Path, "/documents/999"): + return &http.Response{ + StatusCode: 404, + Body: io.NopCloser(strings.NewReader(`{"error":"not found"}`)), + Header: header, + }, nil + case req.Method == http.MethodGet && strings.Contains(req.URL.Path, "/vaults/999"): + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(`{"id":999,"title":"Existing folder"}`)), + Header: header, + }, nil + case req.Method == http.MethodPut: + return nil, fmt.Errorf("unexpected update request: %s", req.URL.Path) + default: + return nil, fmt.Errorf("unexpected request: %s %s", req.Method, req.URL.Path) + } +} + +func TestFilesUpdateAutodetectVaultRejectsContentOnly(t *testing.T) { + app := showTestApp(t, &mockFilesAutodetectVaultTransport{}) + app.Config.ProjectID = "456" + + cmd := NewFilesCmd() + err := executeMessagesCommand(cmd, app, "update", "999", "--content", "desc") + 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, "detected a folder/vault; use --title to rename it") +} + +func TestFilesUpdateTypedVaultEmptyTitleNoChanges(t *testing.T) { + app, _ := setupMessagesTestApp(t) + app.Config.ProjectID = "456" + + cmd := NewFilesCmd() + err := executeMessagesCommand(cmd, app, "update", "999", "--type", "vault", "--title", "") + assert.NoError(t, err) +} + +func TestFilesUpdateTypedUploadEmptyTitleNoChanges(t *testing.T) { + app, _ := setupMessagesTestApp(t) + app.Config.ProjectID = "456" + + cmd := NewFilesCmd() + err := executeMessagesCommand(cmd, app, "update", "999", "--type", "upload", "--title", "") + assert.NoError(t, err) +} + +func TestFilesUpdateTypedUploadEmptyContentNoChanges(t *testing.T) { + app, _ := setupMessagesTestApp(t) + app.Config.ProjectID = "456" + + cmd := NewFilesCmd() + err := executeMessagesCommand(cmd, app, "update", "999", "--type", "upload", "--content", "") + assert.NoError(t, err) +} + +func TestFilesUpdateAutodetectVaultEmptyTitleNoChanges(t *testing.T) { + app := showTestApp(t, &mockFilesAutodetectVaultTransport{}) + app.Config.ProjectID = "456" + + cmd := NewFilesCmd() + err := executeMessagesCommand(cmd, app, "update", "999", "--title", "") + assert.NoError(t, err) +} + +type mockFilesAutodetectUploadTransport struct{} + +func (t *mockFilesAutodetectUploadTransport) RoundTrip(req *http.Request) (*http.Response, error) { + header := make(http.Header) + header.Set("Content-Type", "application/json") + + switch { + case req.Method == http.MethodGet && strings.Contains(req.URL.Path, "/projects.json"): + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(`[{"id":456,"name":"Test Project"}]`)), + Header: header, + }, nil + case req.Method == http.MethodGet && strings.Contains(req.URL.Path, "/documents/999"): + return &http.Response{ + StatusCode: 404, + Body: io.NopCloser(strings.NewReader(`{"error":"not found"}`)), + Header: header, + }, nil + case req.Method == http.MethodGet && strings.Contains(req.URL.Path, "/vaults/999"): + return &http.Response{ + StatusCode: 404, + Body: io.NopCloser(strings.NewReader(`{"error":"not found"}`)), + Header: header, + }, nil + case req.Method == http.MethodGet && strings.Contains(req.URL.Path, "/uploads/999"): + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(`{"id":999,"filename":"report.pdf"}`)), + Header: header, + }, nil + case req.Method == http.MethodPut: + return nil, fmt.Errorf("unexpected update request: %s", req.URL.Path) + default: + return nil, fmt.Errorf("unexpected request: %s %s", req.Method, req.URL.Path) + } +} + +func TestFilesUpdateAutodetectUploadEmptyContentNoChanges(t *testing.T) { + app := showTestApp(t, &mockFilesAutodetectUploadTransport{}) + app.Config.ProjectID = "456" + + cmd := NewFilesCmd() + err := executeMessagesCommand(cmd, app, "update", "999", "--content", "") + assert.NoError(t, err) +} + +func TestFilesUpdateTypedVaultWhitespaceTitleNoChanges(t *testing.T) { + app, _ := setupMessagesTestApp(t) + app.Config.ProjectID = "456" + + cmd := NewFilesCmd() + err := executeMessagesCommand(cmd, app, "update", "999", "--type", "vault", "--title", " ") + assert.NoError(t, err) +} + +func TestFilesUpdateTypedUploadWhitespaceContentNoChanges(t *testing.T) { + app, _ := setupMessagesTestApp(t) + app.Config.ProjectID = "456" + + cmd := NewFilesCmd() + err := executeMessagesCommand(cmd, app, "update", "999", "--type", "upload", "--content", " ") + assert.NoError(t, err) +} + +func TestFilesUpdateTypedDocumentWhitespaceTitleNoChanges(t *testing.T) { + app, _ := setupMessagesTestApp(t) + app.Config.ProjectID = "456" + + cmd := NewFilesCmd() + err := executeMessagesCommand(cmd, app, "update", "999", "--type", "document", "--title", " ") + assert.NoError(t, err) +} diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index b29ab263..0aaa8599 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -532,9 +532,13 @@ basecamp files folder create "Folder" --in basecamp files doc create "Doc" "Body" --in basecamp files doc create "Draft" --draft --in basecamp files doc create "Notes" "..." --no-subscribe --in -basecamp files update --title "New" --content "Updated" +basecamp files update --title "New" --content "Updated" +basecamp files update --title "New" --in # Preserves existing document content +basecamp files update --content "Updated" --in # Preserves existing document title ``` +**Document update semantics:** `basecamp files update ` is safe for partial updates in the CLI: when you pass only `--title` or only `--content`, the CLI first fetches the current document and preserves the untouched field. + **Subcommands:** `folders`, `uploads`, `documents` (each with pagination flags) ### Schedule