Skip to content

fix(cfl): stop macro parameter order failing a clean write - #480

Merged
piekstra merged 4 commits into
mainfrom
piekstra/cfl-macro-parameter-order
Aug 18, 2026
Merged

fix(cfl): stop macro parameter order failing a clean write#480
piekstra merged 4 commits into
mainfrom
piekstra/cfl-macro-parameter-order

Conversation

@piekstra

Copy link
Copy Markdown
Contributor

Summary

Confluence returns a macro's parameters in an order of its own choosing. The post-write verification strips tags and compares the remaining text, so a parameter's value counted as reader-visible text and a server-side reordering read as rewritten content.

The result was a write refused with a same-length "content differs" mismatch, whose excerpt showed only the parameter values transposed:

sent   "nonewide760... which should produce"
stored "wide760none... which should produce"

The page was stored correctly. Nothing was lost.

Change

Macro parameters are removed from the compared text and diffed as a multiset instead:

  • Reordered parameters now compare equal, so the write passes.
  • Added, dropped or edited parameters still fail the write, exactly as before.
  • The report now names the parameter (macro parameter theme=none (1→0)) instead of pointing at a text offset in a string the caller never wrote.

The strictness is deliberately unchanged. Only the reordering case is newly tolerated; a test covers each of the three outcomes, and each of them fails without the fix.

Why it matters

This guard exists to refuse a write that silently lost content. A guard that also fails correct writes is one people learn to work around, which costs more than the loss it was built to catch.

Testing

go build, go vet, full go test ./... and golangci-lint run all pass. The new test fails without the change:

--- FAIL: TestCompareStoredBodyXHTMLMacroParameterOrder/parameters_reordered_by_the_server
    TextChanged = true, want false
    Clean() = false, want true

Confluence returns a macro's parameters in an order of its own choosing.
The post-write check strips tags and compares the remaining text, so a
parameter value was part of that text and a server-side reordering read as
rewritten content. The write was refused with a same-length mismatch whose
excerpt showed the parameter values transposed.

Parameters are now removed from the compared text and diffed as a
multiset instead, so a reordering compares equal. Adding, dropping or
editing one still fails the write exactly as it did before, and the report
names the parameter rather than pointing at a text offset.

A guard that fails correct writes is one people learn to work around, which
costs more than the loss it was built to catch.

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated PR Review

Reviewed commit: eeabe32077bc
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer Findings
go:implementation-tests 1
policies:conventions 1
architecture:solid-reviewer-agnostic 4
go:implementation-tests (1 finding)

Major - tools/cfl/internal/cmd/page/verify.go:347

macroParameterProfile builds one multiset over the whole document body, not per macro instance, so a parameter can be treated as "reordered" (and the write passed) when it actually moved between two different <ac:structured-macro> blocks with the same name=value pair. E.g. two adjacent code macros that both carry ac:name="theme" set to none would tolerate the server swapping which macro theme=none lands in, or a value being transposed across macros rather than reordered within one — exactly the kind of silent-loss case this verification guard exists to catch. The doc comment on the function and the PR description both frame the tolerance as "a macro's parameters" reordering, but nothing scopes the regex match or the profile to a single macro's parameter list. Given the guard's whole purpose is to refuse writes that lost or rearranged content incorrectly, this is worth scoping the multiset per macro instance (e.g. keyed by the macro's own identity, or diffed macro-by-macro) rather than document-wide, and no test in verify_test.go covers the multi-macro case to pin the intended scope.

policies:conventions (1 finding)

Minor - tools/cfl/internal/cmd/page/verify.go:354

macroParameterProfile feeds DroppedAttrs/AddedAttrs with keys shaped "macro parameter =" (e.g. "macro parameter theme=none"). Those two slices are rendered under the "attributes dropped:"/"attributes added:" headers (present/mutation.go attrLines), which tools/cfl/internal/cmd/OUTPUT_SPEC.md documents for page edit as <node>.attrs.<name> (<before>→<after>) — the format every other producer (adfAttrProfile) follows. This is the first XHTML-path producer of DroppedAttrs/AddedAttrs, and it emits a differently-shaped entry under the same documented header without an OUTPUT_SPEC.md update describing the new shape. Either match the <node>.attrs.<name> convention (e.g. ac:parameter.<name>) or add a line to OUTPUT_SPEC.md's page edit contract documenting the macro-parameter entry shape as a recognized exception.

architecture:solid-reviewer-agnostic (4 findings)

Blocking - tools/cfl/internal/cmd/page/verify.go:338

acParameterRe assumes every <ac:parameter ...> has a closing tag. A self-closing one (valid storage XHTML, and the sent body is caller-authored verbatim) makes the match run from that tag to the next </ac:parameter>, swallowing everything between them. Reproduced on this branch:

b := `<ac:structured-macro ac:name="m"><ac:parameter ac:name="e"/></ac:structured-macro>` +
     `<p>IMPORTANT PARAGRAPH</p>` +
     `<ac:structured-macro ac:name="code"><ac:parameter ac:name="f">v</ac:parameter></ac:structured-macro>`

xhtmlText(b)            // ""  — the paragraph is gone from the compared text
macroParameterProfile(b) // map["macro parameter e=</ac:structured-macro><p>IMPORTANT PARAGRAPH</p>…":1]

Two consequences, both against the contract this guard exists to enforce (U-L1). The reader-visible text it compares — and VisibleSent/VisibleStored, diffOffset and every excerpt derived from it — can lose an arbitrary span of the document, so the numbers reported are no longer facts about the page. And an empty parameter written self-closing but stored expanded (or the reverse) refuses a clean write:

sent   <ac:parameter ac:name="e"/><ac:parameter ac:name="g">x</ac:parameter>
stored <ac:parameter ac:name="e"></ac:parameter><ac:parameter ac:name="g">x</ac:parameter>
Clean() == false, dropped [macro parameter e=<ac:parameter ac:name="g">x (1→0)]

That is the same false-refusal class this PR removes for reordering, reintroduced for empty parameters — and it also collapses the following genuine parameter into a garbage key.

Suggested fix: match the two forms separately, e.g. alternate <ac:parameter\b[^>]*/> (empty value) with <ac:parameter\b[^>]*[^/]>(.*?)</ac:parameter>, and add both forms to the new table test.

Major - tools/cfl/internal/cmd/page/verify.go:80

TextChanged is documented at verify.go:48-50 as "the document's text content differs", and present.PresentWriteDrift words its entire first branch around that meaning. Forcing it true for a parameter-only diff makes that report assert something untrue. Reproduced on this branch (parameter value 760→500, page text identical on both sides):

Stored xhtml body does not match what was sent: content differs at the same length of 50 characters.
The page was updated, but it does not hold the content supplied. Re-read the page before treating the change as applied.
  attributes dropped:
    - macro parameter breakoutWidth=760 (1→0)
  attributes added:
    + macro parameter breakoutWidth=500 (0→1)

The compared text is byte-identical (diffOffset returns -1, which is why the offset line is suppressed), yet the operator is told content differs at a character count. The PR set out to replace an opaque text offset with the parameter's name; the parameter is now named, but the sentence above it still describes a text change that did not happen.

The carrier fields are overloaded the same way: DroppedAttrs/AddedAttrs are documented at verify.go:54-55 as "nodeType.attrName" keys and rendered by the presenter as "attributes dropped"/"attributes added", neither of which is true of macro parameter theme=none. One field now means "tolerated normalization" on the ADF path and "fatal" on the XHTML path (U-S1, U-L1).

Suggested fix: give writeDrift its own ParametersChanged bool plus DroppedParams/AddedParams, have Clean() and the failure check in verifyStoredBody (verify.go:474) consult it, and add a presenter branch worded as macro parameters differing. Same strictness, a report that matches what happened. If the existing fields are kept instead, the doc comments at :54-55 need updating to say what they now carry.

Minor - tools/cfl/internal/cmd/page/verify.go:349

storageProfile strips comments and CDATA with storageInertRE before regex-scanning a storage body, on the stated grounds that angle brackets inside CDATA are content and not markup (verify.go:550-557). The new scan does not follow that established pattern, so markup quoted inside a code macro is read as page configuration:

macroParameterProfile(`<ac:structured-macro ac:name="code"><ac:plain-text-body>` +
  `<![CDATA[<ac:parameter ac:name="x">1</ac:parameter>]]></ac:plain-text-body></ac:structured-macro>`)
// map["macro parameter x=1":1]

Effect is confined to the profile — CDATA text is already absent from the compared text because the tag-depth scanner drops it — so this is cheap to fix and cheap to leave, but it makes two scanners in the same file disagree about what counts as markup, which is the kind of divergence the next reader will not expect. Running storageInertRE.ReplaceAllString (or a shared helper) before the parameter scan keeps them aligned (U-G1).

Minor - tools/cfl/internal/cmd/page/verify.go:354

strings.TrimSpace only trims the ends, whereas the text this value used to travel in is whitespace-collapsed by strings.Join(strings.Fields(...), " ") at verify.go:382. So internal whitespace inside a parameter value is newly load-bearing: <ac:parameter ac:name="t">a b</ac:parameter> against …>a b<… compares equal on main and yields Clean() == false here (verified both ways). That is a strictness increase in a direction the PR description states it did not make ("strictness is deliberately unchanged").

Suggested fix: strings.Join(strings.Fields(m[2]), " ") so a parameter value is normalized exactly as it was when it was compared as part of the text (U-G1).

Reviewer Coverage

  • go:implementation-tests — complete (broad); skipped: none; constraints: none
  • policies:conventions — complete (broad); inspected 1 assigned file (2 inspected across reviewers): tools/cfl/internal/cmd/page/verify.go; skipped: none; constraints: none
  • architecture:solid-reviewer-agnostic — complete (broad); inspected 1 assigned file (2 inspected across reviewers): tools/cfl/internal/cmd/page/verify.go; skipped: none; constraints: Findings 1, 2 and 4 were reproduced by running temporary probe tests against this branch under tools/cfl; the probe files were deleted afterwards and the tree is clean. Scope limited to the assigned file; verify_test.go was read as context only and not reviewed. Whether Confluence itself emits a self-closing <ac:parameter/> was not verified. The sent side is caller-authored verbatim XHTML, which is enough on its own to trigger finding 1. go build ./..., go vet ./... and go test ./... pass in tools/cfl on this branch; golangci-lint was not run.
Inspected files (2)
  • tools/cfl/internal/cmd/page/verify.go
  • tools/cfl/internal/cmd/page/verify_test.go

0 PR discussion threads considered. 0 summarized; 0 resolved.


Completed in 6m 21s | ~$4.04 (est.) | claude-sonnet-5, claude-opus-5 | cr 0.10.286
Field Value
Model claude-sonnet-5, claude-opus-5
Reviewers go:implementation-tests, policies:conventions, architecture:solid-reviewer-agnostic
Engine claude_cli · claude-sonnet-5, claude-opus-5
Reviewed by cr · monit-reviewer
Duration 6m 21s wall · 8m 59s compute
Cost ~$4.04 (est.)
Tokens 82 in / 35.0k out

Per-workstream usage

  • orchestrator-selection — claude-sonnet-5
    • In: 6
    • Out: 2.2k
    • Cache read: 97.5k
    • Cache create: 83.9k
    • Cost: ~$0.38 (est.)
    • Duration: 33s
  • go:implementation-tests — claude-sonnet-5
    • In: 14
    • Out: 6.4k
    • Cache read: 483.0k
    • Cache create: 116.2k
    • Cost: ~$0.68 (est.)
    • Duration: 1m 26s
  • policies:conventions — claude-sonnet-5
    • In: 22
    • Out: 6.1k
    • Cache read: 794.1k
    • Cache create: 107.0k
    • Cost: ~$0.73 (est.)
    • Duration: 1m 25s
  • architecture:solid-reviewer-agnostic — claude-opus-5
    • In: 34
    • Out: 19.4k
    • Cache read: 1.3M
    • Cache create: 113.5k
    • Cost: ~$1.82 (est.)
    • Duration: 5m 13s
  • orchestrator-rollup — claude-sonnet-5
    • In: 6
    • Out: 873
    • Cache read: 106.5k
    • Cache create: 104.0k
    • Cost: ~$0.44 (est.)
    • Duration: 20s

Comment thread tools/cfl/internal/cmd/page/verify.go Outdated
Comment thread tools/cfl/internal/cmd/page/verify.go Outdated
Comment thread tools/cfl/internal/cmd/page/verify.go Outdated
Comment thread tools/cfl/internal/cmd/page/verify.go
Comment thread tools/cfl/internal/cmd/page/verify.go Outdated
Comment thread tools/cfl/internal/cmd/page/verify.go Outdated
Follow-up on the parameter-order fix, addressing review:

- A self-closing <ac:parameter/> no longer matches as if it opened a pair.
  It did, so the value capture ran on to the next closing tag and swallowed
  every element between them, page content included. That hid exactly the
  loss this guard exists to catch.
- Comment and CDATA spans are stripped before scanning, matching
  storageProfile. Markup quoted inside a code macro is content, not page
  configuration.
- Parameters are scoped to the macro holding them, so a value moving
  between two macros is a change rather than a reordering.
- Parameter values are whitespace-collapsed as they were when they
  travelled inside the compared text, so respacing alone is not an edit.
- Parameter drift is carried in its own field instead of forcing
  TextChanged, which made the report state the text differed while the page
  text was identical. It still fails the write, now with a message that says
  what actually happened, and OUTPUT_SPEC.md documents the shape.

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated PR Review

Reviewed commit: a3431094c623
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer Findings
go:implementation-tests 1
policies:conventions 0
architecture:solid-reviewer-agnostic 2
go:implementation-tests (1 finding)

Major - tools/cfl/internal/cmd/page/verify.go:547

The new len(drift.ParamChanges) > 0 fatal path (and its presenter branch in mutation.go's PresentWriteDrift) has no end-to-end test comparable to TestRunEditFailsWhenStoredContentDiffers. Every existing test that exercises ParamChanges (TestCompareStoredBodyXHTMLMacroParameterOrder, TestMacroParameterProfileIsScopedPerMacro) stops at compareStoredBody/diffMacroParameters and only asserts on the returned writeDrift value or Clean(); none drives runEdit/runCreate through driftServer with a parameter-only mismatch (TextChanged=false, ParamChanges non-empty) to confirm the command actually fails, nor does any test assert the presenter's "does not hold the macro parameters that were sent" wording. Since Clean() already reports false for this case via the existing tests, deleting this specific if len(drift.ParamChanges) > 0 { return fmt.Errorf(...) } block (or the matching presenter branch) would silently regress the PR's stated guarantee that "added, dropped or edited parameters still fail the write" without any test failing. Add a runEdit-level test mirroring TestRunEditFailsWhenStoredContentDiffers/TestRunEditToleratesNormalization for the parameter-only-mismatch case, and a driftReport-style assertion on the new presenter message.

architecture:solid-reviewer-agnostic (2 findings)

Major - tools/cfl/internal/cmd/page/verify.go:433

macroParameterProfile now strips inert spans first (verify.go:370) so markup quoted inside a code block is content, not configuration — but xhtmlText removes parameters from the raw string with no such guard. The two scanners therefore disagree about what a parameter is, and the span they disagree about is compared by neither: the profile skips it as CDATA, the text drops it as a parameter. Reproduced on this head against a code macro documenting storage markup (the sample contains an earlier >, so the tag-depth scanner is at depth 0 by the time it reaches the quoted markup):

sample := func(inner string) string {
  return `<ac:structured-macro ac:name="code"><ac:plain-text-body><![CDATA[if (a > b) {}` + "\n" +
    inner + "\n" + `tail]]></ac:plain-text-body></ac:structured-macro>`
}
sent   := sample(`<ac:parameter ac:name="x">DOCUMENTED SAMPLE</ac:parameter>`)
stored := sample(`<ac:parameter ac:name="x">SERVER MANGLED IT</ac:parameter>`)

// main (b46e5a4): "b) {} DOCUMENTED SAMPLE tail]]" vs "b) {} SERVER MANGLED IT tail]]" → TextChanged true
// this head:      "b) {} tail]]"                 vs "b) {} tail]]"                  → Clean() == true, ParamChanges []

So body text that main refuses this head accepts as a clean write. That is a silent false-clean on the one check whose purpose is to refuse a write that lost content (U-L1), and a narrowing the PR description says it did not make.

Suggested fix: remove parameters only from the segments outside storageInertRE matches — split on the inert spans and apply acParameterPairRE to the markup between them, or protect the inert spans and restore them — so xhtmlText and macroParameterProfile share one notion of what is markup, which is what the profile's doc comment already claims. Note that simply calling storageInertRE.ReplaceAllString inside xhtmlText would widen the blind spot instead, since it would drop all code-macro text from the comparison. Worth mirroring TestMacroParameterProfileIgnoresQuotedMarkup on the xhtmlText side.

Minor - tools/cfl/internal/cmd/page/verify.go:421

diffMacroParameters bakes the - / + markers into the values and then sorts the merged slice. + is 0x2B and - is 0x2D, so every added line sorts ahead of every dropped line. The emitted block is therefore:

Stored xhtml body does not hold the macro parameters that were sent. Page text is intact; these parameters differ:
  + macro 1 parameter breakoutWidth=500 (0→1)
  - macro 1 parameter breakoutWidth=760 (1→0)

while OUTPUT_SPEC.md:319-322 and :347-349 — described in that file as the authoritative output contract — both document - before +, matching every sibling block in the report (attributes dropped: precedes attributes added:). An edit also reads backwards: the new value is listed above the one it replaced.

The marker is also presentation the presenter no longer owns. present.WriteDrift is documented at mutation.go:127-128 as "Commands supply the finding; the wording is owned here", and DroppedAttrs/AddedAttrs honor that by staying two marker-free lists. ParamChanges collapses both directions into one pre-formatted list, which is what forces the sort that contradicts the spec (U-S1, U-G1).

Suggested fix: carry DroppedParams and AddedParams as separate marker-free lists, mirroring the attribute fields, and let the presenter emit - then + . Order then matches OUTPUT_SPEC.md without a doc change.

Reviewer Coverage

  • go:implementation-tests — complete (constrained); inspected 3 assigned files (4 inspected across reviewers): tools/cfl/internal/cmd/page/verify.go, tools/cfl/internal/cmd/page/verify_test.go, tools/cfl/internal/present/mutation.go; skipped: none; constraints: none
  • policies:conventions — complete (constrained); inspected 1 assigned file (4 inspected across reviewers): tools/cfl/internal/cmd/page/verify.go; skipped: none; constraints: none
  • architecture:solid-reviewer-agnostic — complete (constrained); inspected 2 assigned files (4 inspected across reviewers): tools/cfl/internal/cmd/OUTPUT_SPEC.md, tools/cfl/internal/cmd/page/verify.go; skipped: none; constraints: Findings were reproduced with temporary probe tests under tools/cfl/internal/cmd/page; the probe files were deleted afterwards and the tree is clean. Scope limited to the two assigned files. verify_test.go and internal/present/mutation.go were read as context only; mutation.go is not in the allowed file list, so nothing is reported against it. The base comparison in finding 1 used a local copy of main's xhtmlText (the depth scanner without this PR's parameter strip) rather than a full base checkout. The five earlier findings on this PR were re-checked against a343109 and all are fixed: self-closing swallow, field overloading, CDATA in the profile, whitespace collapse, per-macro scoping. go build ./..., go vet ./... and go test ./... pass in tools/cfl at this head; golangci-lint was not run.
Inspected files (4)
  • tools/cfl/internal/cmd/OUTPUT_SPEC.md
  • tools/cfl/internal/cmd/page/verify.go
  • tools/cfl/internal/cmd/page/verify_test.go
  • tools/cfl/internal/present/mutation.go

0 PR discussion threads considered. 0 summarized; 0 resolved.


Completed in 5m 44s | ~$4.60 (est.) | claude-sonnet-5, claude-opus-5 | cr 0.10.286
Field Value
Model claude-sonnet-5, claude-opus-5
Reviewers go:implementation-tests, policies:conventions, architecture:solid-reviewer-agnostic
Engine claude_cli · claude-sonnet-5, claude-opus-5
Reviewed by cr · monit-reviewer
Duration 5m 44s wall · 7m 36s compute
Cost ~$4.60 (est.)
Tokens 60 in / 25.4k out

Per-workstream usage

  • go:implementation-tests — claude-sonnet-5
    • In: 14
    • Out: 4.6k
    • Cache read: 662.8k
    • Cache create: 185.2k
    • Cost: ~$0.96 (est.)
    • Duration: 1m 19s
  • policies:conventions — claude-sonnet-5
    • In: 14
    • Out: 3.5k
    • Cache read: 597.8k
    • Cache create: 165.2k
    • Cost: ~$0.85 (est.)
    • Duration: 1m 01s
  • architecture:solid-reviewer-agnostic — claude-opus-5
    • In: 26
    • Out: 16.7k
    • Cache read: 1.4M
    • Cache create: 193.5k
    • Cost: ~$2.31 (est.)
    • Duration: 4m 58s
  • orchestrator-rollup — claude-sonnet-5
    • In: 6
    • Out: 619
    • Cache read: 112.5k
    • Cache create: 117.4k
    • Cost: ~$0.48 (est.)
    • Duration: 16s

Comment thread tools/cfl/internal/cmd/page/verify.go Outdated
Comment thread tools/cfl/internal/cmd/page/verify.go Outdated
Comment thread tools/cfl/internal/cmd/page/verify.go Outdated
… step

Second review pass:

- ParamChanges carried its own "- "/"+ " markers and sorted the merged
  list, so '+' (0x2B) sorted ahead of '-' (0x2D) and an edit read
  backwards, against the order OUTPUT_SPEC.md documents and every sibling
  block uses. It is now two marker-free lists, ParamsDropped and
  ParamsAdded, matching DroppedAttrs/AddedAttrs, and the presenter owns the
  markers and the wording as it does everywhere else.
- xhtmlText stripped parameters from the raw body while the profile skipped
  comment and CDATA spans, so the two disagreed about what a parameter is.
  A parameter quoted inside a code sample was dropped from the compared
  text and absent from the profile, leaving a server edit to it caught by
  neither, which main caught. Both scans now skip inert spans.
- The parameter-only fatal path had no end-to-end test, so deleting it
  would not have failed anything. A runEdit test now drives a
  parameter-only mismatch through the server and asserts the command fails
  and names the parameters.

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated PR Review

Reviewed commit: 00e7246bbc04
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer Findings
go:implementation-tests 1
policies:conventions 0
architecture:solid-reviewer-agnostic 1
go:implementation-tests (1 finding)

Minor - tools/cfl/internal/present/mutation.go:260

The two-header ParamsDropped/ParamsAdded rendering (and the dropped-before-added ordering this round's fix specifically called out: "the presenter owns the markers and emits dropped before added in both branches, so an edit reads old-then-new") has no direct presenter test. verify_test.go only asserts the runEdit error contains the substring "macro parameters" (TestRunEditFailsWhenStoredMacroParametersDiffer) and that compareStoredBody returns the right counts in ParamsDropped/ParamsAdded — neither exercises PresentWriteDrift. The file already has the right pattern to copy for exactly this: TestPresentedStorageLossExplainsTheCause and TestPresentedUncomparableStorageIsStated build a cflpresent.WriteDrift directly and assert on the rendered message text. Add an equivalent test that sets ParamsDropped/ParamsAdded and asserts both the "does not hold the macro parameters that were sent" wording and that a dropped entry's line precedes an added entry's line, so the ordering fix can't silently regress.

architecture:solid-reviewer-agnostic (1 finding)

Nits - tools/cfl/internal/cmd/page/verify.go:347

Optional, and fail-safe in the direction it errs — noting it because the invariant is now maintained by two independently written scans rather than one (U-S1). stripMacroParameters skips inert spans by segmenting around them, while macroParameterProfile skips them by deleting them and then scanning. The two agree for an inert span between parameters, but not for one inside a parameter's value: the segmenting cuts the element in half, so no pair match is found and the value stays in the compared text, while the profile (which deleted the comment first) sees the element whole and also counts the value.

a := `<ac:parameter ac:name="a"><!-- c -->AAA</ac:parameter>`
b := `<ac:parameter ac:name="b"><!-- c -->BBB</ac:parameter>`
sent   := `<ac:structured-macro ac:name="m">` + a + b + `</ac:structured-macro>`
stored := `<ac:structured-macro ac:name="m">` + b + a + `</ac:structured-macro>`

// SentText "AAABBB" vs StoredText "BBBAAA" → TextChanged true, write refused
// same shape without the comments → Clean() == true

So a server-side reordering is still refused when the parameter values carry an inert span, which is the case this PR exists to stop refusing. It fails closed rather than open, and a comment inside a macro parameter value is close to unheard of, so this is defensible to leave: I would not hold the PR for it.

If it is worth closing, one pass removes the divergence: mask inert spans with same-length placeholders so offsets survive, run acParameterPairRE once over the masked string, then use those spans both to build the profile keys (reading values from the original string) and to cut the text. Then a parameter is either fully excluded from the text and fully counted, or neither, by construction rather than by two scans agreeing. Evidence that would change the call: any real Confluence body with an inert span inside a parameter value.

Reviewer Coverage

  • go:implementation-tests — complete (constrained); inspected 3 assigned files (4 inspected across reviewers): tools/cfl/internal/cmd/page/verify.go, tools/cfl/internal/cmd/page/verify_test.go, tools/cfl/internal/present/mutation.go; skipped: none; constraints: none
  • policies:conventions — complete (constrained); inspected 1 assigned file (4 inspected across reviewers): tools/cfl/internal/cmd/page/verify.go; skipped: none; constraints: none
  • architecture:solid-reviewer-agnostic — complete (constrained); inspected 2 assigned files (4 inspected across reviewers): tools/cfl/internal/cmd/OUTPUT_SPEC.md, tools/cfl/internal/cmd/page/verify.go; skipped: none; constraints: A parameter whose value is CDATA (<ac:parameter><![CDATA[v]]></ac:parameter>) is compared by neither path, but main reports it clean too, so it is pre-existing and not reported. Both findings from the previous round were re-verified as fixed at 00e7246: dropped now precedes added under marker-free fields, and markup quoted inside a code block is compared again (the reproduction now fails the write). Reproductions were run with temporary probe tests under tools/cfl/internal/cmd/page; the probe files were deleted afterwards and the tree is clean. Scope limited to the two assigned files. verify_test.go and internal/present/mutation.go were read as context only; mutation.go is not in the allowed file list, so nothing is reported against it. go build ./..., go vet ./... and go test ./... pass in tools/cfl at this head; golangci-lint was not run.
Inspected files (4)
  • tools/cfl/internal/cmd/OUTPUT_SPEC.md
  • tools/cfl/internal/cmd/page/verify.go
  • tools/cfl/internal/cmd/page/verify_test.go
  • tools/cfl/internal/present/mutation.go

0 PR discussion threads considered. 0 summarized; 0 resolved.


Completed in 7m 09s | ~$5.78 (est.) | claude-sonnet-5, claude-opus-5 | cr 0.10.286
Field Value
Model claude-sonnet-5, claude-opus-5
Reviewers go:implementation-tests, policies:conventions, architecture:solid-reviewer-agnostic
Engine claude_cli · claude-sonnet-5, claude-opus-5
Reviewed by cr · monit-reviewer
Duration 7m 09s wall · 9m 45s compute
Cost ~$5.78 (est.)
Tokens 68 in / 23.2k out

Per-workstream usage

  • go:implementation-tests — claude-sonnet-5
    • In: 14
    • Out: 5.2k
    • Cache read: 817.7k
    • Cache create: 246.7k
    • Cost: ~$1.25 (est.)
    • Duration: 1m 39s
  • policies:conventions — claude-sonnet-5
    • In: 24
    • Out: 4.7k
    • Cache read: 1.4M
    • Cache create: 213.3k
    • Cost: ~$1.29 (est.)
    • Duration: 1m 48s
  • architecture:solid-reviewer-agnostic — claude-opus-5
    • In: 24
    • Out: 12.9k
    • Cache read: 1.6M
    • Cache create: 258.4k
    • Cost: ~$2.72 (est.)
    • Duration: 4m 29s
  • orchestrator-rollup — claude-sonnet-5
    • In: 6
    • Out: 504
    • Cache read: 117.3k
    • Cache create: 127.9k
    • Cost: ~$0.52 (est.)
    • Duration: 1m 48s

Comment thread tools/cfl/internal/present/mutation.go
Comment thread tools/cfl/internal/cmd/page/verify.go
Third review pass, both findings non-blocking:

- The invariant that the text scan and the parameter profile agree on what
  a parameter is was being held by two independently written scans, which
  disagreed for an inert span inside a parameter value: segmenting cut the
  element in half so the text kept the value, while the profile deleted the
  comment first, saw the element whole and counted it. Both now match
  against one blankInert view that masks comment and CDATA spans in place,
  preserving offsets, and index back into the original. One notion of what
  counts as markup now serves both.
- The dropped-before-added ordering was a claim about rendered output with
  no test on rendered output. A presenter test now asserts both branches
  read old value then new, and fails if the emission order is swapped.
@piekstra
piekstra marked this pull request as ready for review August 18, 2026 13:49
@piekstra
piekstra merged commit d97a005 into main Aug 18, 2026
16 checks passed
@piekstra
piekstra deleted the piekstra/cfl-macro-parameter-order branch August 18, 2026 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants