fix(#6148): map <details> blocks to ADF expand nodes in Jira converter - #6814
fix(#6148): map <details> blocks to ADF expand nodes in Jira converter#6814fullsend-ai-coder[bot] wants to merge 5 commits into
Conversation
MarkdownToADF now converts HTML <details>/<summary> blocks into Jira's ADF expand node type instead of falling back to raw-text paragraphs containing literal HTML markup. This handles both single-block (no blank lines inside) and multi-block (blank lines split across AST siblings, as produced by sticky.BuildUpdatedBody) layouts. Sticky history sentinels (<!-- sticky:history-start/end -->) are consumed as parser metadata during conversion and do not appear as visible text inside the rendered expansion. ADFToMarkdown now renders expand nodes back as <details>/<summary> HTML, providing round-trip fidelity: <details> → expand → <details> converges to a stable format after one cycle. Note: pre-commit could not fetch remote hook repos (network-restricted sandbox). Local hooks (gofmt, go vet) and secret scan passed. Remote hooks (pre-commit-hooks, gitleaks, ruff, etc.) were not run. Closes #6148
|
🤖 Finished Review · ✅ Success · Started 5:56 PM UTC · Completed 6:15 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.81 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Risk Assessment: moderate (2/5) DetailsModerate risk unchanged from prior assessment: Tier 1 signals remain stable (3 files, no protected paths, no dependency or CI changes, bot author, 33% test ratio) despite a large blast radius tag; high churn and fix density in the ADF converter area continues to indicate an actively stabilizing subsystem but is offset by well-scoped changes proportionate to the linked issue and good test coverage. Previous runRisk Assessment: moderate (2/5) DetailsModerate risk unchanged from prior assessment: Tier 1 signals remain stable (3 files, no protected paths, no dependency or CI changes, bot author, 33% test ratio) despite a large blast radius tag; high churn and fix density in the ADF converter area continues to indicate an actively stabilizing subsystem but is offset by well-scoped changes proportionate to the linked issue and good test coverage. Previous run (2)Risk Assessment: moderate (2/5) DetailsModerate risk driven primarily by high churn and fix density in the ADF converter area (12 commits and 11 fixes in 30 days), indicating an actively stabilizing subsystem. The change itself is well-scoped: 2 files, strong 50% test ratio, no protected paths or security concerns, bot author with prior contributions, and proportionate to the linked issue scope. Score unchanged from prior assessment as Tier 1 signals remain stable and Tier 2/3 show no material shifts. Previous run (3)Risk Assessment: moderate (2/5) DetailsModerate risk driven primarily by high churn and fix density in the ADF converter area (12 commits and 11 fixes in 30 days), indicating an actively stabilizing subsystem. The change itself is well-scoped: 2 files, strong 50% test ratio, no protected paths or security concerns, bot author with prior contributions, and proportionate to the linked issue scope. Score unchanged from prior assessment as Tier 1 signals remain stable and Tier 2/3 show no material shifts. Previous run (4)Risk Assessment: moderate (2/5) DetailsModerate risk driven primarily by high churn and fix density in the ADF converter area (12 commits and 11 fixes in 30 days), indicating an actively stabilizing subsystem. The change itself is well-scoped: 2 files, strong 50% test ratio, no protected paths or security concerns, bot author with prior contributions, and proportionate to the linked issue scope. |
ReviewFindingsMedium
Low
Previous runReviewFindingsLow
Previous run (2)ReviewFindingsMedium
Low
Next steps:
Previous run (3)ReviewFindingsLow
Next steps:
Previous run (4)ReviewFindingsLow
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 6:17 PM UTC · Completed 6:25 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.83 |
- Emit minimal expand node with empty paragraph when multi-block <details> body contains only sticky sentinels, preventing raw HTML from appearing in Jira comments. - HTML-escape expand title in ADFToMarkdown to prevent injection via </summary> in attrs.title. - Rename containsDetailsClose to hasDetailsClose for naming consistency with other boolean helpers in the file. - Clarify tryDetailsExpand godoc return description to avoid overlap with the expandNode helper function name. - Document detailsInnerBody limitation with attributed <details> tags. - Add tests: empty-body multi-block, nested details limitation, and HTML-escaped title round-trip. Addresses #6814
🔧 Fix agent — iteration 1 (bot-triggered)Addressed all 6 review findings: fixed empty-body multi-block silent content loss by emitting a minimal expand node, HTML-escaped expand titles in ADFToMarkdown, renamed containsDetailsClose to hasDetailsClose for naming consistency, clarified godoc to avoid overlap with helper name, documented detailsInnerBody limitation, and added tests for nested details limitation and HTML-escaped title round-trip. Fixed (6):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 6:27 PM UTC · Completed 6:47 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.92 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 6:49 PM UTC · Completed 6:56 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.75 |
- Rename shadowed `html` variable to `htmlBlock` in tryDetailsExpand to match the multi-block path's naming and avoid shadowing the imported html package. - Decode HTML entities in extractSummary with html.UnescapeString so pre-existing entities (e.g. &) are not double-encoded when adfMarkdownBlock re-encodes the title with html.EscapeString. - Move TestMarkdownToADF_DetailsExpandRoundTrips into the MarkdownToADF section to match the file's direction-based grouping. - Add TestMarkdownToADF_DetailsSummaryWithHTMLEntities exercising the entity round-trip fix. Addresses #6814
🔧 Fix agent — iteration 2 (bot-triggered)Fixed HTML entity double-encoding in extractSummary, renamed shadowed html variable to htmlBlock, moved round-trip test to correct section, and added entity round-trip test. Disagreed on nesting-depth finding as it is already documented by an existing test. Fixed (3):
Disagreed (1):
Tests: passed Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 6:58 PM UTC · Completed 7:12 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.10 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 7:14 PM UTC · Completed 7:21 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.31 |
The duplicate isBlockType in internal/jirapoll/discover.go was not updated when "expand" was added to internal/forge/jira/adf.go's copy, causing walkADFNode to omit newlines between block-level siblings inside or around expand nodes in plain-text extraction. Addresses #6814
🔧 Fix agent — iteration 3 (bot-triggered)Added "expand" to jirapoll's duplicate isBlockType function to match adf.go, fixing newline omission around expand nodes in plain-text extraction. The nested details nesting limitation is already documented by an existing test. Fixed (1):
Disagreed (1):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 7:23 PM UTC · Completed 7:39 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.81 |
Superseded by updated review
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
ready-for-merge
|
/fs-fix fix the HTML injection issue |
|
🤖 Finished Fix · ✅ Success · Started 8:21 PM UTC · Completed 8:33 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.65 |
…injection extractSummary previously preserved HTML tags found inside <summary> elements (e.g. <script>, <img onerror=...>, <a>) and stored them verbatim in the ADF expand title attribute. If the consuming renderer interpreted the title as HTML, this created an injection risk. Now decode entities first (to avoid double-encoding on round-trip), then strip all HTML tags with a regex, ensuring the ADF title is guaranteed plain text. Addresses #6814
🔧 Fix agent — iteration 4 (human-triggered)Fixed HTML injection in extractSummary by stripping HTML tags from Fixed (1):
Tests: passed
Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 8:35 PM UTC · Completed 8:53 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.36 |
fullsend-ai-review
Bot
added
requires-manual-review
1 participant
Summary
MarkdownToADFnow converts HTML<details>/<summary>blocks into Jira's ADFexpandnode type, andADFToMarkdownrendersexpandnodes back as<details>/<summary>HTML. Previously,<details>blocks fell through to a raw-text fallback, displaying literal HTML markup in Jira comments.Changes
MarkdownToADF): AddedtryDetailsExpandinadfBlockContentto detect<details>HTML blocks and convert them to ADFexpandnodes. Handles both single-block (no blank lines inside the markup) and multi-block (blank lines split across AST siblings, as produced bysticky.BuildUpdatedBody) layouts.<!-- sticky:history-start -->and<!-- sticky:history-end -->comments are consumed as parser metadata during conversion and do not appear as visible text inside the rendered expansion.ADFToMarkdown): Addedexpandcase inadfMarkdownBlockthat renders the node as<details><summary>TITLE</summary>\nBODY\n</details>.<details>→expand→<details>converges to a stable format after one cycle.<details>as an example of "unknown" blocks to use<div>instead, and removed theexpandentry from the "unknown container" test table.Testing
go vet ./internal/forge/jira/...passesgo test ./internal/forge/jira/passes (all 66 tests)tryDetailsExpand94.6%, all other new functions 100%Closes #6148
Post-script verification
agent/6148-details-to-adf-expand)b92ce41f54d5b93410e4e153fac6c4eed0e7c88e..HEAD)