From a55ba0020e05e4c3587b9ad589092e845c54ef51 Mon Sep 17 00:00:00 2001 From: piekstra Date: Fri, 14 Aug 2026 09:32:34 -0400 Subject: [PATCH 1/4] fix(cfl): INT-732 detect formatting loss a lossy read hides The verification added in INT-725 compares what the caller sent against what Confluence stored. When the caller read their content with --body-format adf, both sides carry the same loss and the check passes over a damaged page. Confluence's ADF representation does not always carry marks its storage representation does: em and strong wrapping a code span inside a table cell are dropped. Reading a page as ADF and writing the identical bytes back therefore destroys them. Reproduced on a scratch page, where a no-op round trip took the storage body from three em and three strong elements to one of each while the command reported success. This is not hypothetical. It happened to a real page during onboarding work: three table rows lost the bold italic that the page's own legend describes as marking rows changed in a migration, and every ADF-based check agreed the write was clean because the baseline came through the same lossy read. The storage body survives the round trip, so it is what gets watched. It is captured before the write and compared after, and element types whose counts fell are reported. A text edit moves no element counts and stays silent; losing emphasis, a code span or a link is collateral a caller rarely intends. A missing baseline reports nothing rather than reporting no loss, so a failed read never manufactures a clean result. Markdown writes are unaffected: they are not verified, so no extra read is paid for. [INT-732] --- tools/cfl/internal/cmd/page/create.go | 13 ++-- tools/cfl/internal/cmd/page/edit.go | 20 ++++-- tools/cfl/internal/cmd/page/verify.go | 80 +++++++++++++++++++++- tools/cfl/internal/cmd/page/verify_test.go | 80 ++++++++++++++++++++++ tools/cfl/internal/present/mutation.go | 10 +++ 5 files changed, 190 insertions(+), 13 deletions(-) diff --git a/tools/cfl/internal/cmd/page/create.go b/tools/cfl/internal/cmd/page/create.go index 622aafb..e4dfea7 100644 --- a/tools/cfl/internal/cmd/page/create.go +++ b/tools/cfl/internal/cmd/page/create.go @@ -183,12 +183,13 @@ func runCreate(ctx context.Context, opts *createOptions) error { // A create writes the same verbatim body an edit does, so it can suffer // the same unseen server-side loss. return verifyStoredBody(ctx, verifyRequest{ - opts: opts.Options, - client: client, - pageID: page.ID, - bodyFormat: bodyFormat, - sentContent: sentContent, - enabled: !opts.noVerify, + opts: opts.Options, + client: client, + pageID: page.ID, + bodyFormat: bodyFormat, + sentContent: sentContent, + storageBefore: "", + enabled: !opts.noVerify, }) } diff --git a/tools/cfl/internal/cmd/page/edit.go b/tools/cfl/internal/cmd/page/edit.go index f703021..218528c 100644 --- a/tools/cfl/internal/cmd/page/edit.go +++ b/tools/cfl/internal/cmd/page/edit.go @@ -215,6 +215,13 @@ func runEdit(ctx context.Context, opts *editOptions) error { req.Body = existingPage.Body } + // Captured before the write: the storage body is what reveals loss the + // caller inherited from a lossy read of their own. + var storageBefore string + if verificationApplies(bodyFormat, hasNewContent, opts.noVerify) { + storageBefore, _ = readStorageBody(ctx, client, opts.pageID) + } + page, err := client.UpdatePage(ctx, opts.pageID, req) if err != nil { return err @@ -231,12 +238,13 @@ func runEdit(ctx context.Context, opts *editOptions) error { } return verifyStoredBody(ctx, verifyRequest{ - opts: opts.Options, - client: client, - pageID: opts.pageID, - bodyFormat: bodyFormat, - sentContent: sentContent, - enabled: hasNewContent && !opts.noVerify, + opts: opts.Options, + client: client, + pageID: opts.pageID, + bodyFormat: bodyFormat, + sentContent: sentContent, + storageBefore: storageBefore, + enabled: hasNewContent && !opts.noVerify, }) } diff --git a/tools/cfl/internal/cmd/page/verify.go b/tools/cfl/internal/cmd/page/verify.go index a56481f..0c842b4 100644 --- a/tools/cfl/internal/cmd/page/verify.go +++ b/tools/cfl/internal/cmd/page/verify.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "regexp" "sort" "strings" @@ -355,6 +356,20 @@ type verifyRequest struct { bodyFormat string sentContent string enabled bool + // storageBefore is the page's storage body read before the write. It is + // the representation that survives an ADF round trip, so it is what + // reveals loss the caller inherited from their own read. + storageBefore string +} + +// verificationApplies reports whether a write will be verified. Markdown is +// converted before sending, so the stored body is not comparable to what the +// caller supplied and no read is worth paying for. +func verificationApplies(bodyFormat string, hasNewContent, noVerify bool) bool { + if !hasNewContent || noVerify { + return false + } + return bodyFormat == bodyFormatADF || bodyFormat == bodyFormatXHTML } // verifyStoredBody re-reads a page after a write and reports what Confluence @@ -385,7 +400,8 @@ func verifyStoredBody(ctx context.Context, req verifyRequest) error { if err != nil { return fmt.Errorf("verifying stored page: %w", err) } - if drift.Clean() { + lostElements := verifyStorageLoss(ctx, req) + if drift.Clean() && len(lostElements) == 0 { return nil } @@ -402,6 +418,7 @@ func verifyStoredBody(ctx context.Context, req verifyRequest) error { AtomChanges: drift.AtomChanges, DroppedAttrs: drift.DroppedAttrs, AddedAttrs: drift.AddedAttrs, + LostElements: lostElements, } if emitErr := cflpresent.Emit(req.opts, cflpresent.PagePresenter{}.PresentWriteDrift(finding)); emitErr != nil { return emitErr @@ -463,3 +480,64 @@ func readableExcerpt(s string, at int) string { } return string(r[at:end]) } + +// Comparing what was sent against what was stored cannot see loss the caller +// inherited from their own read. Confluence's atlas_doc_format representation +// of a page does not always carry marks its storage representation does — em +// and strong wrapping a code span in a table cell are dropped — so reading a +// page as ADF and writing it straight back destroys them, and both sides of a +// sent-versus-stored comparison carry the loss equally. +// +// The storage representation is the one that survives the round trip, so it +// is the one worth watching. Element types whose counts fall across a write +// are reported: text edits move no element counts, while losing emphasis, a +// code span, or a link is collateral a caller almost never intends. + +var storageElementRE = regexp.MustCompile(`<(\w[\w-]*)[\s/>]`) + +// storageProfile counts elements in a storage-format body. +func storageProfile(body string) map[string]int { + profile := map[string]int{} + for _, m := range storageElementRE.FindAllStringSubmatch(body, -1) { + profile[m[1]]++ + } + return profile +} + +// diffStorageLoss names element types that became less frequent, in sorted +// order. Additions are not reported: adding content is what an edit is for. +func diffStorageLoss(before, after map[string]int) []string { + var lost []string + for name, n := range before { + if after[name] < n { + lost = append(lost, fmt.Sprintf("%s (%d→%d)", name, n, after[name])) + } + } + sort.Strings(lost) + return lost +} + +// verifyStorageLoss compares the page's storage body across the write. An +// empty result means either nothing was lost or the comparison could not be +// made; it never reports loss it did not observe. +func verifyStorageLoss(ctx context.Context, req verifyRequest) []string { + if req.storageBefore == "" { + return nil + } + after, err := readStorageBody(ctx, req.client, req.pageID) + if err != nil || after == "" { + return nil + } + return diffStorageLoss(storageProfile(req.storageBefore), storageProfile(after)) +} + +// readStorageBody fetches a page's storage representation, or "" when it is +// unavailable. Callers treat that as "cannot compare" rather than as "no +// loss", so a failure here never manufactures a clean result. +func readStorageBody(ctx context.Context, client *api.Client, pageID string) (string, error) { + page, err := client.GetPage(ctx, pageID, &api.GetPageOptions{BodyFormat: apiBodyFormat(bodyFormatXHTML)}) + if err != nil { + return "", err + } + return bodyValue(page, bodyFormatXHTML), nil +} diff --git a/tools/cfl/internal/cmd/page/verify_test.go b/tools/cfl/internal/cmd/page/verify_test.go index 1e2c99d..5efd380 100644 --- a/tools/cfl/internal/cmd/page/verify_test.go +++ b/tools/cfl/internal/cmd/page/verify_test.go @@ -544,3 +544,83 @@ func TestAttributelessAtomChangeIsNamed(t *testing.T) { t.Errorf("report must name the changed atom when no attributes can:\n%s", report) } } + +// The loss this exists to catch: a no-op ADF round trip that Confluence +// stores without the emphasis marks its storage body carried. Sent and +// stored agree, so only the storage comparison sees it. +func TestStorageLossDetectedWhenSentAndStoredAgree(t *testing.T) { + before := `

KEY

bold

` + after := `

KEY

bold

` + lost := diffStorageLoss(storageProfile(before), storageProfile(after)) + if len(lost) != 2 { + t.Fatalf("lost = %v, want em and strong", lost) + } + joined := strings.Join(lost, " ") + if !strings.Contains(joined, "em (1→0)") || !strings.Contains(joined, "strong (2→1)") { + t.Errorf("unexpected loss report: %v", lost) + } +} + +// Editing text moves no element counts, so an ordinary edit stays quiet. +func TestStorageLossQuietOnTextOnlyEdit(t *testing.T) { + before := `

hello world

` + after := `

goodbye world

` + if lost := diffStorageLoss(storageProfile(before), storageProfile(after)); len(lost) != 0 { + t.Errorf("text-only edit reported loss: %v", lost) + } +} + +// Adding content is what an edit is for; only losses are reported. +func TestStorageLossIgnoresAdditions(t *testing.T) { + before := `

hello

` + after := `

hello

new

` + if lost := diffStorageLoss(storageProfile(before), storageProfile(after)); len(lost) != 0 { + t.Errorf("additions reported as loss: %v", lost) + } +} + +// A missing baseline must read as "cannot compare", never as "no loss". +func TestStorageLossNotClaimedWithoutBaseline(t *testing.T) { + req := verifyRequest{storageBefore: ""} + if lost := verifyStorageLoss(context.Background(), req); lost != nil { + t.Errorf("claimed a comparison with no baseline: %v", lost) + } +} + +func TestVerificationApplies(t *testing.T) { + tests := []struct { + format string + hasNewContent, noVer bool + want bool + }{ + {bodyFormatADF, true, false, true}, + {bodyFormatXHTML, true, false, true}, + {bodyFormatMarkdown, true, false, false}, + {bodyFormatADF, true, true, false}, + {bodyFormatADF, false, false, false}, + } + for _, tc := range tests { + if got := verificationApplies(tc.format, tc.hasNewContent, tc.noVer); got != tc.want { + t.Errorf("verificationApplies(%q,%v,%v) = %v, want %v", tc.format, tc.hasNewContent, tc.noVer, got, tc.want) + } + } +} + +func TestPresentedStorageLossExplainsTheCause(t *testing.T) { + model := cflpresent.PagePresenter{}.PresentWriteDrift(cflpresent.WriteDrift{ + BodyFormat: bodyFormatADF, + LostElements: []string{"em (3→1)", "strong (3→1)"}, + }) + var b strings.Builder + for _, sec := range model.Sections { + if msg, ok := sec.(*sharedpresent.MessageSection); ok { + b.WriteString(msg.Message) + } + } + out := b.String() + for _, want := range []string{"lost formatting", "em (3→1)", "writing it back"} { + if !strings.Contains(out, want) { + t.Errorf("report missing %q:\n%s", want, out) + } + } +} diff --git a/tools/cfl/internal/present/mutation.go b/tools/cfl/internal/present/mutation.go index 0f95a39..f01b1c4 100644 --- a/tools/cfl/internal/present/mutation.go +++ b/tools/cfl/internal/present/mutation.go @@ -144,6 +144,9 @@ type WriteDrift struct { AtomChanges []string DroppedAttrs []string AddedAttrs []string + // LostElements names storage element types that became less frequent + // across the write. + LostElements []string } // attrLines lists attribute changes, which identify the embedded content @@ -190,6 +193,13 @@ func describeContentChange(d WriteDrift) string { // other means it landed in a document the server tidied. func (PagePresenter) PresentWriteDrift(d WriteDrift) *sharedpresent.OutputModel { var lines []string + if len(d.LostElements) > 0 { + lines = append(lines, "The stored page lost formatting that was present before this write:") + for _, e := range d.LostElements { + lines = append(lines, " - "+e) + } + lines = append(lines, "Reading a page as ADF and writing it back does not always preserve marks its storage form carries. Compare against the storage body before assuming the change was clean.", "") + } if d.TextChanged { lines = append(lines, fmt.Sprintf("Stored %s body does not match what was sent: %s.", d.BodyFormat, describeContentChange(d)), From c1da2f0ba335230d6c77d9eee9272e59c0b1e122 Mon Sep 17 00:00:00 2001 From: piekstra Date: Fri, 14 Aug 2026 09:45:12 -0400 Subject: [PATCH 2/4] fix(cfl): INT-732 state when the storage comparison could not run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round. The baseline read discarded its error, so a failed read produced exactly the output of a clean write and the check silently stopped existing — the shape of failure this change was written to remove. The comparison now reports whether it ran, and says why when it could not. Storage keeps macro bodies verbatim inside CDATA and carries comments, where angle brackets are content rather than markup; both are removed before profiling so they cannot invent losses. The cause is chosen by the format actually used. Blaming an ADF round trip for a storage write was asserting something not established: an operator piping storage markup through sed lost those elements themselves. An xhtml write already reads the stored body back, so that body is reused instead of fetching it a second time. verificationApplies now owns the decision at both call sites rather than being one of three places encoding it. Documents the new stderr blocks in OUTPUT_SPEC.md. [INT-732] --- tools/cfl/internal/cmd/OUTPUT_SPEC.md | 21 +++++ tools/cfl/internal/cmd/page/create.go | 2 +- tools/cfl/internal/cmd/page/edit.go | 27 ++++-- tools/cfl/internal/cmd/page/verify.go | 95 +++++++++++++++------- tools/cfl/internal/cmd/page/verify_test.go | 77 ++++++++++++++++-- tools/cfl/internal/present/mutation.go | 19 ++++- 6 files changed, 197 insertions(+), 44 deletions(-) diff --git a/tools/cfl/internal/cmd/OUTPUT_SPEC.md b/tools/cfl/internal/cmd/OUTPUT_SPEC.md index dcfafa8..54e94ff 100644 --- a/tools/cfl/internal/cmd/OUTPUT_SPEC.md +++ b/tools/cfl/internal/cmd/OUTPUT_SPEC.md @@ -270,6 +270,27 @@ Confluence normalized the stored body. Content is intact; these attribu - .attrs. (→) ``` +Formatting the stored page no longer carries is reported before the other +blocks, and the command still succeeds — the write landed, only formatting +was collateral: + +```text +The stored page lost formatting that was present before this write: + - (→) + Compare against the storage body before assuming the change was clean. +``` + +`` names why loss is possible for the format in use: an ADF round trip +does not always preserve marks the storage body carries, whereas a storage +write carries only what the caller submitted. + +When the comparison could not be made at all, that is stated rather than +passed over in silence, because silence is what a clean write looks like: + +```text +Could not compare the stored page against its state before the write, so formatting loss would not have been noticed. () +``` + Attributes the server added rather than dropped are reported the same way: ```text diff --git a/tools/cfl/internal/cmd/page/create.go b/tools/cfl/internal/cmd/page/create.go index e4dfea7..39aa393 100644 --- a/tools/cfl/internal/cmd/page/create.go +++ b/tools/cfl/internal/cmd/page/create.go @@ -189,7 +189,7 @@ func runCreate(ctx context.Context, opts *createOptions) error { bodyFormat: bodyFormat, sentContent: sentContent, storageBefore: "", - enabled: !opts.noVerify, + enabled: verificationApplies(bodyFormat, true, opts.noVerify), }) } diff --git a/tools/cfl/internal/cmd/page/edit.go b/tools/cfl/internal/cmd/page/edit.go index 218528c..741745b 100644 --- a/tools/cfl/internal/cmd/page/edit.go +++ b/tools/cfl/internal/cmd/page/edit.go @@ -217,9 +217,17 @@ func runEdit(ctx context.Context, opts *editOptions) error { // Captured before the write: the storage body is what reveals loss the // caller inherited from a lossy read of their own. - var storageBefore string + var storageBefore, storageBeforeErr string if verificationApplies(bodyFormat, hasNewContent, opts.noVerify) { - storageBefore, _ = readStorageBody(ctx, client, opts.pageID) + body, err := readStorageBody(ctx, client, opts.pageID) + switch { + case err != nil: + storageBeforeErr = err.Error() + case body == "": + storageBeforeErr = "the page returned no storage body" + default: + storageBefore = body + } } page, err := client.UpdatePage(ctx, opts.pageID, req) @@ -238,13 +246,14 @@ func runEdit(ctx context.Context, opts *editOptions) error { } return verifyStoredBody(ctx, verifyRequest{ - opts: opts.Options, - client: client, - pageID: opts.pageID, - bodyFormat: bodyFormat, - sentContent: sentContent, - storageBefore: storageBefore, - enabled: hasNewContent && !opts.noVerify, + opts: opts.Options, + client: client, + pageID: opts.pageID, + bodyFormat: bodyFormat, + sentContent: sentContent, + storageBefore: storageBefore, + storageBeforeErr: storageBeforeErr, + enabled: verificationApplies(bodyFormat, hasNewContent, opts.noVerify), }) } diff --git a/tools/cfl/internal/cmd/page/verify.go b/tools/cfl/internal/cmd/page/verify.go index 0c842b4..6914e5f 100644 --- a/tools/cfl/internal/cmd/page/verify.go +++ b/tools/cfl/internal/cmd/page/verify.go @@ -360,6 +360,10 @@ type verifyRequest struct { // the representation that survives an ADF round trip, so it is what // reveals loss the caller inherited from their own read. storageBefore string + // storageBeforeErr explains a missing baseline so the operator learns + // the check could not run, rather than seeing the silence of a clean + // write. + storageBeforeErr string } // verificationApplies reports whether a write will be verified. Markdown is @@ -383,9 +387,6 @@ func verifyStoredBody(ctx context.Context, req verifyRequest) error { if !req.enabled || req.sentContent == "" { return nil } - if req.bodyFormat != bodyFormatADF && req.bodyFormat != bodyFormatXHTML { - return nil - } stored, err := getPageWithBodyFormat(ctx, req.client, req.pageID, req.bodyFormat) if err != nil { @@ -400,25 +401,33 @@ func verifyStoredBody(ctx context.Context, req verifyRequest) error { if err != nil { return fmt.Errorf("verifying stored page: %w", err) } - lostElements := verifyStorageLoss(ctx, req) - if drift.Clean() && len(lostElements) == 0 { + // An xhtml write already read the storage body back; reuse it. + reuse := "" + if req.bodyFormat == bodyFormatXHTML { + reuse = storedContent + } + storage := compareStorage(ctx, req, reuse) + if drift.Clean() && len(storage.Lost) == 0 && !storage.Unavailable { return nil } off := diffOffset(drift.SentVisible, drift.StoredVisible) finding := cflpresent.WriteDrift{ - BodyFormat: req.bodyFormat, - TextChanged: drift.TextChanged, - VisibleSent: drift.VisibleSent, - VisibleStored: drift.VisibleStored, - AtomsChanged: drift.AtomsChanged, - DiffOffset: off, - SentExcerpt: readableExcerpt(drift.SentVisible, off), - StoredExcerpt: readableExcerpt(drift.StoredVisible, off), - AtomChanges: drift.AtomChanges, - DroppedAttrs: drift.DroppedAttrs, - AddedAttrs: drift.AddedAttrs, - LostElements: lostElements, + BodyFormat: req.bodyFormat, + TextChanged: drift.TextChanged, + VisibleSent: drift.VisibleSent, + VisibleStored: drift.VisibleStored, + AtomsChanged: drift.AtomsChanged, + DiffOffset: off, + SentExcerpt: readableExcerpt(drift.SentVisible, off), + StoredExcerpt: readableExcerpt(drift.StoredVisible, off), + AtomChanges: drift.AtomChanges, + DroppedAttrs: drift.DroppedAttrs, + AddedAttrs: drift.AddedAttrs, + LostElements: storage.Lost, + StorageUncomparable: storage.Unavailable, + StorageReason: storage.Reason, + LossCause: storageLossCause(req.bodyFormat), } if emitErr := cflpresent.Emit(req.opts, cflpresent.PagePresenter{}.PresentWriteDrift(finding)); emitErr != nil { return emitErr @@ -495,10 +504,14 @@ func readableExcerpt(s string, at int) string { var storageElementRE = regexp.MustCompile(`<(\w[\w-]*)[\s/>]`) -// storageProfile counts elements in a storage-format body. +var storageInertRE = regexp.MustCompile(`(?s)|`) + +// storageProfile counts elements in a storage-format body. Comment and CDATA +// spans are removed first: storage format carries macro bodies verbatim +// inside CDATA, and angle brackets there are content, not markup. func storageProfile(body string) map[string]int { profile := map[string]int{} - for _, m := range storageElementRE.FindAllStringSubmatch(body, -1) { + for _, m := range storageElementRE.FindAllStringSubmatch(storageInertRE.ReplaceAllString(body, ""), -1) { profile[m[1]]++ } return profile @@ -517,18 +530,44 @@ func diffStorageLoss(before, after map[string]int) []string { return lost } -// verifyStorageLoss compares the page's storage body across the write. An -// empty result means either nothing was lost or the comparison could not be -// made; it never reports loss it did not observe. -func verifyStorageLoss(ctx context.Context, req verifyRequest) []string { +// storageLossCause names why formatting can vanish for the format in use. +// An ADF round trip drops marks Confluence's storage body carries; a storage +// write loses only what the caller's own pipeline removed. +func storageLossCause(bodyFormat string) string { + if bodyFormat == bodyFormatADF { + return "Reading a page as ADF and writing it back does not always preserve marks its storage form carries." + } + return "The submitted storage body did not carry these elements." +} + +// storageComparison reports what the storage bodies showed, and says so when +// they could not be compared at all. A silent nil would be indistinguishable +// from a clean write, which is the failure this check exists to remove. +type storageComparison struct { + Lost []string + Unavailable bool + Reason string +} + +// compareStorage diffs the page's storage body across the write. storedAfter +// may be supplied by a caller that already read it, so an xhtml write does +// not pay for the same fetch twice. +func compareStorage(ctx context.Context, req verifyRequest, storedAfter string) storageComparison { if req.storageBefore == "" { - return nil + return storageComparison{Unavailable: true, Reason: req.storageBeforeErr} } - after, err := readStorageBody(ctx, req.client, req.pageID) - if err != nil || after == "" { - return nil + after := storedAfter + if after == "" { + body, err := readStorageBody(ctx, req.client, req.pageID) + if err != nil { + return storageComparison{Unavailable: true, Reason: err.Error()} + } + after = body + } + if after == "" { + return storageComparison{Unavailable: true, Reason: "the page returned no storage body"} } - return diffStorageLoss(storageProfile(req.storageBefore), storageProfile(after)) + return storageComparison{Lost: diffStorageLoss(storageProfile(req.storageBefore), storageProfile(after))} } // readStorageBody fetches a page's storage representation, or "" when it is diff --git a/tools/cfl/internal/cmd/page/verify_test.go b/tools/cfl/internal/cmd/page/verify_test.go index 5efd380..a3c4964 100644 --- a/tools/cfl/internal/cmd/page/verify_test.go +++ b/tools/cfl/internal/cmd/page/verify_test.go @@ -579,11 +579,55 @@ func TestStorageLossIgnoresAdditions(t *testing.T) { } } -// A missing baseline must read as "cannot compare", never as "no loss". -func TestStorageLossNotClaimedWithoutBaseline(t *testing.T) { - req := verifyRequest{storageBefore: ""} - if lost := verifyStorageLoss(context.Background(), req); lost != nil { - t.Errorf("claimed a comparison with no baseline: %v", lost) +// A missing baseline must be reported as "cannot compare", never pass as +// silence: silence is what a clean write looks like. +func TestStorageComparisonUnavailableWithoutBaseline(t *testing.T) { + req := verifyRequest{storageBefore: "", storageBeforeErr: "resource not found"} + got := compareStorage(context.Background(), req, "") + if !got.Unavailable { + t.Error("a missing baseline was not reported as uncomparable") + } + if got.Reason != "resource not found" { + t.Errorf("Reason = %q, want the captured cause", got.Reason) + } + if len(got.Lost) != 0 { + t.Errorf("claimed loss without a baseline: %v", got.Lost) + } +} + +// An xhtml write already read the stored body back; reuse it rather than +// fetching the same thing twice. +func TestStorageComparisonReusesSuppliedBody(t *testing.T) { + req := verifyRequest{storageBefore: `

a

`} + got := compareStorage(context.Background(), req, `

a

`) + if got.Unavailable { + t.Fatal("reported uncomparable despite a supplied body") + } + if len(got.Lost) != 1 || !strings.Contains(got.Lost[0], "strong") { + t.Errorf("Lost = %v, want the dropped strong", got.Lost) + } +} + +// Storage keeps macro bodies verbatim inside CDATA; angle brackets there are +// content, and counting them would invent losses that never happened. +func TestStorageProfileIgnoresCDATAAndComments(t *testing.T) { + body := `

x

not markup]]>` + p := storageProfile(body) + if p["strong"] != 0 || p["em"] != 0 { + t.Errorf("counted markup inside CDATA/comments: %v", p) + } + if p["p"] != 1 { + t.Errorf("real elements not counted: %v", p) + } +} + +// The stated cause must match the format actually used. +func TestStorageLossCauseMatchesFormat(t *testing.T) { + if !strings.Contains(storageLossCause(bodyFormatADF), "ADF") { + t.Error("adf cause should name the ADF round trip") + } + if strings.Contains(storageLossCause(bodyFormatXHTML), "ADF") { + t.Error("an xhtml write must not be blamed on the ADF round trip") } } @@ -610,6 +654,7 @@ func TestPresentedStorageLossExplainsTheCause(t *testing.T) { model := cflpresent.PagePresenter{}.PresentWriteDrift(cflpresent.WriteDrift{ BodyFormat: bodyFormatADF, LostElements: []string{"em (3→1)", "strong (3→1)"}, + LossCause: storageLossCause(bodyFormatADF), }) var b strings.Builder for _, sec := range model.Sections { @@ -624,3 +669,25 @@ func TestPresentedStorageLossExplainsTheCause(t *testing.T) { } } } + +// A comparison that could not run must say so; silence is what a clean write +// looks like, and the two must never be confused. +func TestPresentedUncomparableStorageIsStated(t *testing.T) { + model := cflpresent.PagePresenter{}.PresentWriteDrift(cflpresent.WriteDrift{ + BodyFormat: bodyFormatADF, + StorageUncomparable: true, + StorageReason: "resource not found", + }) + var b strings.Builder + for _, sec := range model.Sections { + if msg, ok := sec.(*sharedpresent.MessageSection); ok { + b.WriteString(msg.Message) + } + } + out := b.String() + for _, want := range []string{"Could not compare", "would not have been noticed", "resource not found"} { + if !strings.Contains(out, want) { + t.Errorf("report missing %q:\n%s", want, out) + } + } +} diff --git a/tools/cfl/internal/present/mutation.go b/tools/cfl/internal/present/mutation.go index f01b1c4..235854f 100644 --- a/tools/cfl/internal/present/mutation.go +++ b/tools/cfl/internal/present/mutation.go @@ -147,6 +147,12 @@ type WriteDrift struct { // LostElements names storage element types that became less frequent // across the write. LostElements []string + // StorageUncomparable reports that the check could not run; silence + // would otherwise be indistinguishable from a clean write. + StorageUncomparable bool + StorageReason string + // LossCause explains why formatting can vanish for the format in use. + LossCause string } // attrLines lists attribute changes, which identify the embedded content @@ -193,12 +199,23 @@ func describeContentChange(d WriteDrift) string { // other means it landed in a document the server tidied. func (PagePresenter) PresentWriteDrift(d WriteDrift) *sharedpresent.OutputModel { var lines []string + if d.StorageUncomparable { + msg := "Could not compare the stored page against its state before the write, so formatting loss would not have been noticed." + if d.StorageReason != "" { + msg += " (" + d.StorageReason + ")" + } + lines = append(lines, msg, "") + } if len(d.LostElements) > 0 { lines = append(lines, "The stored page lost formatting that was present before this write:") for _, e := range d.LostElements { lines = append(lines, " - "+e) } - lines = append(lines, "Reading a page as ADF and writing it back does not always preserve marks its storage form carries. Compare against the storage body before assuming the change was clean.", "") + tail := "Compare against the storage body before assuming the change was clean." + if d.LossCause != "" { + tail = d.LossCause + " " + tail + } + lines = append(lines, tail, "") } if d.TextChanged { lines = append(lines, From 44bc3fb34fb269a7af87b785b47e53bf2104f77f Mon Sep 17 00:00:00 2001 From: piekstra Date: Fri, 14 Aug 2026 09:54:06 -0400 Subject: [PATCH 3/4] fix(cfl): INT-732 do not treat a create as a failed comparison Review round. A create has no prior state, but an empty baseline was read as a failed comparison, so every clean create with an exact body format announced that formatting loss would not have been noticed. A warning that fires on the happy path teaches the operator to ignore it, which costs more than the warning was worth. The element pattern could not match a namespaced name, so macros and attachment references were not merely miscounted but invisible: removing an ac:structured-macro moved nothing the profile could see. Namespace prefixes are matched now, and the comment says what the profile does and does not cover rather than describing only the CDATA handling. The pre-write baseline re-fetched a body the command already held. The non-editor path fetches the page with the storage representation, so that value is used and the extra read only happens when it is genuinely absent. [INT-732] --- tools/cfl/internal/cmd/page/edit.go | 38 +++++++++++--------- tools/cfl/internal/cmd/page/verify.go | 13 ++++++- tools/cfl/internal/cmd/page/verify_test.go | 41 ++++++++++++++++++++-- 3 files changed, 73 insertions(+), 19 deletions(-) diff --git a/tools/cfl/internal/cmd/page/edit.go b/tools/cfl/internal/cmd/page/edit.go index 741745b..744ca07 100644 --- a/tools/cfl/internal/cmd/page/edit.go +++ b/tools/cfl/internal/cmd/page/edit.go @@ -217,16 +217,21 @@ func runEdit(ctx context.Context, opts *editOptions) error { // Captured before the write: the storage body is what reveals loss the // caller inherited from a lossy read of their own. + // The page was already fetched above, and the non-editor path asks for + // the storage representation, so the baseline is usually in hand. var storageBefore, storageBeforeErr string if verificationApplies(bodyFormat, hasNewContent, opts.noVerify) { - body, err := readStorageBody(ctx, client, opts.pageID) - switch { - case err != nil: - storageBeforeErr = err.Error() - case body == "": - storageBeforeErr = "the page returned no storage body" - default: - storageBefore = body + storageBefore = bodyValue(existingPage, bodyFormatXHTML) + if storageBefore == "" { + body, err := readStorageBody(ctx, client, opts.pageID) + switch { + case err != nil: + storageBeforeErr = err.Error() + case body == "": + storageBeforeErr = "the page returned no storage body" + default: + storageBefore = body + } } } @@ -246,14 +251,15 @@ func runEdit(ctx context.Context, opts *editOptions) error { } return verifyStoredBody(ctx, verifyRequest{ - opts: opts.Options, - client: client, - pageID: opts.pageID, - bodyFormat: bodyFormat, - sentContent: sentContent, - storageBefore: storageBefore, - storageBeforeErr: storageBeforeErr, - enabled: verificationApplies(bodyFormat, hasNewContent, opts.noVerify), + opts: opts.Options, + client: client, + pageID: opts.pageID, + bodyFormat: bodyFormat, + sentContent: sentContent, + storageBefore: storageBefore, + storageBeforeErr: storageBeforeErr, + comparePriorState: true, + enabled: verificationApplies(bodyFormat, hasNewContent, opts.noVerify), }) } diff --git a/tools/cfl/internal/cmd/page/verify.go b/tools/cfl/internal/cmd/page/verify.go index 6914e5f..ffb9398 100644 --- a/tools/cfl/internal/cmd/page/verify.go +++ b/tools/cfl/internal/cmd/page/verify.go @@ -364,6 +364,10 @@ type verifyRequest struct { // the check could not run, rather than seeing the silence of a clean // write. storageBeforeErr string + // comparePriorState is false when there is no prior state to compare + // against, as on a create. That is not a failed comparison and must not + // be reported as one. + comparePriorState bool } // verificationApplies reports whether a write will be verified. Markdown is @@ -502,7 +506,11 @@ func readableExcerpt(s string, at int) string { // are reported: text edits move no element counts, while losing emphasis, a // code span, or a link is collateral a caller almost never intends. -var storageElementRE = regexp.MustCompile(`<(\w[\w-]*)[\s/>]`) +// Storage format is XHTML with namespaced elements: macros arrive as +// , attachment references as . The +// namespace prefix is part of the name, so it has to be matched or a lost +// macro is a loss the profile cannot see. +var storageElementRE = regexp.MustCompile(`<([A-Za-z][\w-]*(?::[\w-]+)?)[\s/>]`) var storageInertRE = regexp.MustCompile(`(?s)|`) @@ -553,6 +561,9 @@ type storageComparison struct { // may be supplied by a caller that already read it, so an xhtml write does // not pay for the same fetch twice. func compareStorage(ctx context.Context, req verifyRequest, storedAfter string) storageComparison { + if !req.comparePriorState { + return storageComparison{} + } if req.storageBefore == "" { return storageComparison{Unavailable: true, Reason: req.storageBeforeErr} } diff --git a/tools/cfl/internal/cmd/page/verify_test.go b/tools/cfl/internal/cmd/page/verify_test.go index a3c4964..30e14f9 100644 --- a/tools/cfl/internal/cmd/page/verify_test.go +++ b/tools/cfl/internal/cmd/page/verify_test.go @@ -582,7 +582,7 @@ func TestStorageLossIgnoresAdditions(t *testing.T) { // A missing baseline must be reported as "cannot compare", never pass as // silence: silence is what a clean write looks like. func TestStorageComparisonUnavailableWithoutBaseline(t *testing.T) { - req := verifyRequest{storageBefore: "", storageBeforeErr: "resource not found"} + req := verifyRequest{storageBefore: "", storageBeforeErr: "resource not found", comparePriorState: true} got := compareStorage(context.Background(), req, "") if !got.Unavailable { t.Error("a missing baseline was not reported as uncomparable") @@ -598,7 +598,7 @@ func TestStorageComparisonUnavailableWithoutBaseline(t *testing.T) { // An xhtml write already read the stored body back; reuse it rather than // fetching the same thing twice. func TestStorageComparisonReusesSuppliedBody(t *testing.T) { - req := verifyRequest{storageBefore: `

a

`} + req := verifyRequest{storageBefore: `

a

`, comparePriorState: true} got := compareStorage(context.Background(), req, `

a

`) if got.Unavailable { t.Fatal("reported uncomparable despite a supplied body") @@ -691,3 +691,40 @@ func TestPresentedUncomparableStorageIsStated(t *testing.T) { } } } + +// A create has no prior state. That is not a failed comparison, and saying +// so on every clean create would train the operator to ignore the warning. +func TestStorageComparisonSilentWhenThereIsNoPriorState(t *testing.T) { + req := verifyRequest{storageBefore: "", comparePriorState: false} + got := compareStorage(context.Background(), req, "") + if got.Unavailable { + t.Error("a create was reported as a failed comparison") + } + if len(got.Lost) != 0 { + t.Errorf("Lost = %v, want none", got.Lost) + } +} + +// Storage carries namespaced elements; a lost macro must not be invisible. +func TestStorageProfileCountsNamespacedElements(t *testing.T) { + body := `

x

y

` + p := storageProfile(body) + for _, want := range []string{"ac:structured-macro", "ac:rich-text-body", "ri:attachment"} { + if p[want] != 1 { + t.Errorf("%s not counted: %v", want, p) + } + } + if p["p"] != 2 { + t.Errorf("plain elements miscounted: %v", p) + } +} + +// The loss that motivated namespacing: a macro removed by a write. +func TestStorageLossDetectsRemovedMacro(t *testing.T) { + before := `

x

y

` + after := `

x

` + lost := strings.Join(diffStorageLoss(storageProfile(before), storageProfile(after)), " ") + if !strings.Contains(lost, "ac:structured-macro") { + t.Errorf("a removed macro was not reported: %s", lost) + } +} From 5e32dda77dc8d977cae461dead808a6b58087a7f Mon Sep 17 00:00:00 2001 From: piekstra Date: Fri, 14 Aug 2026 09:59:21 -0400 Subject: [PATCH 4/4] test(cfl): INT-732 pin the read count on the verified path Both request-dedup fixes were unprotected: the unit test exercises compareStorage directly, and the runEdit tests on the verified xhtml path discarded the request counter, so only the skip paths pinned a count. A future change could have reintroduced either extra read without a test noticing. Also corrects the page create section of OUTPUT_SPEC, which forwarded the reader to page edit for blocks a create cannot emit now that it is not treated as a failed comparison. [INT-732] --- tools/cfl/internal/cmd/OUTPUT_SPEC.md | 7 +++++-- tools/cfl/internal/cmd/page/verify_test.go | 20 ++++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/tools/cfl/internal/cmd/OUTPUT_SPEC.md b/tools/cfl/internal/cmd/OUTPUT_SPEC.md index 54e94ff..5a44c79 100644 --- a/tools/cfl/internal/cmd/OUTPUT_SPEC.md +++ b/tools/cfl/internal/cmd/OUTPUT_SPEC.md @@ -246,8 +246,11 @@ URL: ``` With `--body-format adf` or `xhtml` the page is read back after the write and -compared with what was sent, exactly as `page edit` does. See that section for -the stderr blocks emitted on normalization and on content loss. +compared with what was sent. See the `page edit` section for the stderr blocks +emitted on normalization and on content loss. + +A create has no state to compare against, so the formatting-loss and +comparison-unavailable blocks described there are never emitted here. ## `page edit ` diff --git a/tools/cfl/internal/cmd/page/verify_test.go b/tools/cfl/internal/cmd/page/verify_test.go index 30e14f9..a2d55fa 100644 --- a/tools/cfl/internal/cmd/page/verify_test.go +++ b/tools/cfl/internal/cmd/page/verify_test.go @@ -728,3 +728,23 @@ func TestStorageLossDetectsRemovedMacro(t *testing.T) { t.Errorf("a removed macro was not reported: %s", lost) } } + +// The baseline comes from the page already fetched, and the xhtml readback +// is reused for the storage comparison. Both were review findings; without a +// count pinned on the verified path, either could be reintroduced silently. +func TestRunEditVerifiedPathMakesNoRedundantReads(t *testing.T) { + srv, gets := driftServer(t, func(map[string]any) string { + return `{"storage":{"value":"

what we sent

"}}` + }) + defer srv.Close() + + if err := runEdit(context.Background(), editOptsFor(t, srv, "

what we sent

", false)); err != nil { + t.Fatalf("verified edit: %v", err) + } + // One read before the write, one to read the result back. A third would + // mean the baseline or the storage comparison re-fetched what the + // command already had. + if *gets != 2 { + t.Errorf("GET count = %d, want 2 (pre-write fetch + post-write readback)", *gets) + } +}