fix(cfl): INT-732 detect formatting loss a lossy read hides

[piekstra]

[INT-732]

The verification from #474 compares what the caller sent against what Confluence stored. If the caller got their content from cfl page view --body-format adf, both sides carry the same loss — so the check passes over a damaged page.

The loss is real and reproducible

Confluence's ADF representation doesn't always carry marks its storage representation does. em/strong wrapping a code span inside a table cell are dropped. So reading a page as ADF and writing the identical bytes back destroys them.

Reproduced on a scratch page — a no-op round trip, nothing changed:

	<em>	<strong>
before	3	3
after	1	1

page edit reported success with no warning.

This already happened to a real page: three table rows lost the bold-italic that the page's own legend describes as "Changed-from-Lambda rows are bold italic and marked — CHANGED". Every ADF-based check agreed the write was clean, because the baseline came through the same lossy read. It was only caught by comparing the storage body.

Fix

The storage body is what survives the round trip, so that's what gets watched: captured before the write, compared after, and element types whose counts fell are reported.

• A text edit moves no element counts → silent.
• Losing emphasis, a code span, or a link → reported. That's collateral a caller rarely intends.
• Additions are never reported; adding content is what an edit is for.
• A missing baseline reports nothing rather than "no loss", so a failed read can't manufacture a clean result.
• Markdown writes aren't verified, so they pay for no extra read (verificationApplies gates it, and a test pins the request count).

Dogfood — against real Confluence, with the built binary

I did not do this properly for #474: I shipped on unit tests plus one no-op live call, and the very first real use exposed this blind spot. So this time, on a scratch page created and deleted for the purpose:

Detects the real loss:

⚠ The stored page lost formatting that was present before this write:
  - em (2→0)
  - strong (2→0)
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.

No false positive — an ADF text edit on a page with no nested emphasis completed silently, and the intended edit landed in both cases.

make test (61 packages) and make lint (3 modules) clean.

One judgement call for you

This warns and exits 0 rather than failing. Rationale: the write succeeded and the content is intact, so failing would misrepresent what happened. The counter-argument is that a pipeline checking only the exit code would corrupt formatting across many pages silently — which is exactly how the original damage went unnoticed. Happy to flip it to non-zero if you'd rather; --no-verify already exists for callers who don't want the check.

[monit-reviewer]

Automated PR Review

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

Summary

Reviewer	Findings
go:implementation-tests	0
policies:conventions	1
architecture:solid-reviewer-agnostic	5
structure:harness-engineering	1

policies:conventions (1 finding)

Major - tools/cfl/internal/present/mutation.go:197

tools/cfl/internal/cmd/OUTPUT_SPEC.md calls itself "the authoritative declaration of the target cfl output contract" and its page edit/page create sections spell out every stderr block those commands emit (normalization, content-loss, offsets, exit codes). This PR adds a new stderr block — "The stored page lost formatting that was present before this write: ..." plus the follow-up explanation line — but OUTPUT_SPEC.md is untouched. A caller or agent relying on the spec to know what page edit/page create can print won't learn this warning exists, when it fires (storage-element-count regression, independent of the existing sent-vs-stored drift check), or that it is stderr-only and exits 0 (unlike the adjacent content-loss block, which exits non-zero). Add a short subsection to OUTPUT_SPEC.md's page edit/page create entries describing this warning shape and its exit-code behavior.

architecture:solid-reviewer-agnostic (5 findings)

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

U-L1 (contracts are behavioral): storageProfile is documented as "counts elements in a storage-format body", but <(\w[\w-]*)[\s/>] cannot match a namespaced element — after ac comes :, which is not in [\s/>], so the match fails entirely. I ran the regex against representative storage XHTML to confirm:

input:  <p>x</p><ac:structured-macro ac:name="code">…<ac:plain-text-body><![CDATA[<div><strong>hi</strong></div>]]></ac:plain-text-body></ac:structured-macro><ri:page ri:content-title="T"/><!-- <em>c</em> -->
result: map[div:1 em:1 p:1 strong:1]

Two consequences, in opposite directions:

• Silent miss. ac:structured-macro, ac:parameter, ri:page, ri:attachment — i.e. most Confluence-specific storage markup — are invisible to the profile. Dropping an entire macro or attachment reference across a write reports nothing, while the presenter's wording ("lost formatting that was present before this write") reads as a general verdict on the write.
• False positive. Element names inside CDATA and HTML comments are counted, so editing prose inside a code block that quotes <div>/<strong> reports invented "formatting loss".

Suggested fix: widen to something like <([\w-]+(?::[\w-]+)?)[\s/>], and — following the precedent adfContent sets at verify.go:142-145 — state the heuristic's limits in the comment (comments/CDATA are counted; this is a signal, not a parse). If quoted markup false positives matter, strip <!-- --> and <![CDATA[…]]> spans before profiling.

Major - tools/cfl/internal/cmd/page/edit.go:222

U-L2 (errors are contracts): the baseline read's error is discarded (storageBefore, _ = ...), and a failed read then makes verifyStorageLoss return nil, so the operator sees exactly the same output as a clean write. The check silently stops existing.

That is the failure mode this PR was written to fix: the original page damage went unnoticed precisely because every signal said "clean". readStorageBody's doc comment says callers treat "" as "cannot compare" — but nothing downstream ever tells the caller a comparison could not be made, so "cannot compare" is indistinguishable from "nothing lost" at the only place it matters, the terminal.

Suggested fix: keep the write path non-fatal, but emit a warning when the baseline (or the post-write) storage read fails — e.g. carry a baselineUnavailable bool (or an error) on verifyRequest and add a line to PresentWriteDrift such as "formatting-loss check skipped: could not read the page's storage body before the write". The same applies to the swallowed error at verify.go:528.

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

U-S1 (responsibility): the presenter asserts a cause it has not established. verificationApplies enables the storage-loss check for XHTML writes too, so an operator piping storage markup through sed (the documented workflow in edit.go's own --body-format xhtml example) who loses an element is told "Reading a page as ADF and writing it back does not always preserve marks its storage form carries" — an explanation of a round trip they did not perform. The model already carries d.BodyFormat, and this branch ignores it.

Diagnosing why counts fell is core knowledge (it is stated once already in verify.go:484-494); the presenter's job is wording the finding. Suggested fix: word the shared part on the observation ("these element types are less frequent in the stored page than before the write"), and gate the ADF explanation on d.BodyFormat == "adf" — or have the command supply the cause hint as a model field rather than the presenter inferring it.

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

U-S1 (one reason to change): verificationApplies was introduced to own the "will this write be verified" decision, but it does not — the same policy is still encoded twice more. verifyStoredBody re-derives it from req.enabled plus its own format check (verify.go:383-388), and each call site computes enabled by hand (hasNewContent && !opts.noVerify in edit.go:247, !opts.noVerify in create.go:192). The new helper has exactly one production caller.

The drift this invites is asymmetric and silent: add a body format to verificationApplies and the baseline read starts happening while verifyStoredBody still returns early — a paid-for read with no comparison. Reverse the omission and the loss check goes quiet with no signal (see the edit.go:222 finding).

Suggested fix: let verifyStoredBody gate on verificationApplies(req.bodyFormat, req.hasNewContent, req.noVerify) and have verifyRequest carry those inputs instead of a pre-computed enabled, so the predicate has one home. create passes hasNewContent: true, which reads as the fact it is.

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

U-L2 (errors are contracts) — answering the judgement call in the PR description. I would keep exit 0: the write did land, TextChanged already owns "the content is wrong", and the existing normalization branch sets exactly this precedent (report, tolerate, exit 0). Flipping this one to non-zero would make the exit code mean two different things depending on which check fired.

The gap worth closing is not the exit code, it is that loss has no machine-readable channel: PresentWriteDrift emits prose to stderr, so a pipeline can only branch by string-matching the warning text, which U-L2 rules out. Suggested fix: surface LostElements as data in the structured output alongside the warning, or add an opt-in --fail-on-loss so callers who want a hard stop can ask for one — rather than making every caller's exit code carry the ambiguity.

Defensible either way; what would change my view is evidence that the primary consumers are non-interactive scripts that never read stderr, in which case warn-only is the same silence this PR is fixing.

structure:harness-engineering (1 finding)

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

verifyStorageLoss fetches the storage (XHTML) body unconditionally via readStorageBody, but for a --body-format xhtml edit that body was already fetched moments earlier: verifyStoredBody's own readback (getPageWithBodyFormat at line 390, called with req.bodyFormat==bodyFormatXHTML) already returns bodyValue(...,bodyFormatXHTML), i.e. Storage.Value — the exact same representation verifyStorageLoss re-fetches at line 527. So every non-no-verify edit with --body-format xhtml issues two GETs for the identical body instead of reusing storedContent computed in verifyStoredBody. (For --body-format adf the two fetches are legitimately different representations, so this only applies to the xhtml path.) This directly undercuts the PR's own framing that reads are only paid for when needed (markdown skips the read entirely), and it's invisible to the added tests: verify_test.go only pins the GET count for the --no-verify and markdown-skip paths (TestRunEditNoVerifySkipsReadback, TestRunEditSkipsVerificationForMarkdown), never for the normal xhtml verify path where the duplicate actually fires. Fix: thread the already-fetched storedContent into verifyStorageLoss (or have verifyStoredBody pass it in as the 'after' body) when req.bodyFormat == bodyFormatXHTML, and add a GET-count assertion for the xhtml verify path so a future change can't silently reintroduce the duplicate.

Reviewer Coverage

• go:implementation-tests — complete (broad); skipped: none; constraints: none
• policies:conventions — complete (broad); inspected 4 assigned files (5 inspected across reviewers): tools/cfl/internal/cmd/page/create.go, tools/cfl/internal/cmd/page/edit.go, tools/cfl/internal/cmd/page/verify.go, tools/cfl/internal/present/mutation.go; skipped: none; constraints: none
• architecture:solid-reviewer-agnostic — complete (broad); inspected 4 assigned files (5 inspected across reviewers): tools/cfl/internal/cmd/page/create.go, tools/cfl/internal/cmd/page/edit.go, tools/cfl/internal/cmd/page/verify.go, tools/cfl/internal/present/mutation.go; skipped: none; constraints: Confluence's real storage output was not available, so claims about namespaced macro markup were verified by running the new regex against representative storage XHTML locally, not against a live page. Ran go test ./internal/cmd/page/... ./internal/present/... in tools/cfl with CGO_ENABLED=0 (the sandbox's inherited cgo cache path is stale); both packages pass. Reviewed the four assigned files; verify_test.go and fetch.go were read as context only and are not reported on.
• structure:harness-engineering — complete (broad); inspected 2 assigned files (5 inspected across reviewers): tools/cfl/internal/cmd/page/verify.go, tools/cfl/internal/present/mutation.go; skipped: none; constraints: none

Inspected files (5)

• tools/cfl/internal/cmd/page/create.go
• tools/cfl/internal/cmd/page/edit.go
• 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 04s | ~$5.56 (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, structure:harness-engineering
Engine	claude_cli · claude-sonnet-5, claude-opus-5
Reviewed by	cr · monit-reviewer
Duration	7m 04s wall · 12m 13s compute
Cost	~$5.56 (est.)
Tokens	136 in / 49.0k out

Per-workstream usage

• orchestrator-selection — claude-sonnet-5
  • In: 6
  • Out: 3.7k
  • Cache read: 101.8k
  • Cache create: 81.8k
  • Cost: ~$0.39 (est.)
  • Duration: 45s
• go:implementation-tests — claude-sonnet-5
  • In: 42
  • Out: 12.2k
  • Cache read: 1.7M
  • Cache create: 105.0k
  • Cost: ~$1.08 (est.)
  • Duration: 2m 43s
• policies:conventions — claude-sonnet-5
  • In: 16
  • Out: 4.2k
  • Cache read: 514.8k
  • Cache create: 93.1k
  • Cost: ~$0.57 (est.)
  • Duration: 1m 11s
• architecture:solid-reviewer-agnostic — claude-opus-5
  • In: 50
  • Out: 17.9k
  • Cache read: 2.2M
  • Cache create: 122.4k
  • Cost: ~$2.29 (est.)
  • Duration: 5m 08s
• structure:harness-engineering — claude-sonnet-5
  • In: 16
  • Out: 6.0k
  • Cache read: 583.8k
  • Cache create: 115.9k
  • Cost: ~$0.70 (est.)
  • Duration: 1m 25s
• orchestrator-rollup — claude-sonnet-5
  • In: 6
  • Out: 5.0k
  • Cache read: 114.4k
  • Cache create: 110.3k
  • Cost: ~$0.52 (est.)
  • Duration: 58s

[piekstra]

Six taken, one agreed-with-no-change, in c1da2f0.

The discarded baseline error was the sharpest, and you're right that it was this PR's own failure mode. storageBefore, _ = ... meant a failed read produced exactly the output of a clean write — the check silently ceased to exist, which is worse than not having it, because now you'd trust it. The comparison reports whether it ran and why it couldn't:

Could not compare the stored page against its state before the write, so
formatting loss would not have been noticed. (resource not found)

CDATA/comments — correct, and it would have invented losses: storage carries macro bodies verbatim inside CDATA where angle brackets are content, not markup. Both stripped before profiling.

Cause asserted without establishing it — also correct, and it's the same class as the counts finding on #474: blaming an ADF round trip for an XHTML write tells an operator who piped storage through sed that Confluence did something it didn't. The cause is now chosen by the format actually used.

Redundant fetch — an XHTML write already read the stored body back; it's reused rather than fetched twice.

verificationApplies didn't own the decision — fair, it was one of three places encoding it. Both call sites now gate on it, and the duplicate format check inside verifyStoredBody is gone.

OUTPUT_SPEC — added, both new blocks.

Exit code — agreed, keeping exit 0, and your reasoning is better than mine: TextChanged already owns "the content is wrong", and the normalization branch sets the precedent. Left as-is.

Re-dogfooded after the changes

Not repeating #474's mistake of shipping review edits on unit tests alone. On fresh scratch pages, created and deleted:

• ADF round trip → em (2→0), strong (2→0), cause names the ADF round trip.
• XHTML write with <strong> stripped → strong (2→0), cause reads "The submitted storage body did not carry these elements" — correctly not blaming ADF. em survived, confirming the XHTML path is lossless.

make test (61 packages) and make lint (3 modules) clean.

[monit-reviewer]

Automated PR Review

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

Summary

Reviewer	Findings
go:implementation-tests	0
policies:conventions	0
architecture:solid-reviewer-agnostic	3
structure:harness-engineering	0

architecture:solid-reviewer-agnostic (3 findings)

Blocking - tools/cfl/internal/cmd/page/create.go:192

U-L1 (contracts are behavioral): the new "could not compare" path fires on every page create with --body-format adf|xhtml, including a perfectly clean one.

create passes storageBefore: "" (line 191) with no storageBeforeErr, and compareStorage treats an empty baseline as a failed comparison (verify.go:556-557) rather than as "a create has no before-state". verifyStoredBody's early return is now gated on !storage.Unavailable (verify.go:410), so the warning block always renders. Confirmed by running runCreate against the existing createDriftServer with the stored body identical to what was posted:

STDERR>>>⚠ Could not compare the stored page against its state before the write, so formatting loss would not have been noticed.
<<<

The claim is false — nothing could have been lost, because there was nothing there before — and the reason parenthetical is empty, so the operator gets an unexplained warning on the happy path of a routine command. The cost compounds: a warning that appears on every create is one operators learn to ignore, which is precisely the desensitization this check exists to prevent. OUTPUT_SPEC.md documents this block as meaning the comparison could not be made, and says nothing about create emitting it unconditionally.

Suggested fix: make "no baseline was expected" a distinct state from "the baseline read failed" — e.g. a storageBaselineApplies bool on verifyRequest, set by edit and left false by create, with compareStorage returning a zero storageComparison when it is false. Cover it where it broke: assert on opts.Stderr in a clean-create test (the existing TestRunCreateToleratesNormalization only asserts no error, which is why this shipped), and mirror it with a clean-edit assertion.

(Not a regression in the fix for the settled baseline-error thread — that part is right for edit, where storageBeforeErr is now captured and surfaced. Only the create path needs distinguishing.)

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

U-L1 (contracts are behavioral): the CDATA/comment half of the earlier regex finding is fixed (storageInertRE, verify.go:507), but the namespaced-element half is unchanged and now undocumented — the new doc comment explains the CDATA handling and is silent about what the profile cannot see at all.

<(\w[\w-]*)[\s/>] still cannot match <ac:…> or <ri:…>: after ac comes :, which is not in [\s/>], so the match fails outright. Run against a page that loses an info macro and an attachment reference:

before: <p>x</p><ac:structured-macro ac:name="info"><ac:rich-text-body><p>hi</p></ac:rich-text-body></ac:structured-macro><ri:attachment ri:filename="a.png"/>
after:  <p>x</p>
profile(before) = map[p:2]
lost            = [p (2→1)]

The macro and the attachment reference vanish and the report names neither; it names p, a paragraph the caller lost inside the macro. So the output is not merely incomplete — for Confluence-specific markup, which is most of what distinguishes storage format from plain XHTML, it points at the wrong thing while the presenter's wording reads as a general verdict on the write.

Suggested fix: widen to <([\w-]+(?::[\w-]+)?)[\s/>] so ac:structured-macro, ac:parameter, ri:page and ri:attachment are counted. If they are deliberately out of scope, say so in the comment the way adfContent states its own limits (verify.go:142-145) — this heuristic is currently the only place the boundary could be recorded, and it isn't.

Minor - tools/cfl/internal/cmd/page/edit.go:222

U-S1 (one reason to change): the post-write duplicate read is gone (the reuse thread at verify.go:404-408 is correctly settled), but the pre-write baseline still re-fetches a body the command already has.

getPageWithBodyFallback (edit.go:174) fetches with BodyFormat: "storage", so existingPage.Body.Storage.Value is already the baseline for both the adf and xhtml non-editor paths; readStorageBody here asks the server for the same representation of the same version again. Measured on a clean xhtml edit through the existing driftServer: GETS=3 (pre-write fallback, baseline, post-write readback) where 2 carry all the information.

Suggested fix: use bodyValue(existingPage, bodyFormatXHTML) when it is non-empty and fall back to readStorageBody only when it is not (the --editor + adf path, where the pre-write fetch asked for atlas_doc_format). Keeping the storageBeforeErr states as they are, since the fallback can still fail.

While you are in the tests: the reuse wiring itself is still unpinned end to end. TestStorageComparisonReusesSuppliedBody exercises compareStorage directly, so a change to the if req.bodyFormat == bodyFormatXHTML line at verify.go:406 would reintroduce the duplicate GET with every test still green. A GET-count assertion on the normal xhtml verify path (the driftServer already returns the counter) closes that and would also pin whatever this fix lands at.

Reviewer Coverage

• go:implementation-tests — complete (constrained); inspected 5 assigned files (6 inspected across reviewers): tools/cfl/internal/cmd/page/create.go, tools/cfl/internal/cmd/page/edit.go, 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 4 assigned files (6 inspected across reviewers): tools/cfl/internal/cmd/page/create.go, tools/cfl/internal/cmd/page/edit.go, tools/cfl/internal/cmd/page/verify.go, tools/cfl/internal/present/mutation.go; skipped: none; constraints: none
• architecture:solid-reviewer-agnostic — complete (constrained); inspected 5 assigned files (6 inspected across reviewers): tools/cfl/internal/cmd/OUTPUT_SPEC.md, tools/cfl/internal/cmd/page/create.go, tools/cfl/internal/cmd/page/edit.go, tools/cfl/internal/cmd/page/verify.go, tools/cfl/internal/present/mutation.go; skipped: none; constraints: Findings below were confirmed by running the package under test in a scratch copy of the checkout (the review checkout was left untouched); go test ./internal/cmd/page/... ./internal/present/... passes on head with CGO_ENABLED=0. No live Confluence instance was available; storage-markup claims were verified by running the shipped regex against representative storage XHTML. Re-review of c1da2f0 against base 51de27e; the earlier round's threads are marked settled, so I verified each fix in the current code rather than restating settled points. verify_test.go was read as context but is not an assigned file, so test-only observations are attached to the production code they cover.
• structure:harness-engineering — complete (constrained); inspected 2 assigned files (6 inspected across reviewers): tools/cfl/internal/cmd/page/verify.go, tools/cfl/internal/present/mutation.go; skipped: none; constraints: none

Inspected files (6)

• tools/cfl/internal/cmd/OUTPUT_SPEC.md
• tools/cfl/internal/cmd/page/create.go
• tools/cfl/internal/cmd/page/edit.go
• 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 14s | ~$6.48 (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, structure:harness-engineering
Engine	claude_cli · claude-sonnet-5, claude-opus-5
Reviewed by	cr · monit-reviewer
Duration	5m 14s wall · 9m 59s compute
Cost	~$6.48 (est.)
Tokens	106 in / 36.3k out

Per-workstream usage

• go:implementation-tests — claude-sonnet-5
  • In: 26
  • Out: 6.7k
  • Cache read: 1.3M
  • Cache create: 168.9k
  • Cost: ~$1.14 (est.)
  • Duration: 1m 52s
• policies:conventions — claude-sonnet-5
  • In: 18
  • Out: 4.4k
  • Cache read: 754.0k
  • Cache create: 137.3k
  • Cost: ~$0.81 (est.)
  • Duration: 1m 11s
• architecture:solid-reviewer-agnostic — claude-opus-5
  • In: 28
  • Out: 16.2k
  • Cache read: 1.7M
  • Cache create: 217.7k
  • Cost: ~$2.62 (est.)
  • Duration: 4m 22s
• structure:harness-engineering — claude-sonnet-5
  • In: 30
  • Out: 7.7k
  • Cache read: 1.8M
  • Cache create: 197.3k
  • Cost: ~$1.40 (est.)
  • Duration: 2m 07s
• orchestrator-rollup — claude-sonnet-5
  • In: 4
  • Out: 1.2k
  • Cache read: 31.8k
  • Cache create: 131.8k
  • Cost: ~$0.52 (est.)
  • Duration: 25s

[piekstra]

All three taken, in 4c9e1a8.

The create false-positive was the worst of the three — every clean page create with an exact body format announced that formatting loss would not have been noticed. A warning that fires on the happy path is worse than no warning, because it trains the operator to skip past the one that matters. A create legitimately has no prior state, which is now distinguished from a baseline that failed to read.

Namespaced elements weren't miscounted — they were invisible. <(\w[\w-]*)[\s/>] can't match <ac:structured-macro> because : isn't in the trailing class, so removing a macro moved nothing the profile could see. That's a hole in the check's whole purpose, not a rough edge. Fixed, and the comment now says what the profile does and doesn't cover instead of describing only the CDATA handling.

Pre-write re-fetch — correct, getPageWithBodyFallback already fetches with the storage representation, so the baseline was in hand. It's used directly; the extra read only happens when it's genuinely absent.

Re-dogfooded live again

• Clean page create --body-format adf → silent. (Previously warned every time.)
• Edit removing an ac:structured-macro → caught: ac:rich-text-body (1→0), ac:structured-macro (1→0). The old pattern could not have seen this at all.

make test (61 packages) and make lint (3 modules) clean.

[monit-reviewer]

Automated PR Review

Reviewed commit: 44bc3fb34fb2
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	0
policies:conventions	0
architecture:solid-reviewer-agnostic	1
structure:harness-engineering	1

architecture:solid-reviewer-agnostic (1 finding)

Nits - tools/cfl/internal/cmd/OUTPUT_SPEC.md:270

U-G1 (documented contracts): the page create section forwards the reader to page edit — "exactly as page edit does" — and that section now describes two blocks a create can never emit. With comparePriorState false for creates (verify.go:564), neither "The stored page lost formatting…" nor "Could not compare the stored page…" is reachable from page create; I confirmed a clean create prints nothing on stderr.

The forwarding sentence does scope itself ("the stderr blocks emitted on normalization and on content loss"), so this is not wrong so much as easy to over-read — and the distinction it elides is exactly the one that took a round to get right in the code.

Suggested fix: one clause in the create section, e.g. "a create has no prior state, so the prior-state comparison blocks in that section do not apply."

structure:harness-engineering (1 finding)

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

The two request-dedup fixes from prior review rounds are both correctly implemented now: edit.go:224 reuses existingPage's storage body for the pre-write baseline instead of re-fetching, and verify.go:409-413 reuses the post-write storedContent for the xhtml path instead of a second readStorageBody call. Both are real, verified fixes. But neither is protected by an end-to-end regression test through the command path: TestStorageComparisonReusesSuppliedBody only calls compareStorage directly (a unit test of the dedup logic), and the runEdit-level tests that exercise the normal (verified, non-no-verify) xhtml path — TestRunEditFailsWhenStoredContentDiffers and TestRunEditToleratesNormalization — discard driftServer's *int GET counter entirely (srv, _ := driftServer(...)). Only the no-verify and markdown-skip paths pin a GET count (verify_test.go:303, :336). So a future change to edit.go's bodyValue(existingPage, bodyFormatXHTML) reuse or verify.go's req.bodyFormat == bodyFormatXHTML reuse condition can silently reintroduce either duplicate GET (this happened once already: the post-write duplicate was the reviewed-and-fixed finding two rounds ago) with the full test suite staying green. Fix: add a GET-count assertion on TestRunEditToleratesNormalization (or a new test) asserting the normal xhtml verify path issues exactly 2 GETs (the pre-write fetch and the post-write readback, no baseline duplicate) — mirroring the pattern already used for the no-verify and markdown cases.

Reviewer Coverage

• go:implementation-tests — complete (constrained); inspected 5 assigned files (6 inspected across reviewers): tools/cfl/internal/cmd/page/create.go, tools/cfl/internal/cmd/page/edit.go, 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 4 assigned files (6 inspected across reviewers): tools/cfl/internal/cmd/page/create.go, tools/cfl/internal/cmd/page/edit.go, tools/cfl/internal/cmd/page/verify.go, tools/cfl/internal/present/mutation.go; skipped: none; constraints: none
• architecture:solid-reviewer-agnostic — complete (constrained); inspected 5 assigned files (6 inspected across reviewers): tools/cfl/internal/cmd/OUTPUT_SPEC.md, tools/cfl/internal/cmd/page/create.go, tools/cfl/internal/cmd/page/edit.go, tools/cfl/internal/cmd/page/verify.go, tools/cfl/internal/present/mutation.go; skipped: none; constraints: Re-review of 44bc3fb against base 51de27e, focused on the c1da2f0→44bc3fb changes and on verifying the settled threads in the current code rather than restating them. Verified by running the package in a scratch copy of the checkout (review checkout untouched): clean create is now silent at 1 GET, clean xhtml edit is silent at 2 GETs, and the profile counts ac:/ri: elements. go test ./internal/cmd/page/... ./internal/present/... passes on head with CGO_ENABLED=0 (the sandbox's inherited cgo cache path is stale). verify_test.go was read as context but is not an assigned file, so test observations are attached to the production code whose behavior they leave unpinned.
• structure:harness-engineering — complete (constrained); inspected 2 assigned files (6 inspected across reviewers): tools/cfl/internal/cmd/page/verify.go, tools/cfl/internal/present/mutation.go; skipped: none; constraints: none

Inspected files (6)

• tools/cfl/internal/cmd/OUTPUT_SPEC.md
• tools/cfl/internal/cmd/page/create.go
• tools/cfl/internal/cmd/page/edit.go
• 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 3m 38s | ~$7.56 (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, structure:harness-engineering
Engine	claude_cli · claude-sonnet-5, claude-opus-5
Reviewed by	cr · monit-reviewer
Duration	3m 38s wall · 8m 27s compute
Cost	~$7.56 (est.)
Tokens	94 in / 31.4k out

Per-workstream usage

• go:implementation-tests — claude-sonnet-5
  • In: 28
  • Out: 8.6k
  • Cache read: 1.8M
  • Cache create: 225.3k
  • Cost: ~$1.52 (est.)
  • Duration: 2m 01s
• policies:conventions — claude-sonnet-5
  • In: 14
  • Out: 4.6k
  • Cache read: 654.4k
  • Cache create: 178.4k
  • Cost: ~$0.93 (est.)
  • Duration: 1m 13s
• architecture:solid-reviewer-agnostic — claude-opus-5
  • In: 20
  • Out: 9.1k
  • Cache read: 1.4M
  • Cache create: 291.2k
  • Cost: ~$2.76 (est.)
  • Duration: 2m 34s
• structure:harness-engineering — claude-sonnet-5
  • In: 26
  • Out: 7.7k
  • Cache read: 1.9M
  • Cache create: 273.5k
  • Cost: ~$1.72 (est.)
  • Duration: 2m 12s
• orchestrator-rollup — claude-sonnet-5
  • In: 6
  • Out: 1.4k
  • Cache read: 134.0k
  • Cache create: 149.1k
  • Cost: ~$0.62 (est.)
  • Duration: 24s

[piekstra]

Both taken, in 5e32dda.

The dedup fixes were unprotected — fair, and the gap is exactly where a regression would hide: only the skip paths pinned a request count, so the verified path could have quietly regained either extra read. TestRunEditVerifiedPathMakesNoRedundantReads now asserts 2 GETs on that path — one before the write, one to read the result back. A third means the baseline or the storage comparison re-fetched something the command already had.

OUTPUT_SPEC's create section forwarded to page edit for blocks a create can no longer emit, now that a create isn't treated as a failed comparison. Corrected to say so explicitly rather than pointing at behaviour that doesn't apply.

make test (61 packages) and make lint (3 modules) clean.

[monit-reviewer]

Automated PR Review

Reviewed commit: 5e32dda77dc8
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	0
policies:conventions	0
architecture:solid-reviewer-agnostic	0
structure:harness-engineering	0

Reviewer Coverage

• go:implementation-tests — complete (constrained); inspected 5 assigned files (6 inspected across reviewers): tools/cfl/internal/cmd/page/create.go, tools/cfl/internal/cmd/page/edit.go, tools/cfl/internal/cmd/page/verify.go, tools/cfl/internal/cmd/page/verify_test.go, tools/cfl/internal/present/mutation.go; skipped: none; constraints: Only tools/cfl/internal/cmd/page/verify_test.go changed in this commit among the assigned files (create.go, edit.go, verify.go, and mutation.go are unchanged since the prior review round and were re-inspected for regressions, not re-reviewed from scratch).
• policies:conventions — complete (constrained); inspected 4 assigned files (6 inspected across reviewers): tools/cfl/internal/cmd/page/create.go, tools/cfl/internal/cmd/page/edit.go, tools/cfl/internal/cmd/page/verify.go, tools/cfl/internal/present/mutation.go; skipped: none; constraints: This round's diff only touches tools/cfl/internal/cmd/OUTPUT_SPEC.md and verify_test.go, neither of which is in this reviewer's assigned file set; the assigned Go files are unchanged since the prior round.
• architecture:solid-reviewer-agnostic — complete (constrained); inspected 5 assigned files (6 inspected across reviewers): tools/cfl/internal/cmd/OUTPUT_SPEC.md, tools/cfl/internal/cmd/page/create.go, tools/cfl/internal/cmd/page/edit.go, tools/cfl/internal/cmd/page/verify.go, tools/cfl/internal/present/mutation.go; skipped: none; constraints: Confirmed the new read-count test is load-bearing by reverting each dedup in a scratch copy: removing the edit.go baseline reuse and disabling the verify.go xhtml reuse each fail it at 3 GETs. Review checkout untouched. No production code changed this round — only verify_test.go and the OUTPUT_SPEC create section — so the assigned .go files were re-read for regressions rather than re-reviewed from scratch. Re-review of 5e32dda against base 51de27e, concentrated on the 44bc3fb→5e32dda changes; earlier rounds' threads were re-checked in the current code rather than restated. The read-count test pins the xhtml verified path; the adf path legitimately makes a third read for the storage body and is intentionally not covered by that assertion. go test ./internal/cmd/page/... ./internal/present/... passes on head with CGO_ENABLED=0 (the sandbox's inherited cgo cache path is stale).
• structure:harness-engineering — complete (constrained); inspected 2 assigned files (6 inspected across reviewers): tools/cfl/internal/cmd/page/verify.go, tools/cfl/internal/present/mutation.go; skipped: none; constraints: none

Inspected files (6)

• tools/cfl/internal/cmd/OUTPUT_SPEC.md
• tools/cfl/internal/cmd/page/create.go
• tools/cfl/internal/cmd/page/edit.go
• 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 2m 01s | ~$7.36 (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, structure:harness-engineering
Engine	claude_cli · claude-sonnet-5, claude-opus-5
Reviewed by	cr · monit-reviewer
Duration	2m 01s wall · 4m 09s compute
Cost	~$7.36 (est.)
Tokens	58 in / 12.8k out

Per-workstream usage

• go:implementation-tests — claude-sonnet-5
  • In: 16
  • Out: 4.8k
  • Cache read: 1.1M
  • Cache create: 276.8k
  • Cost: ~$1.43 (est.)
  • Duration: 1m 13s
• policies:conventions — claude-sonnet-5
  • In: 8
  • Out: 2.2k
  • Cache read: 325.4k
  • Cache create: 215.3k
  • Cost: ~$0.94 (est.)
  • Duration: 37s
• architecture:solid-reviewer-agnostic — claude-opus-5
  • In: 14
  • Out: 3.2k
  • Cache read: 1.0M
  • Cache create: 341.3k
  • Cost: ~$2.73 (est.)
  • Duration: 1m 15s
• structure:harness-engineering — claude-sonnet-5
  • In: 14
  • Out: 2.0k
  • Cache read: 1.0M
  • Cache create: 337.5k
  • Cost: ~$1.61 (est.)
  • Duration: 46s
• orchestrator-rollup — claude-sonnet-5
  • In: 6
  • Out: 478
  • Cache read: 138.8k
  • Cache create: 160.2k
  • Cost: ~$0.65 (est.)
  • Duration: 16s