fix(cfl): stop macro parameter order failing a clean write - #480
Conversation
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
left a comment
There was a problem hiding this comment.
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 carryac:name="theme"set tononewould 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 editas<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'spage editcontract 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
acParameterReassumes 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,diffOffsetand 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
TextChangedis documented at verify.go:48-50 as "the document's text content differs", andpresent.PresentWriteDriftwords 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 (
diffOffsetreturns -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/AddedAttrsare 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 ofmacro 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
writeDriftits ownParametersChanged boolplusDroppedParams/AddedParams, haveClean()and the failure check inverifyStoredBody(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
storageProfilestrips comments and CDATA withstorageInertREbefore 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.TrimSpaceonly trims the ends, whereas the text this value used to travel in is whitespace-collapsed bystrings.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 yieldsClean() == falsehere (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: nonepolicies:conventions— complete (broad); inspected 1 assigned file (2 inspected across reviewers):tools/cfl/internal/cmd/page/verify.go; skipped: none; constraints: nonearchitecture: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.gotools/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
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
left a comment
There was a problem hiding this comment.
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) > 0fatal 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 specificif 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
macroParameterProfilenow strips inert spans first (verify.go:370) so markup quoted inside a code block is content, not configuration — butxhtmlTextremoves 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
storageInertREmatches — split on the inert spans and applyacParameterPairREto the markup between them, or protect the inert spans and restore them — soxhtmlTextandmacroParameterProfileshare one notion of what is markup, which is what the profile's doc comment already claims. Note that simply callingstorageInertRE.ReplaceAllStringinsidexhtmlTextwould widen the blind spot instead, since it would drop all code-macro text from the comparison. Worth mirroringTestMacroParameterProfileIgnoresQuotedMarkupon thexhtmlTextside.
Minor - tools/cfl/internal/cmd/page/verify.go:421
diffMacroParametersbakes 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:precedesattributes 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.WriteDriftis documented at mutation.go:127-128 as "Commands supply the finding; the wording is owned here", andDroppedAttrs/AddedAttrshonor that by staying two marker-free lists.ParamChangescollapses both directions into one pre-formatted list, which is what forces the sort that contradicts the spec (U-S1, U-G1).Suggested fix: carry
DroppedParamsandAddedParamsas 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: nonepolicies:conventions— complete (constrained); inspected 1 assigned file (4 inspected across reviewers):tools/cfl/internal/cmd/page/verify.go; skipped: none; constraints: nonearchitecture: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.mdtools/cfl/internal/cmd/page/verify.gotools/cfl/internal/cmd/page/verify_test.gotools/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
… 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
left a comment
There was a problem hiding this comment.
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).
stripMacroParametersskips inert spans by segmenting around them, whilemacroParameterProfileskips 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() == trueSo 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
acParameterPairREonce 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: nonepolicies:conventions— complete (constrained); inspected 1 assigned file (4 inspected across reviewers):tools/cfl/internal/cmd/page/verify.go; skipped: none; constraints: nonearchitecture: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.mdtools/cfl/internal/cmd/page/verify.gotools/cfl/internal/cmd/page/verify_test.gotools/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
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.
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:
The page was stored correctly. Nothing was lost.
Change
Macro parameters are removed from the compared text and diffed as a multiset instead:
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, fullgo test ./...andgolangci-lint runall pass. The new test fails without the change: