feat(#5989): Jira comment write support + tracker.Client for Jira - #5996
Conversation
PR Summary by QodoAdd Jira comment write support via ADF conversion and tracker JiraClient
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by Qodo
1.
|
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep — 1 HIGH and 2 MEDIUM findings, focused on the new ADF conversion/write path and its overlap with existing jirapoll code.
|
Ran this against a real Jira instance (stage-redhat) to sanity check the whole path: GetIssue, ListComments, CreateComment, UpdateComment against KONFLUX-13045. All four worked end to end, including the markdown->ADF conversion on create and the round trip back through ADFToPlainText on a follow-up ListComments. Posted a comment with bold/italic/link/list/fenced-code, then edited it, to check both directions: https://stage-redhat.atlassian.net/browse/KONFLUX-13045?focusedCommentId=17718459 Oh, and one thing I noticed along the way: that issue has an existing comment with an ADF table in it, and ListComments flattens it to separator-less text (cells run together). Table markup isn't in the block/inline vocabulary adf.go documents as supported, so that tracks with the design, but wanted to flag it here in case tables show up often enough in practice to be worth a follow-up issue. |
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep — 2 HIGH and 2 MEDIUM findings on the ADF conversion/comment-write path and the tracker.Client Jira adapter.
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep — 1 HIGH and 4 MEDIUM new findings, all on the markdown→ADF write path (models: Claude, Gemini). The four still-open items from the previous sweep (parse-time DoS, dropped updateAuthor, silent block-level drop, unvalidated hrefs) remain valid at this head and are not re-posted.
… links Addresses review feedback from waynesun09 on PR #5996: - Bound goldmark's Parse() input size: the existing maxADFWriteDepth cap only bounded the post-parse AST walk, but Parse() itself is ~O(N^2) on deeply nested blockquotes and dominates the cost long before that cap is ever reached (benchmarked at ~3.2s to parse 80,000 nesting levels). - Fall back to a plain-text paragraph for block-level markdown outside the supported vocabulary (e.g. raw HTML blocks), instead of silently dropping it, mirroring walkInline's existing plain-text fallback for unknown inline nodes. - Reject unsafe link schemes (javascript:, data:, vbscript:, ...) on both markdown links and autolinks, allowing http/https/mailto and schemeless relative/anchor links through. - Make blockquote/listItem content schema-conformant: headings degrade to bold paragraphs, nested blockquotes flatten into their parent, and thematic breaks are dropped, since ADF's schema for those containers only allows paragraph/list/codeBlock/media children and Jira Cloud rejects anything else with a 400. - Drop a container (blockquote/bulletList/orderedList/listItem) entirely rather than emit it with empty content, which ADF's minItems: 1 also forbids — reachable via the depth cutoff above or an unsupported-only child. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Addresses review feedback from waynesun09 on PR #5996: fromJiraComment set tracker.Comment.Author to the original author even when a comment was edited, ignoring Jira's UpdateAuthor field. jirapoll/discover.go already attributes edit-detected events to the editor for this reason (ADR 0054) — someone with Edit-All-Comments can rewrite another user's comment, and running that rewritten content under the original author's identity would be a way to launder untrusted input through a trusted name. No consumer of tracker.Client exists yet, but nothing about Comment.Author signals this caveat, so this closes the gap before a future authorization-sensitive caller trips over it. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 6:51 PM UTC · Completed 7:05 PM UTC Commit: |
ReviewFindingsLow
Previous runLooks good to me Previous run (2)ReviewFindingsLow
Labels: PR adds Jira tracker client integration (new feature) Previous run (3)ReviewFindingsLow
Previous run (4)ReviewFindingsLow
Previous run (5)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (6)ReviewFindingsLow
Previous run (7)ReviewFindingsHigh
Medium
Low
Next steps:
|
waynesun09
left a comment
There was a problem hiding this comment.
Review-squad sweep (3 agents: Claude, Grok) — 3 HIGH and 4 MEDIUM new/unresolved findings on the ADF write path and the tracker.Client Jira adapter, all independently verified against this head. One (the link-mark leak) was already raised by an earlier automated review but never landed as an inline comment (GitHub 422'd it against a stale line number); reposting here at the correct current line. The twelve findings from prior review rounds that are already fixed are not repeated.
|
Re: #5996 (comment) The high finding here is the same marks-leak bug flagged inline (and already fixed in a1f367a, with a regression test asserting text after the link carries no mark, which covers the test-gap note too). The The one still-open item is the low one: a paragraph whose only inline child gets dropped (e.g. an image with no alt text and no other content) could emit |
|
🤖 Finished Review · ✅ Success · Started 9:08 PM UTC · Completed 9:24 PM UTC Commit: |
Superseded by updated review
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep (review-only). 1 CRITICAL, 1 HIGH, 1 MEDIUM finding below, none overlapping prior comments on this PR.
Jira Cloud's comment/description fields require Atlassian Document Format (ADF), not markdown, so writing a comment needs a markdown-to-ADF converter, and reading tracker.Client's plain-text Issue.Body/Comment.Body back out needs an ADF-to-plain-text one. MarkdownToADF parses source with goldmark and walks its AST into ADF doc/paragraph/heading/list/blockquote/codeBlock nodes plus strong/em/code/link/hardBreak inline marks. ADFToPlainText is a fresh implementation, not a refactor of jirapoll's existing extractPlainText/walkADFNode: #5989 scopes out read-side changes, and moving the private helpers would touch jirapoll and its tests. Same maxADFDepth=50 recursion cap, for the same reason (attacker-controlled nesting). Adds github.com/yuin/goldmark, a small pure-Go CommonMark parser already widely used in the Go ecosystem (e.g. Hugo), rather than hand-rolling markdown parsing. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Jira Cloud REST v3 comment endpoints: POST /issue/{key}/comment to
create, PUT /issue/{key}/comment/{id} to update. Both take the body
as ADF, converted from the markdown callers pass in via
MarkdownToADF (added in a prior commit). 404/403 already unwrap to
forge.ErrNotFound/ErrForbidden via APIError.Unwrap, so no new error
handling is needed beyond the existing wrapping convention.
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
JiraClient adapts a Jira client to tracker.Client, mapping (project, number) to the Jira issue key "PROJ-123" and converting between tracker.Issue/Comment's plain-text Body and Jira's ADF via jira.ADFToPlainText / jira.LiveClient's ADF-based comment writes. CreateComment sets the returned Comment's Body to the caller's original markdown rather than round-tripping through the ADF Jira echoes back, since that round trip is lossy and the caller already has the exact text verbatim. Comment.HTMLURL is left unset — Jira's comment permalink format isn't confirmed against real Cloud behavior, so guessing at a URL risks a broken link. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…lient tracker.JiraClient stored baseURL with only strings.TrimRight, unlike jira.LiveClient's ValidateBaseURL, so a base URL containing embedded credentials (https://user:token@host) would propagate them into every Issue.URL this client returns. Export jira's existing validation and reuse it in NewJiraClient. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
CreateComment overwrote the returned Comment.Body with the input markdown, while ListComments/GetIssue always derive Body via ADFToPlainText. That made tracker.Comment.Body mean different things depending on which method produced it. Drop the override so CreateComment returns the same plain-text representation everywhere. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
adfBlockContent/convertBlockNode/walkInline recursed once per markdown nesting level with no depth cap, unlike the read-side ADFToPlainText/ walkADFNode pair added in the same file, which caps recursion at maxADFDepth for exactly this reason. Deeply nested input (e.g. thousands of blockquote markers) showed clear superlinear blowup. Thread a depth counter through the write-path converters and drop content past maxADFWriteDepth, mirroring the existing "drop what we don't support" behavior for unrecognized node types. Also fix a comment on maxADFDepth that pointed at a package doc comment explaining the read/write duplication with jirapoll — that doc comment doesn't exist. State the reasoning directly instead. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
… links Addresses review feedback from waynesun09 on PR #5996: - Bound goldmark's Parse() input size: the existing maxADFWriteDepth cap only bounded the post-parse AST walk, but Parse() itself is ~O(N^2) on deeply nested blockquotes and dominates the cost long before that cap is ever reached (benchmarked at ~3.2s to parse 80,000 nesting levels). - Fall back to a plain-text paragraph for block-level markdown outside the supported vocabulary (e.g. raw HTML blocks), instead of silently dropping it, mirroring walkInline's existing plain-text fallback for unknown inline nodes. - Reject unsafe link schemes (javascript:, data:, vbscript:, ...) on both markdown links and autolinks, allowing http/https/mailto and schemeless relative/anchor links through. - Make blockquote/listItem content schema-conformant: headings degrade to bold paragraphs, nested blockquotes flatten into their parent, and thematic breaks are dropped, since ADF's schema for those containers only allows paragraph/list/codeBlock/media children and Jira Cloud rejects anything else with a 400. - Drop a container (blockquote/bulletList/orderedList/listItem) entirely rather than emit it with empty content, which ADF's minItems: 1 also forbids — reachable via the depth cutoff above or an unsupported-only child. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Addresses review feedback from waynesun09 on PR #5996: fromJiraComment set tracker.Comment.Author to the original author even when a comment was edited, ignoring Jira's UpdateAuthor field. jirapoll/discover.go already attributes edit-detected events to the editor for this reason (ADR 0054) — someone with Edit-All-Comments can rewrite another user's comment, and running that rewritten content under the original author's identity would be a way to launder untrusted input through a trusted name. No consumer of tracker.Client exists yet, but nothing about Comment.Author signals this caveat, so this closes the gap before a future authorization-sensitive caller trips over it. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 3:36 PM UTC · Completed 3:49 PM UTC Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Review-squad sweep, round 4 (3 agents; models: Claude, Grok) — 4 HIGH and 3 MEDIUM new findings, all independently verified by execution against this head (7e932b2). All prior-round fixes were re-verified and hold; every finding below is on the new ADFToMarkdown surface and its round trip with MarkdownToADF, not a regression. The common theme: the pair is only tested against ADF shapes goldmark itself produces, never the shapes Jira Cloud's editor actually emits (split mark runs, panels, mentions, embedded fences) — fixtures sourced from real Jira Cloud responses would catch most of these classes at once.
One note on an existing unreplied thread rather than a new comment: the applyADFMarks no-markdown-escaping finding (raised earlier as low) has new escalation evidence — an ADF paragraph whose literal text is # not a heading round-trips through MarkdownToADF into a real heading node, and - /> /1. prefixes similarly change node types. That's block-structure corruption, not just mark leakage; worth re-grading to medium. Two LOW findings (trailing-hardBreak literal backslash; -only input bypassing the fail-closed empty-doc guarantee) and one LOW consistency note (Jira-backed tracker.Comment.CreatedAt is not RFC3339, unlike the forge backends) were held back per the medium-and-above posting scope.
|
/fs-fix address all findings |
|
🤖 Finished Fix · ✅ Success · Started 4:55 PM UTC · Completed 5:06 PM UTC Commit: |
…Markdown - ADFToMarkdown's codeBlock rendering no longer emits a spurious blank line between fences when the code block has no text children. - applyADFMarks now escapes markdown-significant characters (*, _, `, [, ], \) in text nodes before wrapping in formatting delimiters, so Jira-native ADF with literal special characters renders unambiguously. Text inside a code mark is left unescaped since code spans are verbatim. Addresses review feedback on #5996
🔧 Fix agent — iteration 2 (human-triggered)Addressed 2 of 3 review findings: fixed empty codeBlock blank-line edge case and added markdown-significant character escaping in applyADFMarks. Disagreed with 1 finding (fullsend-fix label is operational, not commit-type). Fixed (2):
Disagreed (1):
Tests: passed Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 5:07 PM UTC · Completed 5:19 PM UTC Commit: |
|
/fs-fix Follow-up on 63bd8eb ("handle empty codeBlock and escape markdown chars in ADFToMarkdown"): the empty-codeBlock fix is complete, but the escaping fix is partial. Two gaps remain in 1. Escape line-leading block syntax in ADFToMarkdown text output (the block-structure corruption case is still live)
Fix: in unmarked text, backslash-escape block-syntax characters when they appear at the start of the rendered line ( 2. Escape Literal ADF text 3. The 7 open findings from the round-4 inline review (4 HIGH, 3 MEDIUM), each with repro details and suggested fixes in its thread:
All fixes should come with local regression tests in |
|
🤖 Finished Fix · ❌ Failure · Started 6:12 PM UTC · Completed 6:26 PM UTC Commit: |
The cfmint behaviour-test driver (#6037) writes FULLSEND_MINT_URL as a repo-level variable on test-repo via `fullsend github setup`. Repo variables take precedence over org-level variables of the same name, but nothing deleted it afterward. Since e2e admin tests and behaviour tests share the same org pool and both use a repo named test-repo, a leaked repo-level FULLSEND_MINT_URL from a prior behaviour-test run shadows the org-level value that admin install sets, and points to an already-torn-down CF Worker preview mint — breaking dispatch with a DNS resolution failure for any run that later reuses the org. Delete the repo variable in TeardownPerRepoInstall (immediate cleanup after a per-repo driver run) and in CleanupStaleResources (defensive cleanup so already-poisoned orgs in the pool self-heal). Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Review · ❌ Terminated · Started 6:24 PM UTC · Ended 6:36 PM UTC Commit: |
|
The fix agent completed, but the post-fix script failed before finishing. Workflow run: https://github.com/fullsend-ai/.fullsend/actions/runs/31521387295 Details: |
|
🤖 Finished Review · ✅ Success · Started 6:24 PM UTC · Completed 6:36 PM UTC Commit: |
Fixes 9 issues from the second round of review (waynesun09 plus two automated review agents): - Clamp heading level and ordered-list start to valid ranges instead of panicking or emitting out-of-spec Markdown. - Compute the code-fence length from the longest backtick run in the block's content, and sanitize the language attr, so both can no longer be used to break out of the fence. - Fall back to recursing into block children (then inline text) for unknown ADF container nodes (panel, table, expand, taskList, ...) instead of dropping their content. - Render mention/emoji/status/date/inlineCard atom nodes as their visible text instead of vanishing them. - Preserve an ADF Image's destination as a link on its alt text in MarkdownToADF, instead of dropping it. - Make withMark replace an existing link mark in place (inner-most wins) rather than treating it as a duplicate, matching CommonMark's resolution for a Markdown link wrapping an autolink. - Escape ampersand and line-leading block syntax (#, >, -, +, ordered list markers) in ADFToMarkdown's unmarked text, and resolve backslash escapes and HTML entity/numeric references in a single pass on the MarkdownToADF side — doing it in two passes let an escaped "&" get re-interpreted as the start of an entity reference. - Coalesce adjacent inline nodes with identical mark sets before wrapping them, and hoist leading/trailing whitespace outside emphasis delimiters, so real Jira-Cloud ADF (which commonly splits one bold run across several same-marked text nodes) round-trips instead of corrupting into literal asterisks. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Review · ❌ Terminated · Started 6:59 PM UTC · Ended 7:15 PM UTC Commit: |
|
The automated fix run on this one failed partway through, so I went through all 9 findings by hand in 22e3027 — the two escaping gaps here plus the 7 from the inline review. Each one has a regression test in adf_test.go. Replied on each inline thread with specifics. Let me know if anything's still off. |
|
🤖 Finished Review · ✅ Success · Started 6:59 PM UTC · Completed 7:14 PM UTC Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Approving after five review rounds at head 22e3027.
Code: All 29 findings raised across the review history (including 4 HIGH and 3 MEDIUM in the final round on the ADFToMarkdown surface) are fixed and regression-tested — each fix independently re-verified by execution at this head: block-prefix/entity escaping, panel/taskList content preservation, mark-run coalescing, fence-length computation with language-attr sanitization, heading/order clamping (no panic on malformed ADF), inline-atom rendering, inner-link-wins mark replacement, and image-as-link degradation. Round trips of normal documents are byte-stable; internal/forge/jira and internal/tracker suites pass.
API: Purely additive — new exported jira.MarkdownToADF/ADFToMarkdown/ADFToPlainText/ValidateBaseURL and tracker.NewJiraClient; tracker.Client contract satisfied end-to-end including the IsNotFound semantics and Body type. One new dependency (goldmark v1.8.5 — MIT, pure Go, the de-facto standard CommonMark parser).
Workflow impact: No .github/workflows/ changes; all checks green (behaviour, e2e, unit, codecov/patch, DCO, commit-lint). The one out-of-scope commit (1230d66, e2e test-repo variable cleanup) is small, well-justified test-infra hygiene.
Remaining open items are two LOW bot findings (unescaped ) in link hrefs — confirmed real but minor, same escaping family; and the goldmark scope-attribution note) — neither blocks merge and both are reasonable follow-ups.
|
🤖 Finished Retro · ✅ Success · Started 9:15 PM UTC · Completed 9:43 PM UTC Commit: |
Retro Analysis: PR #5996What happenedThis human-authored PR by ralphbean added markdown↔ADF conversion ( The fix agent contributed 2 clean commits (e81ff80, 63bd8eb) addressing 6 findings with no regressions. A third fix agent run (31521387295) completed its work but lost a push race when ralphbean pushed an unrelated commit (e2e test cleanup) concurrently — What went well
Evidence for existing issues
Proposals1 new proposal filed (review agent 422 stale-diff handling). Proposals filed |
Summary
internal/forge/jira/adf.go) usinggoldmark, since Jira Cloud's comment/description fields require Atlassian
Document Format rather than markdown.
CreateComment/UpdateCommentto the Jira REST client(
internal/forge/jira/client.go), using Jira Cloud REST v3.tracker.JiraClient(internal/tracker/jira_client.go), atracker.Clientimplementation backed by the Jira client, mapping(project, number)to the Jira issue keyPROJECT-NUMBER.Stacked on #5993 (adds
numbertotracker.Client.UpdateComment, neededbecause Jira's update-comment endpoint requires the issue key alongside the
comment ID). Base branch is
agent/5988-tracker-clientso this shows asstacked; will need rebasing onto
mainonce #5993 merges.Out of scope (per #5989):
forge.Clientfor Jira (Jira isn't a forge), andCLI wiring (tracked separately as a follow-up to #5991).
Closes #5989.
Test plan
go test ./internal/forge/jira/... ./internal/tracker/...go build ./...go vet ./...pre-commit run(gofmt, go vet) on changed filesgo test ./...— passes except two pre-existing, unrelated failuresalso present on
origin/main(internal/scaffoldTestFileModeMatchesFilesystem,internal/runtimeTestDummyRuntime_Bootstrap/TestDummyRuntime_ClearIterationArtifacts)