From a67e1425ef44ef62c603470c04a3e16292ed8d92 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Thu, 6 Aug 2026 16:20:17 +0200 Subject: [PATCH] fix(cli): stop a body file with no content from wiping the page A --body-file replaces: the new blocks are appended, then every old one is deleted. loadBody's empty check is the only thing standing between a file with no content and a body that is gone with nothing in its place, and it was strings.TrimSpace, which does not strip U+FEFF -- that left Unicode's White_Space in 6.3. So a BOM-only file -- what an editor on Windows writes for an empty file saved as UTF-8-with-BOM, and what `> notes.md` produces in PowerShell -- read as content. From there: goldmark returns zero blocks, ValidateAppendable ranges over an empty slice and returns nil, splitIntoRequests emits no batch so no PATCH goes out at all, and replaceBody deletes every non-sub-page child anyway. The wire trace is GET children, DELETE, and no write between them. Exit 0. PR #36 fixed the same bypass on the append path, where it costs a false appended:true. On the replace path it costs the body. isBlank already existed from that fix; this reuses it. The twin hole went with it: loadAppendBody re-checks emptiness after --expand, loadBody did not, so a file of nothing but "{{ticket}}" addressed by --page-id expanded to nothing, parsed to zero blocks, and took the same path. Both are argument handling in one command, though, and the content they protect is not recoverable. replaceBody now refuses a request with no blocks whatever the caller passed, so the property lives next to the deleting rather than two packages away from it. Also fixes the SKILL.md clause that PR #36's own change made stale: the over-limit message names a request size only when it built one, and a floor on the file size when it did not. Closes #37 --- internal/cli/body.go | 12 ++++- internal/cli/body_test.go | 80 ++++++++++++++++++++++++++++++++ internal/service/service.go | 9 ++++ internal/service/service_test.go | 22 +++++++++ skills/notion-track/SKILL.md | 6 ++- 5 files changed, 126 insertions(+), 3 deletions(-) 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