Repository navigation
fix: preserve bold formatting in collapsible report summaries - #71273
Conversation
Preserve typed inline formatting in collapsible summaries while keeping unsupported HTML, scalar substitutions, mentions, and secrets protected. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Comment
One performance issue found in the new summary tokenizer: repeated slicing/rescanning can make large malformed summaries quadratic to parse. See the inline comment for the impact and a bounded fix.
Review scope
Static review of the changed lines only; no repository code or tests were executed per review instructions. The specialist pass completed and its findings were independently adjudicated.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~3 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
| text(source); | ||
| break; | ||
| } | ||
| text(source.slice(0, start)); |
There was a problem hiding this comment.
This tokenizer can make parsing a large agent-controlled summary quadratic, causing avoidable CPU/latency spikes before the report can be published.
💡 Why this matters
Each iteration finds the next < and then creates a shortened source with slice; malformed input containing many < characters repeatedly copies/rescans the remaining suffix. Since summaries are imported from report content, a sufficiently large adversarial summary can turn this into O(n2) work. Walk with an index (and search each code closer from that index) or enforce a summary length bound before tokenization.
There was a problem hiding this comment.
Skills-Based Review 🧠
Triage (fallback heuristic, pr-triage output used directly): bug_fix. Applied /diagnosing-bugs and /codebase-design to the core AST/parser change in report_ast.cjs and report_markdown.cjs.
This is a well-scoped fix with strong regression coverage (nesting depth, redaction splits, size fitting, unsafe HTML, scalar substitutions) — the test matrix in report_ast.test.cjs is genuinely thorough and traces the original bug (#71245) correctly to the renderer escaping formatting tags rather than patching the symptom.
📋 Key Themes & Highlights
Key Themes
- Validation gap: the new array-summary path does not trim before the empty-summary guard (
report_ast.cjs:399-400), so whitespace-only formatted summaries (e.g.<b> </b>) silently pass validation while the equivalent plain-text case correctly throws. This is a behavioral regression relative to the existing invariant — flagged as an inline comment with a concrete fix. - Minor inconsistency: formatted summaries preserve surrounding whitespace verbatim while string summaries collapse/trim it, producing visibly different output for semantically equivalent inputs. Lower severity but worth aligning for predictability.
Positive Highlights
- ✅ Security-conscious design: unsupported/attribute-bearing HTML, encoded tags, and scalar substitutions are deliberately kept literal/escaped rather than promoted to live formatting — this is the right call for an agent-generated-content renderer.
- ✅ Redaction, size-fitting, and content-policy traversal were all correctly extended to the new array-summary shape, with tests verifying framework-owned descendants and protected content survive.
- ✅ Round-trip idempotency is explicitly tested (parse → render → parse → render produces the same output).
Note: investigation was bounded per the stated budget; manual tracing (not a local vitest run, which isn’t available offline in this sandbox) was used to confirm both findings via direct Node execution of the parser/renderer.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~113 AIC · ⌖ ~15 AIC · ⊞ 10.2K
Comment /matt to run again
| const summary = normalizeReportText(node.summary).replace(/\s+/g, " ").trim(); | ||
| if (!summary) throw new TypeError("Report details must have a summary"); | ||
| const summary = typeof node.summary === "string" ? normalizeReportText(node.summary).replace(/\s+/g, " ").trim() : normalizeInlines(node.summary, depth + 1); | ||
| if (!summary.length) throw new TypeError("Report details must have a summary"); |
There was a problem hiding this comment.
[/diagnosing-bugs] Formatted summaries can bypass the "must have a summary" guard with whitespace-only content, unlike string summaries which trim first. <details><summary><b> </b></summary> round-trips to a visible-but-empty bold summary instead of throwing.
💡 Repro and suggested fix
// string path trims then rejects empty:
parseReportMarkdown("<details><summary> </summary>..."); // throws "Report details must have a summary"
// array path does not trim before the length check:
parseReportMarkdown("<details><summary><b> </b></summary>..."); // succeeds, renders <summary><b> </b></summary>Consider checking collectInlineText(summary, false).trim() (or similar) before the !summary.length guard at line 400, so an all-whitespace formatted summary is rejected the same way a plain-text one is.
@copilot please address this.
| if (node.type === "details") { | ||
| const summary = normalizeReportText(node.summary).replace(/\s+/g, " ").trim(); | ||
| if (!summary) throw new TypeError("Report details must have a summary"); | ||
| const summary = typeof node.summary === "string" ? normalizeReportText(node.summary).replace(/\s+/g, " ").trim() : normalizeInlines(node.summary, depth + 1); |
There was a problem hiding this comment.
[/codebase-design] String summaries are collapsed/trimmed (.replace((s/redacted)+/g, " ").trim()), but formatted (array) summaries keep leading/trailing whitespace verbatim, e.g. <summary> <b>Label</b> </summary> renders with stray spaces around the bold text. This divergence is easy to miss since both paths share the same details case.
💡 Suggestion
Trim the leading/trailing whitespace-only text runs produced by normalizeInlines(node.summary, depth + 1) so both summary representations normalize consistently, keeping the two code paths' observable behavior aligned.
@copilot please address this.
There was a problem hiding this comment.
🔵 Needs a closer look
Security-sensitive HTML publication and redaction changes need human review, with unresolved parsing and literal-tag preservation defects.
2 open issues
What changed in this PR
This PR extends gh-aw’s typed report pipeline to preserve formatting in collapsible summaries, addressing escaped bold labels in generated reports.
Changes:
- Import supported summary formatting into inline nodes and render it as HTML.
- Extend secret redaction, content-policy traversal, and size fitting to formatted summaries.
- Add regression coverage and a patch release note.
| File | Description |
|---|---|
actions/setup/js/report_size.test.cjs |
Tests size fitting with protected summary content. |
actions/setup/js/report_size.cjs |
Traverses formatted summaries during size fitting. |
actions/setup/js/report_redaction.test.cjs |
Tests redaction while preserving framework-owned summary content. |
actions/setup/js/report_redaction.cjs |
Applies content-policy redaction to summary nodes. |
actions/setup/js/report_markdown.cjs |
Parses supported summary HTML into typed inline nodes. |
actions/setup/js/report_ast.test.cjs |
Adds formatting, escaping, round-trip, and secret-redaction coverage. |
actions/setup/js/report_ast.cjs |
Normalizes, renders, and checks formatted summaries. |
.changeset/patch-report-summary-bold.md |
Records the summary-formatting patch. |
Review effort: Max
| continue; | ||
| } | ||
| source = source.slice(tag.raw.length); | ||
| const token = /^<(\/?)(b|strong|i|em|s|del|sub|code)>$/i.exec(tag.raw); |
| } | ||
| const name = token[2].toLowerCase(); | ||
| if (name === "code" && !token[1]) { | ||
| const end = /<\/code>/i.exec(source); |
|
🎉 This pull request is included in a new release. Release: |
Summary
Collapsible report summaries were publishing literal
<b>...</b>text instead of bold labels, as seen in #71245. The report parser treated summaries as plain strings and the renderer escaped their formatting tags.Import attribute-free summary formatting into typed inline nodes and render those nodes as HTML within
<summary>. Keep unsupported HTML, encoded literal tags, and scalar substitutions escaped. Extend secret redaction, content-policy traversal, and size fitting to formatted summaries while preserving framework-owned and protected content.This fixes future generated reports. It does not edit #71245 or triage its security findings.
Validation
make agent-report-progressgate passed, including build, type checking, linting, and impacted tests.