Skip to content

feat(jira): convert ADF rich text to markdown instead of plain text - #7543

Merged
bolichen97 merged 1 commit into
mainfrom
feat/adf-to-markdown
Sep 1, 2026
Merged

feat(jira): convert ADF rich text to markdown instead of plain text#7543
bolichen97 merged 1 commit into
mainfrom
feat/adf-to-markdown

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

Jira Cloud v3 returns an issue description and every comment body as ADF
(Atlassian Document Format), a JSON document tree. The walker that read it,
_adf_to_plain_text(), collected text leaf nodes only, so all structure was
gone before the field left the backend: headings became unstyled text,
bold/italic/code spans were stripped, links kept their anchor text and lost
the URL entirely, code blocks lost their fences, list items lost their
bullets, and tables lost every cell boundary.

Why it matters

IssuePanel.tsx renders source.description and each comment.body through
MarkdownRenderer, and both other providers already put real markdown in that
same payload key -- a GitHub issue body, a GitLab description. Jira Cloud was
the one source whose rich text arrived pre-flattened, so a Jira issue opened in
the panel was a wall of undifferentiated text while the same panel rendered a
GitHub issue properly.

One loss is worse than cosmetic. A link's URL was unrecoverable from the
panel, because the walker kept only the anchor text, so an issue whose
reproduction steps are a list of links arrived as a list of bare phrases.

Feeding plain text into a markdown renderer also had it both ways: text that
merely looked like markup was still re-parsed as markup, so a literal ** in
a Jira description turned bold and a literal <b> was dropped by the HTML
sanitizer -- while the real structure beside it was already gone.

What changed (motivation -> approach -> change)

Symptom: a Jira description renders as one undifferentiated block. Root
cause: the traversal was lossy by construction. It returned node["text"]
for a leaf and appended a newline after a known block type, and that was the
entire use it made of a node's type; marks were never read at all. No amount
of post-processing recovers formatting from that output, so the traversal
itself has to emit per node type.

_adf_to_markdown() replaces it and dispatches on node type:

  • Blocks (_adf_block_to_markdown): heading level to # repeated, code block
    to a fence carrying the language attribute, bullet and ordered lists to
    - / N. markers with a hanging indent so a nested list stays nested, an
    ADF task list to a GFM checklist, table to a GFM table, blockquote and panel
    to > , rule to ---, expand to its bold title plus its content. A chain of
    single-child quotes is collapsed and marked in one pass rather than re-marking
    at every level, which is quadratic in the nesting depth: a 6.7MB document
    nested 60 deep measured 2.57s of blocking work against 0.47s for the same
    content unnested, and 0.51s after the collapse, with byte-identical output. A
    heading
    collapses its internal newlines, since only its first line carries the # and
    a hardBreak would otherwise leave a second line whose leading - renders as a
    list.
  • Inline (_adf_inline_to_markdown): marks to **, */_, backticks and ~~.
    A mark shared by adjacent nodes is emitted ONCE around all of them, because
    wrapping each node on its own produced **a*****b****c* for strong, strong+em,
    em -- which a CommonMark parser reads as strong(a), a LITERAL ***b***, then
    em(c): the delimiters became visible text and the middle node lost both marks.
    Italic takes * normally, since underscore will not open emphasis intraword and
    a_b_c loses the italic, and _ where a * would touch another asterisk run.
    Where neither spelling is safe at both ends the mark is dropped and the text
    kept, which loses an italic instead of showing a delimiter as content;
    a link mark and an inlineCard to [text](url); a mention to @name; a
    hard break to a two-space line break, which is what the panel needs since it
    renders with CommonMark soft-break collapse. An external media node (one
    carrying a public url) becomes a plain link as well, not an image: that
    keeps the address recoverable without the panel auto-fetching a
    provider-controlled URL when someone opens the issue. A media node that
    only references an attachment by id has no fetchable address and contributes
    nothing, exactly as before.
  • A node type that is neither recurses into its children through the same span
    handling the top level uses, so an ADF type Atlassian adds later still
    contributes its text -- and gets the same mark merging and credential check its
    children would get at the top level.

_adf_to_markdown() is also the ONE guarded entry to the recursion: it holds
the 64-level depth cap, and every descent -- a container's children, a list
item's nested list, a table cell -- re-enters through it rather than calling a
renderer directly. Without that, a list nested a few hundred deep re-entered
its own renderer past the cap and exhausted the stack, turning one hostile or
machine-generated Jira description into an HTTP 500 on issue fetch.

Literal text is escaped on the way out (_md_escape_inline for the inline
syntax openers, _md_escape_block_leads for a line-leading -, +, #, 1. or a setext
underline of =).
That is what closes the second half of the bug on the ADF path -- and only
there: the old walker emitted raw provider text into a markdown renderer, so the
panel could not tell a description's real formatting from text that happened to
look like formatting. The Jira Server/DC v2 branches keep that misrender, so for
those issues this half of the bug stays open; the scope note below says why.
Block containers are classified as blocks. layoutSection / layoutColumn,
bodiedExtension and decisionList group other blocks, and reaching the inline
fallthrough concatenated them: a two-column layout of a paragraph and a heading
rendered as firstsecond, losing both the separator and the ##. blockCard
and embedCard carry their URL in an attribute with no content, so the same
fallthrough rendered them as the empty string and the URL was lost outright --
the same unrecoverable loss this change exists to fix. Markdown has no columns,
so a layout flattens to its children in document order.

A link destination is scanned before it is emitted, and a URL that fails is
dropped to plain text rather than linked. The old walker emitted no href at all,
so a provider-controlled destination reaching the panel is new here, and the
payload-level scan cannot be relied on to clean one up: _URL_RE's path group
excludes whitespace, ), ", ' and >, so ANY of those in the path truncates
the match and puts the entire query outside every check that follows. Measured: a high-entropy query blob is redacted
without the paren and not redacted with it, and no credential-pattern pass covers
a bare blob. Every site that emits provider text runs its redaction through one primitive,
_md_redact_untruncated -- the gap is per-CALL-SITE, not per-node-type, so when
only the link destination was covered an expand title, a mention label, an inline
card and a media URL each still leaked. The rule is to scan the form that will
actually be EMITTED, which is why whitespace differs per site: a link DESTINATION
is one address and the angle-bracket form emits a space as %20, so the scan
encodes it too, while in PROSE a space genuinely ends the URL (the renderer's
autolinker stops there) and encoding it treated a URL followed by a commit SHA as
one address, firing the entropy heuristic and replacing the whole paragraph. The
scan form is only a scan form:
used only to decide, never emitted -- and a URL with parentheses that passes, such
as a wiki or Confluence page, keeps its link in the angle-bracket form. The
truncation itself is in the shared scanner and affects its other callers, so it
is filed as #7611 rather than recorded only in a docstring here.

A table's header is widened to the widest row. GFM fixes the width at the header
and DROPS a longer row's excess -- the text is gone, not wrapped -- so a narrow
first row silently lost the later rows' cells. Only the header and separator grow,
which is linear in the width; padding every row is what made this quadratic
before.

An ordered list keeps its own start number, including an explicit order: 0,
which ADF allows and CommonMark honours as <ol start="0">. The start is bounded
so the LAST item's marker still fits in nine digits: ten digits is not a list
marker at all, so an out-of-range order would render the whole list as paragraphs
carrying visible numbers, and a start of 999999999 overflows on its second item.

The run gate reads a media node's ALT, not its URL. When a URL fails the
destination scan the link is dropped and the label is emitted with no brackets, so
the alt is what can join a neighbour's text into one credential -- measured as a
recoverable 32-character token before this, with the backslash from escaping
ghp_ then defeating the payload pass. Reading the alt errs toward MORE
contiguity than the output has when the brackets do survive, which is the safe way
to be wrong. An inline card keeps the URL, since its label IS the redacted URL.

A code body is deliberately NOT URL-scanned. Verified against the real renderer:
a fenced block and an inline code span both render with zero anchors and zero
images, so a URL in code is text rather than a fetchable address -- the same
reason whitespace ends a URL in prose. Running the entropy heuristic there would
replace legitimate code samples (an API example with a long opaque token reads
exactly like an exfiltration query) for no reachable gain, and credentials in code
are still caught by the token-shaped payload pass.

A code block's body keeps its own trailing newlines. The newline before the
closing fence separates the body from it, so emitting one unconditionally changed
the CONTENT: a source ending in one newline came back with two, and an empty body
became a block holding a blank line.

A code mark is exclusive -- a code span is literal by definition, so nothing
is escaped inside one, the fence widens past any backticks in the content, and
content that would lose a boundary space to CommonMark's own strip rule is
padded so it survives. ! is in the escape set for its own reason: this
converter emits a real [ for a link, an inline card and a media node, so a
literal ! landing immediately before one would splice into image syntax and
the panel would auto-fetch the URL -- the beacon the media-as-link form exists
to avoid. $ is there because the same renderer runs remark-math, so a literal
$$x$$ would render as KaTeX rather than as the characters someone typed. Both
were checked against the project's own parser rather than assumed.

Node ATTRIBUTES take a stricter path than text does, because they are labels
rather than prose and every one of them is provider-controlled. A newline
inside an attribute would end the construct the attribute sits in and let the
remainder become document structure, so _adf_attr_label collapses whitespace
before escaping (a mention's name, an expand's title, a media alt text, an
inline card's URL label). A code block's language is stricter still: a
fence's info string runs to the end of its line, so only a single highlighter
token is admitted (_MD_CODE_LANGUAGE_RE -- letters, digits and the
punctuation real names carry, as in c++ or shell_session) and anything else
drops to a bare fence, costing syntax highlighting and nothing else.

Escaping also forces a REDACTION contract, because _redact_provider_data runs
over the finished payload and matches contiguous secrets (ghp_...) that
markdown punctuation would break apart -- something the old unescaped, seamless
plain-text walk could not do. Redaction therefore happens exactly where this
converter inserts characters, and in both cases inside its own depth-capped
traversal:

  • _adf_inline_sequence checks each inline run ONCE, against the plain-text
    rendition a seamless walk would produce -- every node's own text in order with
    no markup between any of it, which is exactly the string the payload pass used
    to see. When it fires, the run is emitted as that redacted string: it loses its
    formatting, but no node's text is lost with it. Checking the whole run rather
    than some span of it is deliberate: any narrower boundary has to answer "which
    nodes contribute text seamlessly", and that answer kept turning out wider than
    the last one -- a bold sibling, an unrecognised container, then a mention or
    emoji label, each contributing text with no delimiter of its own. The run has
    no such boundary to get wrong. A nested container reuses its ancestor's scan
    rather than repeating it, since _adf_plain_text recurses and so the outer
    scan already covered every descendant: rescanning per level is depth-times-text
    work, measured at 7.0s for 1MiB under 60 unrecognised containers and 27.8s for
    4MiB, against 0.12s and 0.50s after.
  • _adf_attr_label redacts, collapses whitespace, then escapes, and owns every
    attribute label. That covers a credential in a free-form attribute (a media
    alt text, an expand title), which is not part of any inline run's text.

The same span handling fixes a display bug that has nothing to do with
redaction: adjacent text nodes carrying identical marks are merged before those
marks are applied. Wrapping each separately emits **a****b**, which CommonMark
renders as a bold a****b -- the delimiters become visible content -- and a code
mark is worse, since two adjacent code-marked nodes collapse into one span
holding literal backticks. Verified against a CommonMark parser, not assumed.
Each run's text is joined once rather than rebuilt per node: only traversal
DEPTH is capped, so a provider can put 300k adjacent text nodes inside the 8MiB
fetch cap, which measured 1.48s of blocking work against 0.23s after the change.

Everything else the converter emits verbatim -- a link destination, a code
block's body -- stays contiguous and is caught by the payload pass as before.

Deliberately NOT a pre-pass over the raw ADF tree: _redact_provider_data
recurses without a depth cap, so redacting a provider-controlled document before
converting it raises RecursionError a few hundred levels down (measured: fine
at 400, raising at 800 against the default limit of 1000) and turns a valid
issue fetch into a 500. Redacting inside the converter is bounded by the same 64
levels as everything else.

Per-line expansion is bounded by projection. Marking a quote and indenting a
list item both add characters to EVERY line, and a provider controls the line
count as well as the nesting depth that sets the per-line cost: newlines embedded
in a single text node cost about three bytes of payload each, while sixty levels
of nesting adds a hundred and twenty characters to each of them. The payload gate
runs only after conversion, so _md_guard_line_expansion projects the expanded
size first and refuses the document instead of allocating it -- measured before
the guard, a 2.3MiB payload rendered 93MiB of markdown with a 224MiB peak, and
the 8MiB fetch cap extrapolated to roughly 780MiB. The check lives inside both
expanders rather than at their call sites, so a new caller cannot forget it, and
it refuses the way an oversized response is already refused.

Unchanged, and this is the scope boundary of the fix: the
isinstance(raw_desc, str) branch carries Jira Server/DC v2, which returns wiki
markup rather than ADF, and it still passes that string to the renderer
untouched. So the "literal text re-parsed as markup" half is fixed for Cloud v3
only -- a literal ** in a Server description still turns bold there, and its
wiki markup still misrenders. Escaping that path without converting it would be
a regression of its own (visible backslashes where markup used to partly work),
and converting it means a second grammar with its own node vocabulary and tests.
Issue #2581 names the ADF path; the Server path is recorded for its own change.
Both fetch call sites keep their .strip().

_adf_to_plain_text is removed rather than left beside the new function: its
only two callers are the two converted here, so keeping it would leave an
unreachable helper and a test class pinning output nothing consumes.

Tests

test/test_source_providers.py: TestAdfToPlainText becomes
TestAdfToMarkdown, 102 tests. Beyond the four cases carried over from the old
class, they lock in:

  • The node-type mapping: heading and its level clamp, emphasis marks, link
    marks, inlineCard, fenced code with a language, bullet and ordered lists, an
    ordered list's explicit order start, nested-list indentation, task list,
    blockquote, panel, rule, GFM table, mention, and external media as a link
    (plus an id-only media contributing nothing).
  • Span handling: adjacent identical marks merge (emphasis and code), adjacent
    DIFFERENT marks do not, a hardBreak between two equally marked nodes keeps them
    apart, both hold one level down inside an unrecognised container, an italic
    between two plain neighbours keeps its emphasis, and a 100k-node run merges to
    the right string.
  • Nesting shape: a chain of three quotes emits three markers, so the one-pass
    collapse cannot lose a level; and a document whose projected expansion
    exceeds the payload ceiling is refused rather than rendered, checked both
    through the quote path and against the guard directly.
  • Table shape: a short row emits only its own cells, and a wide header with many
    narrow rows stays linear in cell count rather than padding to rows x width.
  • The escaping contract: literal **/<b>/_ survive as text, a literal
    &copy; is not decoded to a copyright sign, a literal $$x$$ is not rendered
    as math, a line-leading -/1. stays text, a === or --- line cannot
    promote the line above it into a heading, a pipe inside a table cell is
    escaped -- including one inside a code span, which is emitted literally and
    would otherwise split the cell -- a cell folds to one line by collapsing
    newlines only, so a code span's repeated spaces survive, a literal ! before a media node cannot
    splice the pair into an auto-fetching image, and a heading with a hardBreak
    stays on one line.
  • The escape set is DERIVED from the frontend renderer's plugin stack ($ for
    remark-math, ! for the image rehypeRaw admits, ~ for GFM strikethrough), so
    that coupling is pinned from both sides rather than described in a comment.
    test/fixtures/adf_markdown_safety.json is shared: the backend test asserts the
    converter turns each adf into exactly that markdown, and
    website/src/test/MarkdownRenderer.adfSafety.test.tsx renders the same markdown
    through the REAL plugin stack and asserts no <strong>, no <b>, no <img>, no
    KaTeX, and no recoverable credential. A second backend test pins the plugin list
    itself, so adding a plugin goes red with a message to re-derive the escapes.
  • Redaction, one test per shape a credential can take: whole in one text node,
    split across a plain and a bold sibling, split either side of an unknown inline
    container, split inside such a container, split deep inside nested containers
    (which is what the reused scan could have broken), split across a label
    boundary (an emoji's text), and sitting in a free-form attribute. Plus one
    recording why a media alt is NOT such a shape: the node emits [alt](url), so
    its label is always bracketed and cannot continue a credential from the text
    before it. Plus one asserting the
    fallback keeps every node's text, so a mention and a card in a redacted
    paragraph do not disappear.
  • The attribute contract: a newline-bearing language cannot close its own
    fence and inject markdown, a real token like c++ survives, a multi-token
    value drops to a bare fence, and an expand title and a mention name each stay
    on one line when the attribute carries newlines.
  • The code-span contract: a fence widening past inner backticks, boundary
    spaces padded so CommonMark's strip rule cannot eat them, whitespace-only and
    one-sided-space content deliberately NOT padded, and a marked empty text node
    emitting nothing rather than bare ****.
  • The depth cap, parametrized over every recursing container (blockquote,
    bulletList, orderedList, taskList, table, and an unrecognised inline
    container): content nested 350 deep is dropped and nothing raises, while
    content nested 3 deep survives. This is the test that pins the
    single-guarded-entry invariant -- it fails with RecursionError on the three
    list containers if a renderer is re-entered directly.
  • The edges: non-dict input, an unknown node type keeping its text, a link with
    parentheses switching to the angle-bracket destination form.

test_realistic_description_round_trips_to_markdown pins one whole document
-- heading, code mark, strong mark, a list containing a link, fenced code,
table, rule, mention -- against its exact expected markdown.

Mutation-verified, each reverted after: making _md_escape_inline return its
input fails exactly the two escaping tests; replacing the hanging-indent
padding with "" fails exactly the nested-list test; making
_md_code_language accept any value fails the fence-injection and multi-token
tests; dropping the whitespace collapse from _adf_attr_label fails the expand
and mention tests. The depth test was written first and observed to fail with
RecursionError on bulletList, orderedList and taskList before the
guarded-entry fix, and to pass after.

Targeted runs only (the full suite was not run locally; that is what CI is
for):

  • pytest test/test_source_providers.py -k AdfToMarkdown -q -n 4 -> 102 passed
  • pytest test/test_source_providers.py -k "jira or Jira" -q -n 4 -> 46 passed
  • flake8 and isort clean on both files;
    scripts/check_black_formatting.py passes; mypy src/kiro_crew/ reports
    nothing for this file.

Manual verification

N/A -- reaching this path needs a live Jira Cloud v3 instance and credentials.
The converter is a pure function of the ADF payload, and the whole-document
test above asserts the exact string the panel would be handed, so the unit
coverage is the same evidence a manual pass would produce.

Related Issues

Closes #2581

Pattern harvest

Rule candidate: semgrep

Pattern: a recursive walker whose depth cap lives in ONE entry function, with a
sibling in the same family calling an inner renderer directly and re-entering
the cycle past the cap. Here _adf_to_markdown held the guard while the
list-item helper called _adf_block_to_markdown straight, so three of the five
nesting containers exhausted the stack while the two that happened to route
through the entry were fine. The greppable shape is a mutually recursive
function group where the guard predicate appears in a strict subset of its
members; a checker can flag a call that closes a cycle without passing through
a guarded member. The test form generalizes too, and is what this PR ships:
parametrize the depth test over EVERY container that can nest, so a container
added later cannot quietly bypass the cap.

Second pattern, confirmed rather than hypothetical: a provider-controlled
string interpolated into a LINE-ORIENTED markdown construct -- a fence's info
string, a heading, an emphasis run -- without collapsing newlines first. The
construct ends at the newline, so the remainder of the attribute becomes
document structure. This diff had it in six places (a code block's language
plus five attribute labels) and closes all six through two chokepoints,
_adf_attr_label and _md_code_language. The greppable shape is an f-string
that places an externally-sourced value on the same line as markdown
punctuation without a single-line guarantee; the durable fix is having exactly
one escaper per sink shape and no direct interpolation at the call sites, which
is what a checker should assert.

Third pattern, the deepest one this change surfaced: a transform that INSERTS
characters into text which a downstream security scanner must still match. Both
credential findings here are that shape -- escaping put a backslash inside
ghp_, and marks put ** between a secret's halves, in each case defeating a
redactor that ran afterwards and had matched fine before. The rule generalizes
past markdown to any encoder, escaper or formatter placed upstream of a
pattern-matching gate: when you add one, the gate either has to move upstream of
it or be told what the transform can break. A checker cannot see that coupling,
but a reviewer can be told to look for it whenever a diff adds an escaping step
to a value that already flows through a scanner.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) -- no doc references the ADF walker
  • No secrets, credentials, or internal references in the diff

@chenmingwei23
chenmingwei23 requested a review from a team as a code owner September 1, 2026 07:03
@chenmingwei23
chenmingwei23 requested a review from cixuuz September 1, 2026 07:03
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of b231f25e29e428fe68fecf0bf04565743ed89b12 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Root-cause fix at the right layer: markdown into the same payload contract the other providers already use, with the new untrusted surfaces (hrefs, escaping-vs-redaction, expansion) each bounded.

Suggestions

[DESIGN-REVIEWED] b231f25

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of b231f25e29e428fe68fecf0bf04565743ed89b12 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified: the shared scanner _URL_RE at security.py:9263 does carry the [^\s)\"'>]* path class the shadow table mirrors (2 scan call sites, security.py:10017 and 10041, deferred as #7611); no other ADF mechanism exists in the repo; IssuePanel.tsx does render source.description and comment.body through MarkdownRenderer. Final review follows.

First-Principles-Verdict: PASS

Every item traces to a named, mostly measured defect — lost URLs, parser-verified mark corruption, counted exfil leaks — and the two deferred halves are declared with their scope.

What this change ships

Intent: a Jira Cloud issue opened in the panel renders with its real formatting instead of a flattened wall of text — a FIX.

  1. Jira Cloud descriptions and comments show headings, lists, tables, quotes, checklists, code — justified
  2. Link, card and external-media URLs are recoverable as links — justified (the declared worst loss)
  3. Literal **, <b>, $$ in Jira text renders as typed, not as markup — justified
  4. External media becomes a link, never an auto-fetched image — justified (external-content boundary)
  5. URLs and text failing the exfil/credential scan are dropped or redacted in the backend — justified; symptom-level shadow of the shared scanner bug, declared and filed (Exfiltration URL scan stops at a ')' in the path, leaving the query unscanned #7611)
  6. A pathologically large or nested document returns an error instead of rendering — justified (measured 2.3MiB→224MiB amplification)
  7. Jira Server/DC v2 issues keep the old flat text — declared scope note, siblings counted below
  8. Shared backend/frontend safety fixture — test-only, rides along harmlessly

Watch

[FIRST-PRINCIPLES-REVIEWED] b231f25

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of b231f25e29e428fe68fecf0bf04565743ed89b12 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] b231f25

False positive or not applicable? A repository writer can comment:
/ai-review override gpt b231f25e29e428fe68fecf0bf04565743ed89b12: <one-sentence reason>

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed b231f25e29e428fe68fecf0bf04565743ed89b12 — this comment is updated in place on each push.

Review details

The single candidate describes _md_one_line collapsing a newline inside an inline code span (`a\nb``a b`) within a heading or table cell. Verifying against the code: a heading and a GFM table cell each occupy exactly one physical line by definition, so folding an embedded newline onto that line is forced — keeping the \n would produce the broken two-line output the heading branch's own comment exists to prevent. The transformation is strictly structure-reducing (newline → space), so it cannot create markup injection, a crash, or exfiltration; it is at most a best-effort fidelity loss the converter's docstring already scopes to. The candidate's own confidence is "low," and the observable outcome is not a defect but a necessary lossy one-line rendering. It does not clear the 80+ bar. No other grounded defect surfaced in this heavily-guarded, extensively-tested change.

No findings.

[OPUS-REVIEWED] b231f25

Verdict parsed from the review's SHA-scoped output markers for commit b231f25e29e428fe68fecf0bf04565743ed89b12.

False positive or not applicable? A repository writer can comment:
/ai-review override fable b231f25e29e428fe68fecf0bf04565743ed89b12: <one-sentence reason>

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Dispositions for round 1 (verdicts on f0f4588c5), all addressed in d6b9a19b6.

GPT BLOCKING 1 -- nested lists bypass the depth limit: FIXED, and it was worse than a theoretical risk.

Confirmed before touching the code. The cap lives in _adf_to_markdown; _adf_item_body called _adf_block_to_markdown directly, so a nested list re-entered its own renderer past the guard. I wrote the test first and it reproduced exactly: at 350 levels, bulletList, orderedList and taskList all raise RecursionError, while blockquote and table pass -- precisely the split between the containers that route through _adf_item_body and the ones that go through the guarded entry.

The fix is the dispatch you prescribed, and _adf_block_to_markdown now has exactly one call site: line 3159, immediately behind the guard at 3156. Three functions carry the depth predicate (_adf_to_markdown, _adf_inline_to_markdown, _adf_raw_text) and every cycle in the family now passes through one of them.

The test ships parametrized over all five recursing containers rather than pinned to the one that broke, so a container added later cannot quietly bypass the cap either. That generalization is what the Pattern harvest section now names as the rule candidate.

GPT BLOCKING 2 -- code spans lose boundary spaces: FIXED.

Correct: CommonMark strips one character from each end of a span whose content both begins and ends with a space or newline. _md_inline_code now pads that case, and three tests pin the boundaries of the rule -- foo is padded to ` foo `, whitespace-only content is deliberately NOT padded (the strip rule exempts it, so padding would add two spaces of its own), and one-sided space is not padded either (the rule needs both ends).

First Principles subtraction -- shrink media from an image to a link: ACCEPTED, implemented.

All three of your counts were right: undeclared in the description, zero tests, and it added a network side effect the fix does not need. An external media node now emits [alt](url), so the address stays recoverable -- the harm this PR names for links -- without the panel auto-fetching a provider-controlled URL when someone opens the issue. It is now declared in the body's inline list and covered by two tests (external media with a url, and an id-only attachment reference contributing nothing).

First Principles watch -- the Server/DC v2 branch still feeds wiki markup unescaped: ACCEPTED AND DEFERRED.

Agreed, and it is the one unescaped path left. Deferring rather than folding it in: wiki markup is a different grammar, so handling it honestly means a second converter (its own node vocabulary, its own escaping, its own tests), not a line in this diff. Escaping that text without converting it would also be a regression of its own -- Server descriptions would render with visible backslashes where markup used to at least partly work. Issue #2581 names the ADF path, and that is what this PR closes. Recorded locally so it gets re-judged on its own merits rather than riding along here.

Self-found in the same span, declared for completeness: a marked empty text node emitted its bare delimiters (**** for strong, `` for code), which render as those literal characters. _adf_apply_marks now short-circuits empty text, with a test across all four marks.

Design Review PASS: noted, no action.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 2 disposition (verdict on d6b9a19b6), addressed in 510e9a44f.

GPT BLOCKING -- unsanitized code language enables markdown injection: FIXED, and the class was wider than the one instance.

Real, reachable, and self-introduced by this diff -- the old walker emitted no fence at all, so this arrived with the fix. A fence's info string runs to the end of its line, so a language attribute carrying a newline closes the fence early and everything after it becomes real document structure, including an image that loads on its own when the panel opens.

Auditing the rest of my own emitters for the same shape found five more: expand.title, a mention's name, an emoji's text, an inline card's URL label, and a media alt text all went through _md_escape_inline, which escapes markdown punctuation but passes newlines through. Same mechanism, same provider-controlled source, different construct.

So the fix is two chokepoints rather than one patch:

  • _md_escape_label collapses whitespace before escaping, and now owns all five attribute labels. An attribute is a label, not prose -- a text node's newline is legitimate content, an attribute's newline never is.
  • _md_code_language admits one highlighter token and nothing else (^[A-Za-z0-9+#._-]{1,32}$), so c++ and shell_session survive while anything that could leave the line drops to a bare fence. The cost of the fallback is syntax highlighting, nothing more.

There are now no direct _md_escape_inline(str(attrs...)) call sites left; grep confirms zero.

Five tests pin it: the newline-bearing language cannot inject (asserting both the exact output and that the injected host does not appear), c++ survives, a multi-token value drops to a bare fence, and an expand title and a mention name each stay on one line. Both guards are mutation-verified -- making _md_code_language accept any value fails the injection and multi-token tests; dropping the whitespace collapse from _md_escape_label fails the expand and mention tests.

The PR body's escaping section now describes the attribute path explicitly instead of implying text escaping covered everything, and the Pattern harvest promotes this from the weak review-prompt it was to a confirmed rule candidate: an externally-sourced string interpolated onto the same line as markdown punctuation without a single-line guarantee, with the durable form being one escaper per sink shape and no direct interpolation at call sites.

45 tests pass; flake8, isort and the black baseline gate are clean.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 3 disposition (verdicts on 510e9a44f), addressed in 64a728dd1.

GPT BLOCKING -- markdown conversion bypasses credential redaction: FIXED, and I could reproduce the leak.

The chain checks out structurally and empirically. _redact_provider_data runs over the finished payload, after _fetch_jira_issue has already converted the ADF, and the credential pattern it matches is gh[opsur]_[A-Za-z0-9]{30,255} -- contiguous. My escaping puts a backslash in the middle of that prefix, so ghp\_... no longer matches and the secret rides out through the API. The old unescaped walker could not do this, so it is a regression this diff introduced.

Both call sites now redact the raw ADF tree before converting, which is what you prescribed: each text node is still contiguous while it is being matched, and escaping happens afterwards on text that no longer holds a secret.

The test asserts the asymmetry in both directions rather than just the fixed path -- a ghp_-shaped secret SURVIVES convert-then-redact and does NOT survive redact-then-convert. That first assertion is the reproduction, and it means the test fails if the call sites ever revert to the wrong order, and also tells us if escaping ever stops interfering.

GPT FINDING -- literal &copy; decoded by MarkdownRenderer: FIXED.

Correct, and it is the same class as the <b> case already covered: rehypeRaw decodes HTML entities, so a literal ampersand sequence in a Jira description silently became a symbol. & is now in _MD_INLINE_ESCAPE with the rationale recorded in the comment above it, and a test pins &copy; 2026 &amp; friends staying literal.

Backend Tests (3.10, 3): MAIN-OWNED, not this PR.

Failing test is test/test_security_posture.py::TestGateSideLogRedactorSpelling::test_no_new_gate_side_log_line_reads_the_baseline_redactor, asserting on dashboard/handlers/memory.py: 2 sites, census says 0. This diff touches two files (dashboard/handlers/source_providers.py, test/test_source_providers.py) and zero lines of memory.py.

Evidence it is pre-existing: main's own CI run 33480742836 on head 5603ae744 fails the identical assertion with the identical message, along with test_session_storage.py::TestSidecarFiles on another shard. The fix is already in flight as #7554 (fix/census-memory-py-7549), whose diff touches exactly memory.py. Not folding it in -- I will rebase once it lands, which is also what cuts a fresh merge ref for the remaining shards.

Design Review PASS and First Principles PASS on 510e9a44f: noted. First Principles' remaining Watch is the v2 wiki-markup branch, which it records as accepted-and-deferred on the same reasoning I gave in round 1; that stands, and it is tracked locally rather than as an issue.

47 tests pass; flake8, isort and the black baseline gate are clean.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 11 disposition (verdicts on 163ed70f1), addressed in d6d135d4d.

GPT BLOCKING -- table flattening corrupts code-span whitespace: FIXED, and I had already reasoned my way to the right answer once and then not applied it here.

Real, and ordinary content again: a Jira table cell containing the code a b reached the API and the panel as a b, because _adf_cell_text collapsed every whitespace run to a single space. A code span's whitespace is literal, so that is a silent rewrite of content.

What makes this one worth naming: in round 6 I hit the same question for HEADINGS and chose to collapse newlines only, specifically to protect a code span's spacing and the boundary-space padding added in round 2. I noted the cell had the same exposure and left it, judging the loss marginal. That judgment was wrong -- repeated spaces in code are exactly what a table of shell or regex snippets carries -- and it was also inconsistent with the sibling I had just written.

Both now share one rule: _md_one_line folds a heading and a cell onto one line by collapsing line breaks only. Repeated spaces and tabs survive; a code span's padding is safe because it sits inside the backticks where no newline can reach it. Net effect is one spelling of the rule instead of two, and one fewer lossy path. A test pins `a b` surviving in a cell.

First Principles CONCERNS (advisory) -- the fix is Cloud-only and the description implied otherwise: ACCEPTED, description sharpened.

Fair, and the imprecision was mine. The body said "every other provider already puts real markdown in that same payload key", which is true of GitHub and GitLab but let the reader infer the field is uniformly markdown after this change. It is not: the isinstance(raw_desc, str) branch carries Server/DC v2 wiki markup and still passes it to the renderer untouched, so a literal ** in a Server description still turns bold there.

The body now states that as the scope boundary rather than as a passing "unchanged" note, including why the two obvious shortcuts are both wrong -- escaping that path without converting it would show visible backslashes where wiki markup used to partly work, and converting it means a second grammar with its own vocabulary and tests. #2581 names the ADF path; the Server path is recorded locally for its own change. I also narrowed "every other provider" to "both other providers", which is what the code supports.

65 tests pass; flake8, isort and the black baseline gate are clean. Design Review PASS and Opus clean on 163ed70f1, noted with no action. Backend Tests are now running against the rebased base carrying #7572, so the shard red that was main-owned should be gone -- I will confirm rather than assume.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 12 disposition (verdict on d6d135d4d), addressed in 507adb9b5. The mechanism is real and fixed; the severity as stated is not, and I measured rather than argued either way.

GPT BLOCKING -- nested blocks amplify bounded ADF into an unbounded response: mechanism FIXED, characterisation corrected.

The mechanism is real and it is mine: _md_prefix_lines re-marks text the level below has already marked, so a chain of nested quotes is quadratic in nesting depth over the number of lines it carries. Main's walk did no marking at all, so the multiplier arrived with this diff.

Measured on this branch, same content in both shapes:

shape payload output ratio time
flat, 100k paragraphs 6.70 MB 0.30 MB 0.04x 0.47s
nested 60 deep, same 100k 6.70 MB 24.20 MB 3.61x 2.57s
nested 60 deep, after this fix 6.70 MB 24.20 MB 3.61x 0.51s

Two corrections to the finding. It is not unbounded: _fetch_jira_issue streams the response through read_capped_response(resp, _MAX_PAYLOAD_BYTES) and rejects anything over 8 MB BEFORE decoding, so the input is capped and the growth ratio is a constant bounded by depth times two characters per line. And it cannot produce an oversized response: _payload_size_bytes still gates the converted payload afterwards, so the 24 MB rendering is transient and rejected, not served. The real cost was ~2s of blocking event-loop work on a maximal crafted document -- a stall worth removing, not a terminated gateway.

The fix removes the quadratic term rather than adding a projected-size check, which is what I would have had to bolt into a formatting helper otherwise. A chain of single-child quotes is now collapsed and marked once; each collapsed level still consumes traversal depth, so the 64-level cap applies exactly as before. Output is byte-identical (24,199,879 characters in both runs) -- only the redundant copying is gone, and nested content now converts in the same time as flat content.

A test pins the behaviour the optimisation could break: three nested quotes must still emit three markers, so a collapsed level cannot go missing.

Also in this push, from the previous round's numbers: nothing. This round is one change.

66 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. Design Review PASS, First Principles PASS (upgraded after the scope-boundary wording), Opus clean -- all on d6d135d4d.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 13 disposition (verdict on 507adb9b5), addressed in 850eee800.

GPT BLOCKING -- prefix expansion bypasses the payload memory ceiling: FIXED. GPT was right and my round-12 rebuttal was measured on the wrong shape.

Last round I said the growth ratio was "a constant bounded by depth times two characters per line" and put it at 3.61x, so the finding looked like a bounded stall rather than an OOM. That number came from a document built as one paragraph per line, which costs about 58 payload bytes per line. It is not the worst shape.

The worst shape puts the newlines INSIDE a single text node, where each costs about three bytes of payload. Same 60 levels of nesting, so the same 120 characters added per line, against 3 bytes instead of 58 -- a 19x denser attack than the one I measured. Re-measured with tracemalloc:

lines payload output peak time
200,000 588 KiB 23 MiB 56 MiB 0.18s
800,000 2.3 MiB 93 MiB 224 MiB 0.75s

The ratio is 40x, not 3.61x, and it is linear, so the 8 MiB fetch cap extrapolates to roughly 325 MiB of output at about a 780 MiB peak. That is the "hundreds of MiB" the finding claimed, and the post-conversion payload gate cannot help because the allocation is what it was supposed to prevent. So my earlier characterisation was wrong on the number that mattered, and the correction is on the record here as well as in the description.

_md_guard_line_expansion now projects the expanded size -- len(text) + per_line * (lines + 1), one count("\n") -- and refuses over _MAX_PAYLOAD_BYTES with a SourceProviderError, matching how an oversized response is already refused. It sits INSIDE the expanders rather than at their call sites, so a new caller cannot forget it, which is the lesson from the depth-guard round earlier in this review.

I also guarded the sibling the finding did not name. _md_hang_indent expands every line of a list item's body by the marker width and compounds through nested lists, which is the same mechanism with a smaller constant. Guarding only the quote path would have left it for a later round.

Two tests: the guard directly (the same text refused at a 120-character expansion, accepted at 2), and end to end through the quote path, where a 100,000-newline text node inside 60 quotes now raises instead of rendering.

On this being round 13. The measurement failure is mine twice over -- round 12 I under-measured, and round 5's ragged-table finding was the same class of amplification, so I had already seen this shape once. What I take from it: for an amplification claim, the input shape has to be chosen adversarially for DENSITY, not just built to be large, and peak allocation has to be measured rather than inferred from output length.

68 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. Design Review PASS, First Principles PASS, Opus clean on 507adb9b5.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 14 disposition (verdict on 850eee800), addressed in bb772dfcc.

All four lanes were clean on 850eee800 -- GPT no blocking findings, Opus no blocking findings, Design PASS, First Principles PASS. GPT left one advisory finding, and I fixed it rather than dispositioning it as accepted.

GPT FINDING (advisory) -- the block-lead escaper misses =: FIXED.

Real, and it is a gap in a rule this diff owns. _MD_BLOCK_LEAD_RE escaped a line-leading -, +, # and 1., which covers a setext H2 underline (---) as a side effect of the list-marker case, but nothing covered a setext H1 underline (===). So Jira text of Title then === promoted the line above it into a heading, while Title then --- was already literal. That inconsistency had no justification -- it was an accident of which characters the list rule happened to include.

= is now in the class, with the comment above it naming the setext case explicitly so the next reader does not have to infer why an underline character sits in a list-marker rule. A test pins both underline forms staying literal.

Why I pushed for an advisory rather than answering in thread. The usual reason not to is that a push re-rolls every lane on a converged PR for no correctness gain. Two things made it the cheaper call here: the PR was not green yet -- seventeen checks including all twelve Backend Tests shards were still in flight, so they had to run again regardless -- and the defect is self-introduced and one character wide. Declaring green while knowingly leaving a real gap in my own escaping rule would also have made the "every concern dispositioned" claim weaker than it reads.

69 tests pass; flake8, isort and the black baseline gate are clean.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 15 disposition, addressed in beeebd826. Both findings real, both verified before fixing.

GPT BLOCKING (on bb772dfcc) -- intraword italic renders as literal underscores: FIXED.

Checked against a CommonMark parser rather than taken on the description: a_b_c renders as the literal text a_b_c -- underscores visible, italic gone -- because CommonMark refuses to open or close underscore emphasis intraword. a*b*c gives a<em>b</em>c. So an ADF paragraph of plain + italic + plain lost its formatting entirely, which is the reverse of what this PR is for.

Before taking the asterisk I checked it does not trade the bug for the round-6 adjacency problem, since strong already uses **: **a***b* renders as <strong>a</strong><em>b</em> and *a***b** as <em>a</em><strong>b</strong>, both correct, and ***x*** still nests as em-wrapping-strong. Two existing assertions moved from _italic_ to *italic* and from **a**_b_ to **a***b*, and a new test pins an italic between two plain neighbours keeping its emphasis.

Opus BLOCKING (on 850eee800) -- quadratic text concatenation in the merge: FIXED, with the severity corrected.

Note the head: Opus reviewed 850eee800, one behind, so its second finding (the unescaped setext H1) was already fixed in bb772dfcc before it reported. Its blocking finding is against code that change did not touch, so it still applied.

The mechanism is real and mine, introduced with the round-6 mark merging. _adf_merge_marked_text rebuilt the accumulator as previous + current for every node in a run, and Opus is right that only traversal DEPTH is capped -- nothing bounds a node's breadth, so 300k adjacent single-character text nodes fit inside the 8MiB fetch cap. Main's walk joined once per level and was linear.

Measured rather than assumed, since the claim was "tens of seconds":

adjacent nodes before after
100,000 0.26s 0.08s
200,000 0.75s 0.16s
300,000 1.48s 0.23s

So it was superlinear and worth removing, but 1.48s at the reachable bound rather than tens of seconds -- the same overstatement pattern as the memory finding two rounds ago, where the direction was right and the magnitude was not. Each run's text now goes into a list and is joined once, which is linear, and the mark signature is computed once per node instead of twice. A test pins a 100k-node run merging to the right string.

One process failure of mine in this round, recorded. I chained the amend and the black gate with && but put the push on its own line, so when the gate failed the push ran anyway and 3370a6066 went up unformatted. I caught it on the next command, formatted (black touched only the one comprehension it had asked about), re-verified the tests and re-pushed as beeebd826. The gate would have caught it in CI regardless, but the ordering was mine to get right.

71 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. Design Review PASS and First Principles PASS still stand.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 16 disposition (verdicts on beeebd826), addressed in bd9c2e927. One finding fixed, one rebutted with evidence, one advisory re-dispositioned.

GPT BLOCKING -- nested unknown containers repeatedly scan the full text: FIXED. The numbers match the finding.

Real, and it is the composition of two of my own earlier changes: round 8 routed the unrecognised-container fallthrough through _adf_inline_sequence, and round 9 made that function scan the whole run's plain text. Together, N nested containers each re-derive and re-scan their entire subtree -- depth times text.

Measured before and after:

document before after
1MiB under 60 unrecognised containers 7.01s 0.12s
4MiB under 60 unrecognised containers 27.78s 0.50s

So "over 25 seconds" was accurate. A nested container now reuses its ancestor's scan via a _scanned flag rather than repeating it. That is sound rather than merely cheaper: _adf_plain_text recurses, so the outer scan already covered every descendant's text, which makes the inner scans provably redundant.

The risk in that optimisation is a credential deep inside nested containers no longer being caught, so a test pins exactly that -- a split credential three containers down is still redacted.

GPT BLOCKING -- media alt text can bypass credential redaction: REBUTTED, with the output.

Not reachable. A media node emits [alt](url), so its label is always bracketed: the actual output for a credential split across text and an alt is

ghp\_Ab3Df6Hj9Kl2Np5Qr8Tv1Wx4[Yz7Bc0Ef3Gh6](https://ex.com/a.png)

The [ sits between the halves, so they do not form a contiguous credential -- checked after stripping backslashes, which is the generous reading, and the concatenation is absent. The panel shows the first half as text and the second as a link label, not one token.

The finding's premise -- that _adf_plain_text returns the URL rather than the alt for a media node -- is accurate, and that asymmetry is deliberate: the gate must never see MORE contiguity than the output has, and substituting the URL for a bracketed label errs in the safe direction. A credential in the URL is caught anyway, since the destination is emitted verbatim and stays contiguous for the payload pass. A test now records the bracketing so a future change to the media emission cannot quietly make this reachable.

First Principles CONCERNS (advisory) -- the Server v2 branch keeps the misrender: ACCEPTED AND DEFERRED, unchanged from round 11.

Same disposition, and I agree with the framing that a human should know the boundary: the description states it explicitly, naming both sibling sites and why neither shortcut works (escaping without converting shows visible backslashes where wiki markup used to partly work; converting means a second grammar with its own vocabulary and tests). It is recorded locally rather than as an issue. Nothing new to add, but not left silent.

73 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. Design Review PASS and Opus clean on this head.

Process note, since I reported the same mistake last round. The black gate failed on my first amend again, for the same reason: check_black_formatting.py | tail always exits 0, so an && chain reads it as a pass. Last round that let an unformatted commit reach the branch. This round I checked the script's own exit status instead of the pipeline's, caught it before pushing, and only pushed after the gate genuinely passed.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 17 disposition (verdicts on bd9c2e927), addressed in 93cb317ab.

GPT BLOCKING -- link destinations bypass exfiltration redaction: FIXED, and confirmed with a payload. My first attempt at the fix was inadequate; the second is the one that works.

Real, and worse than the one-line summary suggests. The chain, verified at each step rather than reasoned about:

_URL_RE's path group is ([/?][^\s)\"'>]*)? -- it excludes ). So for an href with a ) in the path, the match ends there, and _exfil_url_warning receives a truncated path_and_query with no ? left in it. It returns clean at if qmark == -1. The entire query is outside every check: the hard-credential pass, the fixed-signature pass, the decode loop, and the generic entropy heuristic.

Measured with one blob in a query, ?data=Xk7Qm2...:

href redacted?
https://evil.example.com/a?data=<blob> yes
https://evil.example.com/a)b?data=<blob> no

And there is no backstop for this shape. redact_credentials is token-shaped, so it catches a ghp_... anywhere including past the paren -- but a bare high-entropy blob matches no token pattern, and the entropy heuristic that would have caught it is exactly what the truncation skips.

This is new surface from this PR: the old plain-text walker emitted no href at all, so no provider destination reached the payload. The truncation lives in main's shared _URL_RE, but my change is what feeds hrefs to it, so the fix belongs here and not in that primitive.

The correction worth recording: my first fix scanned the raw href and dropped the link if redaction changed it. That does not work -- it calls the same truncating scanner, so the attack URL came back unchanged and the gate passed it through. I caught this only because I checked the rendered output instead of trusting the fix, and the render still showed the full blob. The working version scans a form with the parentheses percent-encoded, which is the character the truncation turns on; the encoded form is used only to DECIDE and is never emitted.

_md_link_target now returns None for a URL that fails, and all three emission sites (link mark, inlineCard, media) drop to the label alone. The label still carries the text, so nothing silently vanishes -- only the destination goes.

Two tests: the attack renders as bare click with neither the blob nor the host present, and a benign parenthesised URL keeps its link. That second one matters as much as the first -- over-blocking would silently strip legitimate Jira links, and wiki and Confluence URLs carry parentheses routinely. Both https://en.wikipedia.org/wiki/Salt_(chemistry) and a Confluence page with parentheses AND a query keep their links in the angle-bracket form. Encoding two characters cannot trip the heavy-encoding rule, which needs 20 consecutive octets.

First Principles CONCERNS (advisory) -- the description overclaims what escaping closes: FIXED in the same push.

Correct, and it was my wording, not the code. The body said escaping "closes the second half of the bug" without qualification, which is true only for ADF. It now says so explicitly and states that the Jira Server/DC v2 branches keep the misrender, alongside the existing scope note. The deferral itself is unchanged and still recorded locally.

One local red, environment-owned, not folded in. test_provider_executable_accepts_symlinked_install fails in my sandbox with executable parent is owned by another user (uid 65534) -- uid 65534 is nobody, a property of this machine's temp directory, not of the code. My diff has zero hits for provider executable resolution, and that test runs inside the Backend Tests shards that have been green on this PR throughout. Left alone.

75 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. Design Review PASS and Opus clean on the previous head.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 18 disposition (verdicts on 93cb317ab), addressed in 44c6eb30c.

GPT BLOCKING -- ADF layout containers concatenate block content: FIXED, and the class is wider than the two types reported.

Real. layoutSection / layoutColumn group BLOCK content, they were not in _ADF_BLOCK_TYPES, so they fell to the inline container fallthrough and their children were rendered as inline. Rather than add the two names, I checked every ADF container type the dispatch does not classify. Measured before:

type before after
layoutSection (para + heading) 'firstsecond' 'first\n\n## second'
bodiedExtension 'firstsecond' 'first\n\n## second'
decisionList (two items) 'alphabeta' 'alpha\n\nbeta'
blockCard '' '[url](url)'
embedCard '' '[url](url)'
mediaSingle already correct unchanged

So GPT found 1 of 5. Two of the others are worse than concatenation: blockCard and embedCard carry their URL in an attribute with no content, so the fallthrough rendered them as the empty string and the URL was lost outright -- the same unrecoverable loss this PR exists to fix, on a node type the PR itself introduced a path for.

Markdown has no columns, so a layout flattens to its children in document order; the fix is that they flatten as BLOCKS. The new containers recurse through _adf_join_blocks, which calls the guarded entry, so the depth cap covers them -- and the parametrized depth test now includes layoutSection, bodiedExtension and decisionList so that stays pinned.

Design Review CONCERNS 1 -- the ) bypass is shielded locally but stays open everywhere else: FILED as #7611.

Agreed, and this is the more important of the two. I did not fix it here on purpose: _URL_RE is a shared security primitive and changing it is a different blast radius than a Jira rendering change, so folding it in would be exactly the unrelated fix this process forbids. But "recorded only in a docstring" was a fair criticism, so the measured finding is now #7611 with the reproduction, the affected call sites, and a suggested direction -- separating the permissive pattern used for CLASSIFICATION from the strict one used for EMISSION, since excluding ) is a markdown-destination concern and not a property of URLs. The docstring points at the issue.

Design Review CONCERNS 2 -- the two-layer redaction invariant is conventional, not enforceable: FIXED.

Also fair, and the suggested structural test was the right shape. There are now exactly two chokepoints -- _adf_attr_label for attribute text and a new _adf_url_link for a URL destination -- and the four URL emission sites (inlineCard, media, blockCard, embedCard) all route through the latter, which was worth doing anyway since the two new card types would otherwise have been a fifth and sixth hand-rolled copy.

The test reads the node types and attribute names back OUT of the module source and tries every combination with a credential in every attribute, so a node type or attribute added later is covered without anyone remembering the test exists. I mutation-checked it rather than trusting it: replacing one _adf_attr_label call with the raw value makes it fail and name ['blockCard', 'embedCard', 'inlineCard'].

82 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. First Principles is PASS on the previous head after the wording fix.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 19 disposition (verdicts on 44c6eb30c), addressed in aadfc8984. Design Review, First Principles and Opus are all clean on that head; GPT was the only blocker.

GPT BLOCKING -- overlapping adjacent marks emit literal delimiters: FIXED.

Real, and the worst-looking output in this PR so far. Independent per-node wrapping emitted **a*****b****c* for strong / strong+em / em, which a CommonMark parser reads as:

<strong>a</strong>***b***<em>c</em>

The middle node loses BOTH marks and six asterisks appear as visible text. Two-node cases were fine, so it takes a mark shared and then dropped across three nodes -- bold, bold-italic, italic, which is an ordinary thing to write in Jira.

GPT's prescribed direction was right and I took it: a mark shared by adjacent nodes is now emitted ONCE around all of them, greedily taking the mark that spans the longest run. Worth recording why that alone is not sufficient: with * for italic, ANY boundary between two asterisk runs is ambiguous, so factoring still emitted **a*b****c*. The fix needs a second delimiter. Italic therefore takes _ exactly where a * would touch another asterisk run -- a position where _ is always safe, because the neighbour is an asterisk and so not intraword. Every expectation was checked through a parser:

shape emitted parses as
strong, strong+em, em **a*b***_c_ <strong>a<em>b</em></strong><em>c</em>
em, em+strong, strong _a**b**_**c** <em>a<strong>b</strong></em><strong>c</strong>
strong, em, strong **a**_b_**c** <strong>a</strong><em>b</em><strong>c</strong>
plain, em, plain a*b*c a<em>b</em>c (round 15 still holds)

One case has no correct encoding: an italic that both abuts an asterisk run AND is followed by a word needs * at one end and _ at the other. There the mark is dropped and the text kept -- **a*b***cd -- which loses an italic where emitting a delimiter anyway would show it as content and lose the italic too. Code, strike and link marks are untouched: they are the marks whose delimiters do not share a character with *, so a neighbour's backtick, tilde or bracket already separates the runs. Verified per pair rather than assumed.

I introduced a quadratic in the first version of this fix and caught it before pushing. The emitter needs the character before each mark to choose a delimiter, and I passed the accumulated output to get it -- rebuilding the whole prefix at every position. Measured 0.87s / 3.42s / 13.85s for 25k / 50k / 100k nodes, a clean 4x per doubling. It only ever reads one character, so it now carries one character: 0.16s / 0.30s / 0.72s, and the output is byte-identical. I measured only because this PR has already had two superlinear findings; the reflex was worth having.

Recursion in the new emitter is bounded by the number of factorable marks -- each level removes one, so at most two deep -- and needs no guard of its own.

GPT FINDING (advisory) -- re.match accepts a trailing newline in the code-fence language: FIXED.

Correct, and it is the same class as the round-2 fence hole, which makes it worth more than its cosmetic impact. $ also matches just before a trailing newline, so "python\n" passed a check whose comment claims one token: measured match=True, fullmatch=False, and the fence rendered with a blank first line inside the block. Now fullmatch, and the anchors are gone rather than left in place -- they are what made the check look stricter than it was. Only a single trailing newline could ever pass, so this was a spurious blank line and not a fence escape.

86 tests pass, 46 in the wider Jira slice, 26 in the depth and redaction slice; flake8, isort and the black baseline gate are clean.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 20 disposition (verdict on aadfc8984), addressed in ff769c526. Design Review, First Principles and Opus were all clean on that head; GPT was the only blocker.

GPT BLOCKING -- code fences duplicate trailing newlines: FIXED, and it was two cases.

Real, and it is a content-fidelity bug rather than a cosmetic one: the newline before the closing fence SEPARATES the body from it, so emitting one unconditionally added a newline to the block's CONTENT. Measured on the round trip, where the expected content is what CommonMark reads back between the fences:

ADF body before after
"x" "x\n" "x\n" (was already right)
"x\n" "x\n\n" "x\n"
"" "\n" ""
"x\n\n" "x\n\n\n" "x\n\n"
"a\nb" "a\nb\n" "a\nb\n" (was already right)

GPT named the trailing-newline case. Checking the class rather than the instance turned up the empty body as the same root cause: it produced a block containing one blank line where the source had no content at all. Both come from the same unconditional separator and both are fixed by it being conditional.

87 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean.


A process observation for the maintainer, offered with numbers rather than as a complaint.

This is the twentieth consecutive round in which the GPT lane has returned at least one blocking finding, and I have fixed or rebutted every one on its merits -- no override has been used on this PR. The findings have NOT been noise: by my count 22 of 25 were real and self-introduced, and the three I rebutted I rebutted with a test or a rendered output rather than an assertion.

What I cannot tell from inside the loop is whether the count is converging. The last four rounds moved from a security bypass (a redaction gate defeated by a )), to a data-loss class (five container types), to a correctness class (overlapping emphasis delimiters), to this one -- a trailing newline in a code block. That trajectory reads like decreasing severity on a genuinely large surface, which is what convergence looks like; it is also what an unbounded review of a 600-line converter looks like, and I raised the same question at round 7 without an answer.

Two things would help, and both are yours to decide rather than mine:

  1. Whether the remaining surface is worth continuing to grind in THIS PR, or whether what is left belongs in follow-ups the way the shared-scanner bypass already does as Exfiltration URL scan stops at a ')' in the path, leaving the query unscanned #7611. The converter's core contract -- no lost content, no injected markup, no unredacted credential, bounded work -- is pinned by 87 tests and has been stable for several rounds; what keeps turning up now is fidelity at the edges.
  2. Whether a round cap is appropriate here. I will keep going as long as the loop runs, because every finding so far has been legitimate, but a PR that cannot converge because its reviewer always finds one more true thing is a process question, not an engineering one.

I am not requesting an override and will not post one without your sign-off.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 21 disposition (verdict on ff769c526), addressed in ea4170083. Design Review, First Principles and Opus all clean on that head; GPT the only blocker.

GPT BLOCKING -- explicit zero-based lists are renumbered: FIXED, plus a worse sibling in the same expression.

Real. _int_or_zero(...) or 1 cannot tell an explicit order: 0 from an absent one, so a list ADF says starts at 0 rendered starting at 1. Both halves of that are worth stating: ADF permits order: 0, and CommonMark honours it -- 0. a parses to <ol start="0">, checked rather than assumed.

Auditing the same expression turned up a second defect that GPT did not report and that is worse in effect, because it does not merely renumber -- it stops the list being a list:

start rendered
999999999. a <ol start="999999999">
1000000000. a <p>1000000000. a</p>

Ten digits is not a list marker, so an out-of-range order turned the whole list into paragraphs carrying visible numbers. And because the marker increments per item, a start of 999999999 -- itself valid -- overflows on its SECOND item, so the bound has to be on the last item's marker rather than on the attribute. Out-of-range now falls back to 1.

order is honoured only when it is a genuine non-negative int; bool is excluded explicitly, since it is an int in Python and order: true is not a start number. Three tests: the zero start, the boundary at 999999998 / 999999999 / 10^9, and the non-integer fallbacks. _int_or_zero still has eleven other callers and is unchanged.

90 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean.

On the convergence note I left last round: this finding is the same shape as the previous one -- a real fidelity defect at an edge, found in code the PR introduced, fixed in one expression with its class audited. I am still not requesting an override. The two questions from that comment remain open and are yours: whether the remaining edge work belongs in follow-ups like #7611, and whether a round cap applies here.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 22 disposition (verdict on ea4170083), addressed in a7b83ad03. Design Review, First Principles and Opus clean on that head.

GPT BLOCKING -- apostrophes truncate URL security scanning: FIXED. This one is my own class-audit failure, and it is worth naming as such.

Real. _URL_RE's path class is [^\s)\"'>]*, so the parenthesis I fixed in round 17 was one member of a class of six. Measured, with the blob in a query and only the paren handled:

terminator in path leaked before
) no (round 17)
' yes
" yes
> yes
space yes
tab / newline yes

I have spent several rounds correctly widening GPT's findings from the instance to the class, and here I did the opposite in my own code -- worse, I wrote in #7611 that "<, >, \" and ' in the same character class" were worth checking, and then did not check them in the gate I had just written. Identifying a class and fixing one member of it is not a partial fix; for a security gate it is a false sense of one.

The scan form now percent-encodes every character that terminates the match, via one table rather than chained .replace calls -- which is also what made the omission easy to miss. \s in Python is [ \t\n\r\f\v], so all six whitespace characters are in the table rather than the three that come to mind. The encoded form is still only used to DECIDE; a URL that passes is emitted exactly as it arrived.

Both directions are tested: all ten characters are pinned as sealed, and a benign Confluence URL containing an apostrophe keeps its link, since over-blocking would silently strip ordinary Jira links.

92 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean.

I have added the measurements to #7611, since they make the shared-scanner case stronger than when I filed it: the bypass is not one character, it is the whole terminating class, and it applies to every caller of that scanner.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 23 disposition (verdicts on a7b83ad03), addressed in 943c57ec2.

GPT BLOCKING -- wider body rows lose cells: FIXED. This one is a consequence of my own round-5 change.

Real, and it is silent data loss. GFM fixes a table's width at the HEADER row: a row with FEWER cells gets empty ones inserted, but a row with MORE has the excess dropped. Verified against a GFM parser -- under a two-column header, | c | d | e | f | renders only c and d.

In round 5 I removed the ragged-row padding because it was quadratic, and justified it in the docstring with "GFM already inserts empty cells for a row shorter than the header and ignores a longer row's excess". The first half is true; on the second I wrote "ignores" and reasoned about it as a layout question, when it means the text is gone. The right fix in round 5 was to remove the quadratic, not the width normalisation.

The header and separator are now widened to the widest row and body rows are left alone. That keeps both properties: no cell is dropped, and the quadratic does not come back, because only two lines grow rather than every row. A test pins the correctness case and a second pins the shape that caused the original quadratic -- one 400-cell row among 400 single-cell rows now emits under 2500 pipes, against the 40,602 that padding every row produced.

Design Review CONCERNS 1 -- the escape set mirrors the frontend renderer's plugins with nothing to catch drift: FIXED, as suggested.

This was the right thing to flag: the escape set is DERIVED from that stack -- $ because remark-math is in it, ! because rehypeRaw admits an auto-fetching image, ~ because of GFM strikethrough -- and the coupling existed only in Python comments, across a language boundary.

There is now a test that reads MarkdownRenderer.tsx and asserts its exact set of remark/rehype imports. Adding a plugin turns the BACKEND test red with a message saying to re-derive the escape set. It pins the stack rather than the conclusion, which is the direction that catches drift instead of freezing it, and skips rather than fails if the frontend is not in the checkout.

Design Review CONCERNS 2 -- the shadow terminator set will drift when #7611 lands: ACCEPTED, recorded in the code.

Correct. The constant now says explicitly that it is a shadow of the scanner's set, exists only while the scanner carries the bug, and should be retired rather than maintained when #7611 lands -- two copies of one set drift, and the copy that matters is the scanner's.

Design Review SUGGESTION -- extract the ~830-line converter into its own module: ACCEPTED AND DEFERRED, deliberately not in this PR.

I agree with the reasoning and would support it as a follow-up. Declining here is about risk, not disagreement: a pure move of 830 lines out of a 4k-line file would rewrite essentially the whole diff of a PR that has taken 23 review rounds to converge, discard the line-level review context every lane has built up, and make the real changes in this round invisible inside the move. A no-behaviour-change extraction reviews cleanly on its own and badly on top of this. It is also strictly easier after the fact, since the module boundary is now exactly the set of functions this PR added.

95 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. First Principles PASS, Opus clean.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 24 disposition (Design Review on 943c57ec2), addressed in 23770c067. GPT and Opus have not yet reported on that head; this round is Design Review's remaining concern, taken as asked.

Design Review CONCERNS 1 -- no test spans the backend/frontend boundary: DONE, the stronger version you asked for.

One correction first, because it changes what was outstanding rather than whether: 943c57ec2 -- the head reviewed -- does contain a boundary test. It reads MarkdownRenderer.tsx and asserts its exact remark/rehype import set, so a plugin addition already turned the BACKEND test red. So the boundary was pinned; what it pinned was the trigger.

Your stronger ask is a different and better thing, and I have built it: "a fixture (or shared contract test) that renders converter output through the real plugin stack". Pinning the plugin list detects that the stack changed. Rendering through the stack verifies the escape set actually holds. The second subsumes the first as evidence, so both are now in place.

test/fixtures/adf_markdown_safety.json is read by both sides:

  • The BACKEND test asserts the converter turns each adf into exactly that markdown, so a case cannot drift from the converter.
  • website/src/test/MarkdownRenderer.adfSafety.test.tsx renders the same markdown through the real MarkdownRenderer and asserts, per case, that no <strong>, <b>, <img>, KaTeX node or recoverable credential appears, and that the literal text the author typed is what the reader sees -- backslashes included in neither.

Six cases: literal emphasis, literal HTML including an <img> tag, literal $$math$$, literal ![image](url) syntax, external media rendering as an anchor rather than an image, and a credential split across marks. A failure there means the escape set needs re-deriving, which the file says in as many words so the next person does not "fix" the fixture instead.

Verified rather than assumed, and this needed a detour worth recording: this worktree has no node_modules, so I could not run vitest in it. Rather than push an unverified frontend test into a lane I cannot execute, I ran it in a clean sibling checkout that does have the dependencies -- 7 tests pass against the real stack -- then mutation-checked it by unescaping the emphasis case in that copy, which fails with strong rendered and passes everything else. The sibling checkout is back to zero local changes.

Design Review CONCERNS 2 -- a human should confirm #7611 stays live: NOTED, and agreed that it is a human's to hold.

#7611 is open with the measurements from round 22 added, and the shadow set in the code says to retire it when that lands rather than maintain two copies. I cannot guarantee an issue stays alive, which is exactly why it is filed rather than left in a docstring.

Design Review SUGGESTION -- extract the converter into its own module: still ACCEPTED AND DEFERRED.

Unchanged from last round, and your new framing ("would make the #7611 retire step a one-file change") is a fair argument for it. The reason to keep it out of THIS PR is unchanged: a pure move of ~850 lines would rewrite the whole diff of a PR at 24 review rounds and bury this round's real changes inside the move. It reviews cleanly as its own no-behaviour-change follow-up.

96 backend tests pass, 46 in the wider Jira slice, 7 frontend; flake8, isort and the black baseline gate are clean.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of b231f25e29e428fe68fecf0bf04565743ed89b12 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All the evidence is in: this is a backend converter change whose output is user-visible in the Issues panel, plus tests and a shared fixture; no screenshots and no new frontend surface. The verdict below is my review.

UX-Verdict: PASS

Jira issues now render with the same rich formatting as GitHub/GitLab sources; escaping and link-not-image choices are sound and disclosed.

Suggestions

  • _adf_url_link returns "" when a media node has no public url, dropping its alt too — a description reading "see screenshot:" is followed by nothing, with no hint an attachment exists. Emit the alt (_adf_attr_label(attrs.get("alt"))) as literal text when the URL is absent, as _adf_plain_text already does for the redaction gate.
  • Panels flatten to a bare blockquote, so an ADF warning/error panel reads as a neutral quote and the severity signal vanishes. Prefix the quote with a bold attrs.panelType label (e.g. > **Warning**) when it isn't info.

[UX-REVIEWED] b231f25

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 25 disposition (verdict on 23770c067), addressed in 4fba70e32.

GPT BLOCKING -- attribute labels bypass suspicious-URL redaction: FIXED, and it was four sites, not one.

Real, and mine. Rounds 17 and 22 normalised the URL before scanning at the link-destination chokepoint only. The truncation is a property of the SCANNER, so it applies wherever provider text is scanned -- and _adf_attr_label scanned the raw value. Measured with a high-entropy query behind a ):

emission site leaked before
expand title (attribute label) yes -- GPT's report
mention label (attribute label) yes
inline card URL (the inline-run gate) yes
external media URL (the inline-run gate) yes
link destination no (rounds 17, 22)

The two extra classes matter because they are not attribute labels: the inline-run gate scans _adf_plain_text, which returns the URL for a card or media node, so it was scanning URLs through the truncating path too. GPT named one of four; the fix is one shared primitive, _md_redact_untruncated, now used by all three chokepoints.

This is the third round in which the same underlying bug has been reported at a new call site, and the reason is worth stating plainly: I kept fixing the site GPT named instead of the property. The property is "every place this converter scans provider text must scan a form the URL regex cannot truncate", and it is now expressed once, in one function, with the call sites routed through it rather than each holding a copy of the trick.

One deliberate behaviour note: when nothing is found the ORIGINAL text is emitted, so the percent-encoding is never visible in the ordinary case -- pinned by a test that an everyday URL containing parentheses, an apostrophe and a > comes back byte-identical. Only when the scan fires does the redacted encoded form appear, where a stray escape sits beside a redaction marker.

98 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean.

Backend Tests (Windows) (3) red: not mine, and not a runner timeout.

The failure is test/test_sel.py::TestThreadSafety::test_concurrent_writes - assert 39 == 40 -- 39 of 40 concurrent writes landed. Evidence it is not this PR:

  • The diff is four files: source_providers.py, its test, the shared fixture and the new frontend test. It contains zero lines touching SEL, threading or concurrency -- grep for those terms in the source diff returns nothing.
  • The job ran 14m52s, so this is a real assertion failure rather than the 30-minute runner timeout.
  • Main's own CI on ebc0936f2, this PR's exact merge-base, also fails Backend Tests (Windows) (3) -- with a different flaky test (test_perf_sampler.py::TestCliSample::test_in_process_run_writes_a_private_artifact). Same shard, same platform, main-owned instability, so the shard is not byte-identically failing but is demonstrably unstable on main.

I could not rerun the job (gh reports the job cannot be rerun while its parent run is still in progress). The push above has started a fresh run of the whole matrix, which re-runs this shard on the new head; I will report the outcome. No unrelated fix has been folded in.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 26 disposition (verdict on 4fba70e32), addressed in 6c72e32c8. This is a NEW finding, not a re-raise: it is a regression my round-25 fix introduced, and it is correct.

GPT BLOCKING -- whole-text encoding corrupts benign Jira prose: FIXED.

Reproduced exactly as described. Round 25 encoded the scan form across the WHOLE text, whitespace included, which joined a URL's query to the word after it. A comment citing a URL and a commit SHA:

see https://example.com/pr?id=7 9f2c1ab4de5607893bcf24e01a7d6b3958e04c12 for detail

scanned as ONE address whose query carried the SHA, so the entropy heuristic fired and the paragraph was emitted as:

see%20\[REDACTED: suspicious URL to example.com\]

Every word after the URL gone, on entirely benign content. That is worse than the leak it was guarding, and I had noted the whitespace risk while writing round 25 before choosing the conservative path anyway -- the conservative choice was the destructive one.

The fix is not simply "don't encode whitespace", and the test caught me getting that wrong. My first attempt encoded terminators only inside URL spans, on the reasoning that a space ends a URL. That is true in prose and false for a link DESTINATION: _md_link_target emits the angle-bracket form with whitespace as %20, so a space there does not end the URL -- it joins. The existing terminator test failed on exactly that case, which is what surfaced it.

So the rule is now stated properly: scan the form that will actually be EMITTED.

  • A link destination is one address; the scan encodes whitespace because the emission does.
  • In prose the scan encodes only the terminators that can sit inside a URL, within https?://\S* spans, because a space really does end the URL there.

The prose half is verified against the real renderer rather than argued from the spec: for https://host/a?data= <blob> the emitted anchor's href is https://host/a?data=, so following text cannot ride along in a fetchable address. I ran that through MarkdownRenderer in a checkout with the frontend dependencies and removed the probe afterwards.

Both directions are pinned: all ten terminators stay sealed on the destination path, and the URL-plus-SHA paragraph now comes back byte-identical with no %20 and no marker.

99 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean.

On the Windows shard: it went green on the previous head, consistent with the round-25 analysis that it is main-owned flake in test_sel.py's concurrency test rather than anything in this diff.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 27 disposition (verdict on 6c72e32c8), addressed in a482cbe9f. Design Review, First Principles, Opus and UX Review are all clean on that head.

GPT BLOCKING -- code-block URLs bypass complete redaction: REBUTTED with rendered evidence, and the reasoning is GPT's own from last round.

The mechanism is accurately described: a code body goes through _adf_plain_text and is fenced without passing the URL-entropy scan, so a ) before a high-entropy query is not caught there. What does not hold is the consequence, because exfiltration needs the URL to be FETCHABLE, and in code it is not.

Rendered through the project's own MarkdownRenderer, same URL in three positions:

position anchors images
fenced code block 0 0
inline code span 0 0
bare text in a paragraph 1, href carrying the ) and the whole query 0

So a URL inside code is text, not an address: nothing auto-fetches it and there is nothing to click. This is precisely the reasoning GPT's OWN previous finding established -- whitespace ends a URL in prose, so encoding it to make a blob vanish was destruction without a gain. The same test applies here, and the answer comes out the same way.

Adding the scan to code bodies would have a real cost in the same direction as that finding: an API example containing a long opaque token reads exactly like an exfiltration query, so legitimate code samples in Jira issues would be replaced by a redaction marker. Trading working code samples for a threat that cannot be reached is the wrong side of that trade.

Credentials in code blocks are NOT unprotected, which is the part worth being explicit about: the payload-level pass is token-shaped, so it catches ghp_... in a code body regardless of any URL truncation. What a code body does not get is the URL-entropy heuristic, which is the check that only matters for a fetchable address.

Two tests record the boundary rather than leaving it in a comment: a code body containing such a URL is emitted verbatim, and the case that DOES linkify -- a bare URL with a ) in ordinary prose, which the table above shows becomes an anchor carrying the full query -- is confirmed redacted, with the marker naming the host so a reader knows what was removed.

101 tests pass, 46 in the wider Jira slice, 7 frontend; flake8, isort and the black baseline gate are clean.

I am not requesting an override: this is a rebuttal on the merits with the renderer's output as evidence, and if the lane re-raises it I will escalate rather than argue it twice.

Jira Cloud v3 returns a description and each comment body as ADF, and the
walker that read them collected text leaf nodes only, so every heading,
list, link URL, code fence and table was gone before the Issues panel saw
the field. That panel renders the field through MarkdownRenderer, and every
other provider already puts real markdown in the same payload key (a GitHub
issue body, a GitLab description), so Jira was the one source whose rich
text arrived flattened.

Replace _adf_to_plain_text with _adf_to_markdown, which maps each ADF node
type to its markdown form and escapes literal text so a description cannot
smuggle markup into the panel. The Server/DC path (a str of wiki markup,
not ADF) is untouched.

Closes #2581
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 28 disposition (verdict on a482cbe9f), addressed in b231f25e2. Design Review, First Principles and UX Review are all PASS on that head.

GPT BLOCKING -- media alt bypasses run-level credential redaction: FIXED. It looks like a re-raise of round 16 and it is not; my own later change is what made it true.

I rebutted this in round 16 with rendered output: a media node emits [alt](url), so the bracket sits between a text node's tail and the alt and the two cannot form one token. That was correct for the code as it stood.

Round 17 then added a path where a URL failing the destination scan DROPS the link and emits the label alone -- no brackets. GPT's wording names exactly that ("rendered link delimiters disappear"), and it is right. My rebuttal quietly stopped holding two rounds after I made it, and nothing in the test suite noticed, because the test I added in round 16 pinned the bracketed case only.

It took a specific payload to isolate, which is worth recording so the disposition is not just an assertion:

  • 20 characters after ghp_ -- too short to match the credential pattern alone, and still too short when the gate appended the URL, since ...Tvhttps stops at the colon.
  • 12 more in the alt, reaching 32 once joined.
  • A media URL containing whitespace, which fails the DESTINATION scan (that path encodes whitespace) while the prose run gate's URL span stops at the space, so the run gate did not fire either.

Output before: ghp\_Ab3Df6Hj9Kl2Np5Qr8TvWx4Yz7Bc0Ef3 -- one recoverable credential, and the backslash escaping ghp_ then defeats the payload-level pass, which is the failure mode from round 3. After: \[REDACTED: credential\].

The fix is the one GPT prescribed. _adf_plain_text now returns a media node's ALT with the URL as fallback, so the run gate sees the contiguity the output can actually have. When the brackets DO survive it now errs toward more contiguity than the output has -- a false positive rather than a false negative, which is the right way round. An inline card still returns its URL, because a card has no alt and its label IS the redacted URL.

Round 16's test is deleted rather than left passing beside its own counterexample, and replaced by one that asserts both the bracketed and the dropped-link cases redact, plus one confirming an ordinary alt still renders as a link so the stricter gate does not cost the normal case.

What I am taking from this. Three of the last four findings have been in code the earlier rounds introduced, and this one specifically invalidated a rebuttal I had already given. A rebuttal is only as durable as the code path it describes; when I later changed that path I did not go back and re-check what it had been holding up. The tests I write for a rebuttal need to pin the property, not the one shape I measured.

102 tests pass, 46 in the wider Jira slice, 7 frontend; flake8, isort and the black baseline gate are clean.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed via parallel subagent audit: diff matches description, CI fully green, no blocking findings, no unresolved threads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Convert Jira ADF descriptions to markdown instead of plain text

2 participants