diff --git a/internal/cli/body.go b/internal/cli/body.go index 8dbcc4d..401e825 100644 --- a/internal/cli/body.go +++ b/internal/cli/body.go @@ -75,7 +75,10 @@ func loadBody(path string, stdin io.Reader, progress io.Writer, vars map[string] if len(raw) > maxBodyFileBytes { return nil, nil, Errorf(ExitUsage, "body file %s is over the %d-byte limit", path, maxBodyFileBytes) } - if strings.TrimSpace(string(raw)) == "" { + // isBlank rather than TrimSpace: this guard is what stands between a file + // with no content and a wiped page, since a replace deletes the old body + // whether or not the new one parsed to anything. + if isBlank(string(raw)) { return nil, nil, Errorf(ExitUsage, "body file %s is empty", path) } // Before parsing, not after: the expanded text is what the author meant to @@ -88,6 +91,13 @@ func loadBody(path string, stdin io.Reader, progress io.Writer, vars map[string] return nil, nil, Errorf(ExitUsage, "%s: %v", path, err) } raw = []byte(expanded) + // Checked again after expanding, as the append path does: a file of + // nothing but "{{ticket}}" is content on disk and expands to nothing + // when the row was addressed by --page-id or --id. + if isBlank(string(raw)) { + return nil, nil, Errorf(ExitUsage, + "body file %s expands to nothing: every placeholder in it resolved to an empty value", path) + } } blocks, warnings, err := markdown.ToBlocks(raw) if err != nil { diff --git a/internal/cli/body_test.go b/internal/cli/body_test.go index 18ab53f..8dcbaa8 100644 --- a/internal/cli/body_test.go +++ b/internal/cli/body_test.go @@ -1186,3 +1186,83 @@ func TestPreGateMessageDoesNotClaimAMeasuredRequestSize(t *testing.T) { t.Errorf("the message should name Notion's limit, got: %v", err) } } + +// --body-file REPLACES: the new blocks are appended, then every old one is +// deleted. loadBody's empty guard is the only thing standing between a file +// with no content and a body wipe — and strings.TrimSpace does not strip +// U+FEFF, which left Unicode's White_Space in 6.3. A BOM-only file therefore +// reads as content, goldmark returns zero blocks, ValidateAppendable returns +// nil on the empty slice, splitIntoRequests emits no batch so no PATCH goes +// out, and replaceBody deletes every child anyway. The body is gone, nothing +// replaced it, and the run exits 0. See #37. +func TestSetBodyFileHoldingOnlyInvisibleCharactersMakesNoHTTPCall(t *testing.T) { + cases := []struct { + name string + content string + }{ + {"a bare BOM", "\ufeff"}, + {"a BOM with whitespace", "\ufeff \n\t\n"}, + {"a zero-width space", "\u200b"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var calls int32 + cfg := withStubbedAPI(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/v1/data_sources/ds1" { + atomic.AddInt32(&calls, 1) + } + w.Write([]byte(cliSchemaJSON)) + }) + + p := filepath.Join(t.TempDir(), "blank.md") + if err := os.WriteFile(p, []byte(tc.content), 0o600); err != nil { + t.Fatal(err) + } + + if code := executeArgs([]string{"set", "--ticket", "BDF-231", + "--body-file", p, "--config", cfg}); code != ExitUsage { + t.Fatalf("a body file with no content must exit %d, got %d", ExitUsage, code) + } + if n := atomic.LoadInt32(&calls); n != 0 { + t.Fatalf("it must be refused before anything reaches Notion, got %d calls", n) + } + }) + } +} + +// The overcorrection this fix must not make: a BOM in front of real content is +// an ordinary file, and refusing it would break writing bodies from any editor +// that emits one. +func TestBodyFileKeepsAFileWhoseBOMPrefixesRealContent(t *testing.T) { + p := filepath.Join(t.TempDir(), "bom-content.md") + if err := os.WriteFile(p, []byte("\ufeff## Progress\nShipped.\n"), 0o600); err != nil { + t.Fatal(err) + } + + req, _, err := loadBody(p, nil, nil, nil) + if err != nil { + t.Fatalf("a BOM before real content must not make the file blank: %v", err) + } + if len(req.Blocks) == 0 { + t.Fatal("the content parsed to no blocks at all") + } +} + +// The twin hole, on the same replace path: --expand can empty a file that was +// non-blank on disk. loadAppendBody re-checks after expanding; loadBody did +// not, so a file of nothing but "{{ticket}}" addressed by --page-id parsed to +// zero blocks and the body was deleted with nothing to replace it. +func TestBodyFileThatExpandsToNothingIsRefused(t *testing.T) { + p := filepath.Join(t.TempDir(), "expand.md") + if err := os.WriteFile(p, []byte("\ufeff{{ticket}}\n"), 0o600); err != nil { + t.Fatal(err) + } + + _, _, err := loadBody(p, nil, nil, map[string]string{"ticket": ""}) + if err == nil { + t.Fatal("a body file that expands to nothing must be refused") + } + if exitCodeFor(err) != ExitUsage { + t.Fatalf("want a usage error, got %v (code %d)", err, exitCodeFor(err)) + } +} diff --git a/internal/service/service.go b/internal/service/service.go index 983dc61..835fe06 100644 --- a/internal/service/service.go +++ b/internal/service/service.go @@ -287,6 +287,15 @@ const sentinelAdvice = "re-run to converge" // happen), which the CLI surfaces even in the partial-failure JSON (spec §8). func (s *Service) replaceBody(ctx context.Context, pageID string, req *BodyRequest) (BodyResult, error) { var res BodyResult + // A replace with nothing to write is a delete. The CLI already refuses a + // blank file, but that guard is one check in one command's argument + // handling and it has been wrong before (#37: it did not strip a BOM, and + // a BOM-only file parses to zero blocks). The page's content is not + // recoverable, so the refusal belongs here too, next to the deleting. + if len(req.Blocks) == 0 { + return res, errors.New( + "refusing to replace a body with no blocks: this would delete the page's content and write nothing in its place") + } old, err := s.client.ListBlockChildren(ctx, pageID) if err != nil { return res, err diff --git a/internal/service/service_test.go b/internal/service/service_test.go index 2509e1d..e4c09c8 100644 --- a/internal/service/service_test.go +++ b/internal/service/service_test.go @@ -1253,3 +1253,25 @@ func TestAmbiguousAppendErrorUnwrapsThroughExtraWrapping(t *testing.T) { t.Fatal("errors.Is must still reach the sentinel through extra wrapping") } } + +// replaceBody deletes the page's existing blocks after writing the new ones. +// With no blocks to write, "replace" is just the delete: the body goes and +// nothing takes its place, and the run reports success. Nothing upstream +// should send that — loadBody rejects a blank file — but that guard is one +// TrimSpace in one command's argument handling, and it has been wrong (#37: +// it did not strip a BOM, and a BOM-only file parses to zero blocks). The +// property belongs here too, where the deleting actually happens. +func TestReplaceBodyRefusesToDeleteWhenThereIsNothingToWrite(t *testing.T) { + var seen []string + srv := bodyRoutes(t, rowJSON, `{"id":"old1","type":"paragraph"}`, &seen) + defer srv.Close() + + s := New(notion.New("t", notion.WithBaseURL(srv.URL), notion.WithSleep(func(time.Duration) {})), testProfile()) + _, err := s.Set(context.Background(), tracker.Fields{Ticket: "BDF-231"}, &BodyRequest{}) + if indexOf(seen, "DELETE", "/blocks/old1") >= 0 { + t.Fatalf("the old body was deleted with nothing to replace it: %v", seen) + } + if err == nil { + t.Fatal("replacing a body with no blocks must be refused, not performed") + } +} diff --git a/skills/notion-track/SKILL.md b/skills/notion-track/SKILL.md index 98556c3..ba2c811 100644 --- a/skills/notion-track/SKILL.md +++ b/skills/notion-track/SKILL.md @@ -256,8 +256,10 @@ REQUEST, not the file: an append is one payload and Notion caps a payload at 500KB, and the serialized request is always bigger than the file (Markdown is escaped into JSON, so every newline costs two bytes, and `--expand` runs first). A 450KB file of short lines can exceed it. Over the limit is exit 2 before any -request, with the request size named — an append cannot be split, so send it in -two runs or use `--body-file`, which is batched and allows 1 MiB. Mutually +request. The message names the request size when it built one, and a floor on +the file size (`is at least N bytes`) when the file alone is already too big to +be worth serializing — an append cannot be split, so send it in two runs or use +`--body-file`, which is batched and allows 1 MiB. Mutually exclusive with `--body-file` — one write picks one strategy. **Not idempotent.** Running it twice appends the note twice; there is no