feat(#5989): Jira comment write support + tracker.Client for Jira - #5996
feat(#5989): Jira comment write support + tracker.Client for Jira#5996ralphbean wants to merge 6 commits into
Conversation
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>
ralphbean
added
the
fullsend-fix
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.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
waynesun09
left a comment
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. |
…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>
waynesun09
left a comment
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.
| // Markdown constructs outside that vocabulary (tables, images, raw HTML, | ||
| // footnotes, ...) are silently dropped rather than rendered as something | ||
| // ADF doesn't understand. | ||
| func MarkdownToADF(source string) map[string]any { |
There was a problem hiding this comment.
[HIGH] 133ecd4's depth-cap fix does not mitigate the MarkdownToADF DoS — bottleneck is goldmark's Parse(), not the post-parse walk
I independently re-ran the DoS benchmark against this PR's current head (133ecd4, built and executed in a throwaway worktree) with markdown consisting of N repeated > blockquote markers, timing goldmark.DefaultParser().Parse() alone vs. the full MarkdownToADF call:
| N | parse-only | full MarkdownToADF |
|---|---|---|
| 10000 | 52.8ms | 52.1ms |
| 20000 | 206ms | 207ms |
| 40000 | 828ms | 829ms |
| 80000 | 3.26s | 3.20s |
Parse-only and full-conversion timings are statistically identical at every N, confirming essentially 100% of the cost is incurred inside goldmark's own blockquote-nesting parser, which fully builds the AST before adfBlockContent/convertBlockNode/walkInline (and the new maxADFWriteDepth=50 cap) ever run. Capping the post-parse conversion walk doesn't bound the actual CPU cost, since goldmark.Parse() itself already exhibits ~O(N^2) blowup on this input before the cap-checking code is reached. A caller feeding a few hundred KB of deeply-nested markdown into MarkdownToADF (and thus into LiveClient.CreateComment/UpdateComment) still blocks for seconds to minutes despite the fix. The thread on this line was marked resolved after 133ecd4 landed, but that resolution addressed only the AST-walk depth, not the underlying parse-time algorithmic-complexity issue.
Suggestion: either (a) impose a size limit on markdown input length before calling goldmark.DefaultParser().Parse() (reject/truncate bodies over some reasonable size, consistent with Jira comment size limits), or (b) check whether goldmark's parser exposes an option that bounds parse-time nesting work directly (as opposed to just AST-walk depth). Consider adding a regression test asserting MarkdownToADF returns within a bounded time for adversarially nested input, since the existing TestMarkdownToADF_DeepNestingIsBounded only checks output shape and passes despite the unbounded parse cost.
| return Comment{ | ||
| ID: c.ID, | ||
| Body: jira.ADFToPlainText(c.Body), | ||
| Author: c.Author.DisplayName, |
There was a problem hiding this comment.
[HIGH] fromJiraComment drops updateAuthor, discarding the edit-attribution safeguard ADR 0054 relies on
fromJiraComment maps jira.Comment to tracker.Comment using only c.Author.DisplayName, and tracker.Comment has no field to carry UpdateAuthor at all, so the information is unrecoverably lost at this adapter boundary. jira.Comment's own doc comment spells out why this matters: Author is the account that originally created the comment; UpdateAuthor is the account that last modified it — they differ when someone with Edit-All-Comments edits another user's comment, and a poller must attribute an edit-detected event to the editor, not the author, to avoid running attacker-authored text under the original author's role. internal/jirapoll/discover.go actually implements this fallback (if edited && comment.UpdateAuthor.AccountID != "" { author = comment.UpdateAuthor }) with an explicit comment noting the bypass risk for ADR 0054's authorization gate.
tracker.JiraClient/tracker.NewJiraClient currently have zero non-test callers, so there's no active exploit today — but the whole point of tracker.Client is to let a shared, backend-agnostic comment-handling workflow run across GitHub/GitLab/Jira. If a future consumer authorizes actions based on tracker.Comment.Author the way jirapoll authorizes based on updateAuthor, this silently reintroduces the privilege-escalation path ADR 0054 was written to close, and tracker.Comment doesn't even have a field to hold the correct value.
Suggestion: either (a) add an explicit attribution field to tracker.Comment (e.g. set Author from c.UpdateAuthor.DisplayName when UpdateAuthor.AccountID is non-empty, falling back to c.Author.DisplayName otherwise, mirroring jirapoll's fallback logic), or (b) if tracker.Comment is intentionally scoped to non-authorization use, document that restriction loudly on the Comment.Author field/tracker.Client interface, and add a test mirroring jirapoll's editor-vs-author adversarial case to lock in whichever behavior is chosen.
| return node | ||
| case *ast.ListItem: | ||
| return map[string]any{"type": "listItem", "content": adfBlockContent(n, source, depth+1)} | ||
| default: |
There was a problem hiding this comment.
[MEDIUM] Block-level markdown outside the supported vocabulary (e.g. raw HTML blocks) is silently dropped with zero fallback, unlike the inline walker
convertBlockNode's switch only handles Paragraph/TextBlock/Heading/ThematicBreak/Blockquote/CodeBlock/FencedCodeBlock/List/ListItem; its default: return nil means any other block-level node — including a goldmark HTMLBlock, which is core CommonMark requiring no extension — is dropped entirely with no trace: no log, no placeholder, no error surfaced from CreateComment/UpdateComment. This is a real asymmetry with walkInline's own default case, which explicitly falls through to walk unknown inline nodes (Image, RawHTML) as plain text "so at least the readable content isn't lost" — no equivalent exists at the block level. Since tracker.Client's purpose is to unify comment-posting across GitHub/GitLab/Jira, and this repo has an established convention (internal/sticky, internal/cli/postcomment.go, postreview.go) of wrapping prior-run output in <details><summary>...</summary>...</details> HTML blocks to avoid comment flooding, a future Jira-wired caller reusing that same convention would have every such block silently vanish from the posted Jira comment with no indication anywhere that content was lost.
Suggestion: extend convertBlockNode to handle ast.HTMLBlock (e.g. render its raw text as a plain paragraph/codeBlock, since ADF has no native HTML node) rather than dropping it, or have MarkdownToADF/CreateComment/UpdateComment surface when block-level content was dropped (e.g. return a count or log a warning). At minimum add a test asserting current behavior on an HTMLBlock-containing input (e.g. <details><summary>x</summary>y</details>) so the drop is documented, intentional, test-locked behavior instead of an unverified assumption.
| } | ||
| walkInline(v, source, withMark(marks, map[string]any{"type": markType}), out, depth+1) | ||
| case *ast.Link: | ||
| attrs := map[string]any{"href": string(v.Destination)} |
There was a problem hiding this comment.
[MEDIUM] No URL scheme validation when converting markdown links/autolinks to ADF href attributes
walkInline's *ast.Link case (attrs := map[string]any{"href": string(v.Destination)}) and *ast.AutoLink case ("href": string(v.URL(source))) put the markdown link destination directly into the ADF mark's href attribute with no scheme allowlist or validation. MarkdownToADF's own doc comment on maxADFWriteDepth explicitly anticipates "a future caller could feed external content into it (e.g. quoting an issue or comment body)" — i.e. this is expected to eventually process less-trusted markdown. A javascript: or data: URI in such content would flow unfiltered into the ADF document sent to Jira, with no client-side defense-in-depth comparable to what ValidateBaseURL already applies to base URLs elsewhere in this same package.
Suggestion: restrict accepted href schemes to http/https (and mailto for AutoLink) before emitting the link mark, dropping or neutralizing anything else, mirroring the scheme/credential checks already applied by ValidateBaseURL elsewhere in this package.
waynesun09
left a comment
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.
| case *ast.ThematicBreak: | ||
| return map[string]any{"type": "rule"} | ||
| case *ast.Blockquote: | ||
| return map[string]any{"type": "blockquote", "content": adfBlockContent(n, source, depth+1)} |
There was a problem hiding this comment.
[HIGH] Valid CommonMark nesting produces schema-invalid ADF that Jira Cloud rejects
convertBlockNode emits any recognized block type as a child of blockquote (here) and listItem (line 99), but Atlassian's ADF schema restricts their content: blockquote allows only paragraph/bulletList/orderedList/codeBlock/media nodes; listItem allows only paragraph/bulletList/orderedList/codeBlock/mediaSingle. Running this head's converter confirms it produces invalid documents for perfectly legal CommonMark: > # title (heading inside blockquote), > > inner (nested blockquote), > --- (rule inside blockquote), - # h (heading inside listItem), - > q (blockquote inside listItem). Jira Cloud validates ADF on write, so a comment containing any of these constructs fails with 400 INVALID_INPUT and the entire write is lost — agent-generated markdown that quotes other content (nested >) is a realistic trigger.
Suggestion: make conversion context-aware inside blockquote/listItem — degrade unsupported children instead of emitting them (heading → paragraph with strong mark, nested blockquote → flattened paragraphs, rule → dropped or hoisted) — and add table-driven tests for each construct. (Verified against the published ADF schema; not live-tested.)
| func codeBlockNode(text []byte, lang string) map[string]any { | ||
| node := map[string]any{ | ||
| "type": "codeBlock", | ||
| "content": []any{map[string]any{"type": "text", "text": strings.TrimRight(string(text), "\n")}}, |
There was a problem hiding this comment.
[MEDIUM] Empty code block emits a zero-length text node, which the ADF schema forbids — the whole comment write fails
codeBlockNode unconditionally emits {"type":"text","text":...} as the codeBlock's content. For an empty fenced block (an opening fence immediately followed by a closing fence, or a body that is only newlines after TrimRight), this produces a text node with "text":"" — confirmed empirically by running MarkdownToADF from this head. ADF requires text-node text to be non-empty (minLength: 1), so Jira rejects the entire document with a 400, turning a legitimate markdown comment into a hard failure of the whole CreateComment/UpdateComment call. appendADFText (line 185-188) already guards exactly this on the inline path; the code-block path missed the same guard.
Suggestion: skip the text child when the trimmed content is empty — codeBlock content is optional in the ADF schema, so a bare {"type":"codeBlock"} is valid. Add a test converting an empty fenced block and asserting no empty text node is produced.
| // how unsupported node types are already handled. | ||
| func adfBlockContent(parent ast.Node, source []byte, depth int) []any { | ||
| content := []any{} | ||
| if depth > maxADFWriteDepth { |
There was a problem hiding this comment.
[MEDIUM] Dropped/truncated children leave container nodes with content: [], violating ADF minItems: 1 — and the 133ecd4 depth cap itself produces such output
Per the ADF schema, blockquote, listItem, bulletList, and orderedList all require content with at least one element. Two empirically confirmed triggers at this head: (a) a list item whose only child is an unsupported block — converting - <div>item html</div> yields a listItem with "content":[] (realistic input given this repo's <details>-wrapping convention in internal/sticky/postcomment.go); (b) this depth cap — 60 nested blockquotes leave an innermost {"type":"blockquote","content":[]} at the cap boundary, so the "drop content past the limit" degradation strategy emits ADF Jira will reject rather than degrading gracefully. This is distinct from the open silent-drop thread on line 100: the failure mode here is not lost content but a schema-invalid document that fails the entire write with a 400.
Suggestion: after converting children, if a container ends up with empty content, either drop the container itself (return nil from convertBlockNode, propagating upward) or insert an empty {"type":"paragraph"} placeholder (paragraph content is optional in the schema). Add tests for both the unsupported-only-child and past-the-cap cases.
| case *ast.String: | ||
| appendADFText(out, string(v.Value), marks) | ||
| case *ast.CodeSpan: | ||
| walkInline(v, source, withMark(marks, map[string]any{"type": "code"}), out, depth+1) |
There was a problem hiding this comment.
[MEDIUM] code mark combined with strong/em violates ADF's mark-combination rules
Marks accumulate through walkInline, so bold inline code like ** + backtick-x-backtick + ** produces a text node with "marks":[{"type":"strong"},{"type":"code"}] — confirmed empirically on this head. Atlassian's documentation states the code mark "can ONLY be combined with the following marks: [link]", so combining it with strong or em is schema-invalid and a comment containing bolded inline code (common in bot output, e.g. a bolded --flag) is rejected with a 400.
Suggestion: when entering a CodeSpan, filter the inherited marks down to just link before appending code, instead of appending code to whatever is present.
| for c := parent.FirstChild(); c != nil; c = c.NextSibling() { | ||
| switch v := c.(type) { | ||
| case *ast.Text: | ||
| appendADFText(out, string(v.Value(source)), marks) |
There was a problem hiding this comment.
[MEDIUM] Backslash escapes and HTML entities leak verbatim into posted comment text
The ast.Text case uses v.Value(source) — the raw segment bytes. Goldmark resolves backslash escapes and entity references in its renderer, not the parser, so this converter never resolves them: \*not em\* posts as the literal text \*not em\*, and ©/& stay unresolved (confirmed empirically; goldmark's own HTML output renders *not em* and the resolved entities). Agent-generated markdown escapes punctuation routinely (\_, \[, \*), so posted Jira comments will carry stray backslashes on nearly every realistic body — silently wrong output rather than an API error.
Suggestion: for non-raw ast.Text nodes, apply the same transformation goldmark's HTML writer applies (strip escape backslashes before punctuation, resolve numeric and named character references — see github.com/yuin/goldmark/util), keeping raw segments (code spans/blocks) untouched.
2 participants
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)