Repository navigation
fix(adf): decode markdown escapes and entities in text - #511
Conversation
A page read as markdown escapes punctuation and encodes entities, and the converter stored those as literal text, so each read and write back added visible backslashes and entity names. Decode them for text and image alt text, as CommonMark specifies. Code spans and blocks keep their source.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 25921bf4e0c9
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| harness-engineering:repo-health | 1 |
go:implementation-tests (1 finding)
Major - shared/adf/convert.go:385
decodeInlineTexttreats everything from a&to the next;as one entity candidate, however far away that;is. A bare&followed later by a real entity therefore gets a multi-tokenref.ResolveEntityNamesdecodes the entity inside it,bytes.Equalfails, and the whole span is written and skipped, so backslash escapes in that span are never decoded.This is the round trip the PR is fixing, because the storage read leaves
&bare. Probe results at the PR head:
- Storage
<p>R&D a_b 1 > 2</p>reads as markdownR&D a\_b 1 > 2, andToADFproduces the textR&D a\_b 1 > 2, so the visible backslash remains.a & b_c d <e>also keepsb\_c.- Escaped entities break too:
a & \>producesa & \>instead ofa & >, which violates the PR's rule that an escaped entity stays literal.Any text with a bare
&(R&D,AT&T) before an escape and an entity keeps the corruption, and it can grow on repeated round trips.The new tests miss this because of ordering.
TestRoundtrip_PlainTextThroughStorageReadputs its escaped text before the&, and theunknown entity stayscase has no escape or entity after its bare&.Fix: only accept a syntactically valid reference by scanning
&, an optional#, then alphanumerics, and requiring;immediately after. Resolve only that slice, and otherwise emit&and continue at the next byte. Add table cases such asR&D a\_b > c->R&D a_b > canda & \>->a & >, plus a storage round trip with&before the escaped text.
harness-engineering:repo-health (1 finding)
Major - tools/cfl/pkg/md/roundtrip_test.go:248
The invariant this test documents, that a page read as markdown and written back keeps its text, does not hold for literal entity text, and this diff introduces that regression. The storage read does not escape
&in text that looks like an entity. Probing head:<p>&gt;</p>(a page showing the literal text>) reads as markdown>, andToADFnow stores>.&amp;reads as&and is stored as&, so each round trip strips one level. Before this diff the raw segment kept these characters, so this case survived. Impact:cfl page editsilently rewrites pages that document HTML or entities, and neither the test nor the PR's "Not changed" section reveals the gap, so future work will assume the round trip is lossless on the storage path. Fix: make the storage read escape&when the following text would parse as an entity (emit\>, which the new decoder already keeps literal), and add&gt;and&amp;to this test's storage fixture.
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (constrained); scope: 5 files; examples:shared/adf/convert.go,shared/adf/convert_test.go,tools/cfl/CHANGELOG.md,tools/cfl/pkg/md/roundtrip_test.go,tools/jtk/CHANGELOG.md; body-inspected: 5 files; examples:shared/adf/convert.go,shared/adf/convert_test.go,tools/cfl/CHANGELOG.md,tools/cfl/pkg/md/roundtrip_test.go,tools/jtk/CHANGELOG.md; skipped: none; constraints: Probes ran as temporary tests in a scratch copy of the PR head; the review checkout was not modified. The full test suite and linters were not re-run.harness-engineering:repo-health— complete (constrained); scope: 3 files; examples:shared/adf/convert.go,shared/adf/convert_test.go,tools/cfl/pkg/md/roundtrip_test.go; body-inspected: 3 files; examples:shared/adf/convert.go,shared/adf/convert_test.go,tools/cfl/pkg/md/roundtrip_test.go; skipped: none; context (not coverage): 1 file; examples:tools/cfl/pkg/md/to_adf.go; constraints: Edge cases were run with a go test -overlay probe at the head SHA; the checkout was not modified. Base-branch behavior for literal entity text is inferred from the diff (raw segment stored), not executed.
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 24s | ~$1.03 (est.) | claude-opus-5-5 | cr 0.10.367
| Field | Value |
|---|---|
| Model | claude-opus-5-5 |
| Reviewers | go:implementation-tests, harness-engineering:repo-health |
| Engine | claude_cli · claude-opus-5-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 2m 24s wall · 3m 34s compute |
| Cost | ~$1.03 (est.) |
| Pricing basis | baseline-2026-10-02/pricing |
| Tokens | 40 in / 17.9k out |
Per-workstream usage
orchestrator-selection— claude-opus-5-5- In: 6
- Out: 1.2k
- Cache read: 25.1k
- Cache create: 13.0k
- Cost: ~$0.13 (est.)
- Duration: 15s
go:implementation-tests— claude-opus-5-5- In: 12
- Out: 8.6k
- Cache read: 103.1k
- Cache create: 26.7k
- Cost: ~$0.41 (est.)
- Duration: 1m 37s
harness-engineering:repo-health— claude-opus-5-5- In: 16
- Out: 7.2k
- Cache read: 164.9k
- Cache create: 30.3k
- Cost: ~$0.42 (est.)
- Duration: 1m 26s
orchestrator-rollup— claude-opus-5-5- In: 6
- Out: 893
- Cache read: 54.2k
- Cache create: 5.4k
- Cost: ~$0.07 (est.)
- Duration: 14s
Scan entity references exactly, so a bare ampersand does not swallow the text after it, and escape an entity a page shows literally on the storage read, so the write's decoding does not strip it.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: b3135fb43896
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| harness-engineering:repo-health | 1 |
go:implementation-tests (1 finding)
Minor - tools/cfl/pkg/md/from_html.go:93
restoreLiteralEntitiesrunsprotectCodeRegionson converter output, but that helper does not recognize backslash-escaped backticks, and the read path escapes literal backticks as\``. A literal entity between two literal backticks is therefore treated as code and gets no&` escape.Probe result: storage
<p>></p>(visible text`>`) reads as markdown\`>\`, andToADFproduces`>`. The text is silently changed on write, which is the corruption this commit set out to remove, and it occurs where pages discuss code without using code marks.Fix: mark only text nodes rather than post-processing markdown, as in the
from_html.go:60finding, so that code is known from the DOM rather than guessed. Otherwise, make the scan skip\``. Add this input toTestRoundtrip_LiteralEntityThroughStorageRead`.
harness-engineering:repo-health (1 finding)
Major - tools/cfl/pkg/md/from_html.go:60
A temporary conversion marker must not reach durable page content, but this substitution leaks it. The regex runs over the whole storage document, including attributes, while
restoreLiteralEntitiesonly recognizes the raw marker in converted text. The converter percent-encodes the marker inside URLs, so it survives the restore. Probing head:<a href="http://x.io/?a=1&b;c=2">reads as[l](http://x.io/?a=1%EE%80%80b%3Bc%3D2), andToADFstores that broken href. Imagesrcvalues read the same way, and the same href inside an[INFO]body also leaks withShowMacroson. The sentinel also collides with real content: a page containing U+E000 reads as\&and is stored as&. Impact:cfl page editsilently rewrites links with;-separated query parameters and a private-use character, which readers only notice when the link breaks. The new round-trip test covers text and code spans only, so nothing catches this. Fix: apply the literal-entity marking to text nodes only, for example with a converter text hook or a walk of the parsed tree, not a regex over markup; that also removes the need for a sentinel. If the sentinel stays, also restore its encoded form and handle existing U+E000 in the input. Add a round-trip case for an href containing&name;.
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (constrained); scope: 6 files; examples:shared/adf/convert.go,shared/adf/convert_test.go,tools/cfl/CHANGELOG.md,tools/cfl/pkg/md/from_html.go,tools/cfl/pkg/md/roundtrip_test.go; 1 omitted; body-inspected: 6 files; examples:shared/adf/convert.go,shared/adf/convert_test.go,tools/cfl/CHANGELOG.md,tools/cfl/pkg/md/from_html.go,tools/cfl/pkg/md/roundtrip_test.go; 1 omitted; skipped: none; context (not coverage): 1 file; examples:tools/cfl/pkg/md/codeprotect.go; constraints: Out of scope: a probe of a macro body with a code span, read with ShowMacros and passed to ToADF, left a CFCODE placeholder in ADF text. The ToADF code is not in this diff, and I did not test the base commit. Probes ran as temporary tests in a scratch copy of the PR head; the review checkout was not modified. The full test suite and linters were not re-run.harness-engineering:repo-health— complete (constrained); scope: 4 files; examples:shared/adf/convert.go,shared/adf/convert_test.go,tools/cfl/pkg/md/from_html.go,tools/cfl/pkg/md/roundtrip_test.go; body-inspected: 4 files; examples:shared/adf/convert.go,shared/adf/convert_test.go,tools/cfl/pkg/md/from_html.go,tools/cfl/pkg/md/roundtrip_test.go; skipped: none; context (not coverage): 1 file; examples:tools/cfl/pkg/md/codeprotect.go; constraints: Edge cases were run with a go test -overlay probe at the head SHA; the checkout was not modified. The pre-diff href behavior is inferred from the diff (no marker existed), not executed.
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 30s | ~$0.80 (est.) | claude-opus-5-5 | cr 0.10.367
| Field | Value |
|---|---|
| Model | claude-opus-5-5 |
| Reviewers | go:implementation-tests, harness-engineering:repo-health |
| Engine | claude_cli · claude-opus-5-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 2m 30s wall · 3m 16s compute |
| Cost | ~$0.80 (est.) |
| Pricing basis | baseline-2026-10-02/pricing |
| Tokens | 24 in / 12.8k out |
Per-workstream usage
go:implementation-tests— claude-opus-5-5- In: 10
- Out: 7.2k
- Cache read: 228.2k
- Cache create: 27.9k
- Cost: ~$0.41 (est.)
- Duration: 1m 35s
harness-engineering:repo-health— claude-opus-5-5- In: 10
- Out: 4.8k
- Cache read: 230.4k
- Cache create: 22.5k
- Cost: ~$0.32 (est.)
- Duration: 1m 19s
orchestrator-rollup— claude-opus-5-5- In: 4
- Out: 856
- Cache read: 44.1k
- Cache create: 4.6k
- Cost: ~$0.06 (est.)
- Duration: 21s
Use the converter's text hook, so attributes and code stay as stored and literal backticks around an entity no longer read as code.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 26b436114f2e
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| harness-engineering:repo-health | 0 |
go:implementation-tests (1 finding)
Minor - tools/cfl/pkg/md/from_html.go:92
Marking only text nodes fixes URLs, but image alt text is also an attribute, and
decodeInlineTextstill decodes it on write (shared/adf/convert.go, the*ast.Imagecase). A literal entity in alt text therefore loses a level on the round trip, which is the behaviormarkLiteralEntitiesexists to prevent.Probe result at the PR head:
<img src="https://e.com/i.png" alt="c &gt; d"/>(alt textc > d) reads as markdown, andToADFproduces the textc > d. Before this PR the alt text was stored raw, so this input survived. It is the same regression the literal-entity work fixed for text nodes, and the previous commit's marker handled it (![c \> d]->c > d), so moving the marker to the text-node hook lost it. No test covers it:TestRoundtrip_LiteralEntityThroughStorageReadhas no image, andTestToDocument_DecodesMarkdownEscapesInTexthas no alt-text case even though alt decoding is a changed code path.The impact is narrow, since it needs an entity shown literally in alt text, but it silently alters stored text because the converter writes alt as the ADF text node.
Fix: also escape literal entities in
altwhen reading, for example by registering animgrenderer or a DOM pre-pass that marks thealtattribute. Add a round-trip case foralt="c &gt; d", plus ashared/adftable case such as->a_b >so the alt decode path is tested directly.
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (constrained); scope: 6 files; examples:shared/adf/convert.go,shared/adf/convert_test.go,tools/cfl/CHANGELOG.md,tools/cfl/pkg/md/from_html.go,tools/cfl/pkg/md/roundtrip_test.go; 1 omitted; body-inspected: 6 files; examples:shared/adf/convert.go,shared/adf/convert_test.go,tools/cfl/CHANGELOG.md,tools/cfl/pkg/md/from_html.go,tools/cfl/pkg/md/roundtrip_test.go; 1 omitted; skipped: none; constraints: Probes ran as temporary tests in a scratch copy of the PR head; the review checkout was not modified.go testpassed for shared/adf and tools/cfl/pkg/md only; other packages, -race and linters were not re-run.harness-engineering:repo-health— complete (constrained); scope: 4 files; examples:shared/adf/convert.go,shared/adf/convert_test.go,tools/cfl/pkg/md/from_html.go,tools/cfl/pkg/md/roundtrip_test.go; body-inspected: 4 files; examples:shared/adf/convert.go,shared/adf/convert_test.go,tools/cfl/pkg/md/from_html.go,tools/cfl/pkg/md/roundtrip_test.go; skipped: none; constraints: Edge cases were run with a go test -overlay probe at the head SHA; the checkout was not modified. The pre-diff image alt behavior is inferred from the diff (raw segment stored), not executed.
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 1m 45s | ~$0.69 (est.) | claude-opus-5-5 | cr 0.10.367
| Field | Value |
|---|---|
| Model | claude-opus-5-5 |
| Reviewers | go:implementation-tests, harness-engineering:repo-health |
| Engine | claude_cli · claude-opus-5-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 1m 45s wall · 2m 11s compute |
| Cost | ~$0.69 (est.) |
| Pricing basis | baseline-2026-10-02/pricing |
| Tokens | 22 in / 10.5k out |
Per-workstream usage
go:implementation-tests— claude-opus-5-5- In: 8
- Out: 5.6k
- Cache read: 277.9k
- Cache create: 21.7k
- Cost: ~$0.34 (est.)
- Duration: 1m 03s
harness-engineering:repo-health— claude-opus-5-5- In: 8
- Out: 4.2k
- Cache read: 259.2k
- Cache create: 17.9k
- Cost: ~$0.28 (est.)
- Duration: 55s
orchestrator-rollup— claude-opus-5-5- In: 6
- Out: 685
- Cache read: 85.9k
- Cache create: 4.2k
- Cost: ~$0.06 (est.)
- Duration: 12s
Reading a Confluence page as markdown and writing it back corrupted its text. The read correctly escapes markdown punctuation (
a\_b,\*,\[) and encodes entities (>). The write stored those characters as written, becauseshared/adf/convert.gotook goldmark's raw text segment, and goldmark leaves decoding to its renderer. Each round trip throughcfl page view --content-onlyandcfl page editshowed readersa\_bandA > B, and repeated round trips added more backslashes.Change
decodeInlineTextapplies CommonMark's backslash escapes and entity references to text nodes and image alt text. Code spans and code blocks keep their source. An escaped entity (\>) stays literal, a backslash before a letter stays, and an unknown entity stays.The storage read now also escapes an entity a page shows literally (
&gt;reads as\>), which it previously rendered the same as>. Without that, the decoder would strip one level per round trip.The converter is shared, so
jtkissue and comment text decodes the same way. Both changelogs say so.Not changed
FromADFdoes not escape on read, so literal text such as*a*or# noread through that path and written back still becomes formatting.cfl's normal markdown read goes through storage, which does escape. That path is only the fallback, so it is left as is.An image's alt text that shows an entity literally (
alt="c &gt; d") loses one level on a round trip, which stays a known gap. It needs a literal entity in alt text, and marking attributes would need its own image renderer.Validation
shared/adftests cover escapes, entities, what stays literal, and code. A newcflround-trip test goes from storage to markdown to ADF. Without the fix, both fail.go test -race ./...passes inshared,tools/cflandtools/jtk.golangci-lint run ./...reports 0 issues in all three.Model: Claude Opus 5.5 (context unknown, xhigh effort) in Claude Code.