Tighten step summary hierarchy and progressive disclosure#43495
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot avoid headers, instead treat details.summary as the header BAD: GOOD: |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43495 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR standardizes GitHub Actions step-summary formatting across multiple engine log parsers by removing emoji-heavy headings and moving verbose content behind <details>/<summary> disclosure blocks, while aligning parser fallback/status output and observability warnings with the same hierarchy.
Changes:
- Replaces several
## …/emoji section headers with<details><summary>…</summary>…</details>sections for Initialization/Reasoning/Commands/Information (and similar). - Introduces a shared
buildStepSummaryDetailsSectionhelper and updates multiple parsers to use it for fallback/status rendering. - Cleans up step-summary copy (e.g., removes emoji emphasis from the OTLP export warning) and updates tests to assert the new summary structure.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/parse_pi_log.test.cjs | Updates expectations to match <details>/<summary> section rendering. |
| actions/setup/js/parse_pi_log.cjs | Uses shared details helper for parser-status fallback output. |
| actions/setup/js/parse_gemini_log.test.cjs | Updates expectations to match <details>/<summary> section rendering. |
| actions/setup/js/parse_gemini_log.cjs | Uses shared details helper for parser-status fallback output. |
| actions/setup/js/parse_custom_log.cjs | Switches fallback rendering to a single collapsible parser-status/raw-preview details section; normalizes heading to ### for identified formats. |
| actions/setup/js/parse_copilot_log.test.cjs | Updates expectations to match <details>/<summary> section rendering and removes icon-based assertions. |
| actions/setup/js/parse_copilot_log.cjs | Uses details helper for parser-status fallback and firewall steering notices. |
| actions/setup/js/parse_codex_log.test.cjs | Updates expectations to match <details>/<summary> section rendering and adjusts thinking formatting assertions. |
| actions/setup/js/parse_codex_log.cjs | Converts key sections to <details>/<summary> blocks and uses details helper for initialization and fallback paths. |
| actions/setup/js/parse_claude_log.test.cjs | Updates expectations to match <details>/<summary> section rendering and adjusts thinking formatting assertions. |
| actions/setup/js/parse_claude_log.cjs | Uses shared details helper for parser-status fallback output. |
| actions/setup/js/parse_antigravity_log.test.cjs | Updates expectations for normalized plain-text ### header and new formatting. |
| actions/setup/js/parse_antigravity_log.cjs | Uses shared details helper for parser-status and final-response rendering. |
| actions/setup/js/log_parser_shared.test.cjs | Updates shared-parser expectations for new section structure and “no information” behavior. |
| actions/setup/js/log_parser_shared.cjs | Adds buildStepSummaryDetailsSection, updates information section rendering, and removes emoji from size-limit warning. |
| actions/setup/js/log_parser_format.cjs | Updates shared conversation markdown rendering to use <details>/<summary> progressive disclosure and introduces a local details helper. |
| actions/setup/js/generate_observability_summary.test.cjs | Updates expectation to remove emoji emphasis in OTLP warning copy. |
| actions/setup/js/generate_observability_summary.cjs | Removes emoji emphasis from OTLP export failure warning line. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 18/18 changed files
- Comments generated: 3
- Review effort level: Low
| /** | ||
| * Builds a step-summary section with a collapsible details body whose summary | ||
| * acts as the section header. | ||
| * @param {string} title | ||
| * @param {string} summary | ||
| * @param {string} body | ||
| * @param {{open?: boolean, emptyBodyMessage?: string}} [options] | ||
| * @returns {string} | ||
| */ | ||
| function buildStepSummaryDetailsSection(title, summary, body, options = {}) { | ||
| const { open = false, emptyBodyMessage = "No details available." } = options; | ||
| const openAttr = open ? " open" : ""; | ||
| const trimmedBody = typeof body === "string" ? body.trim() : ""; | ||
| const content = trimmedBody || emptyBodyMessage; | ||
| const summaryText = typeof title === "string" && title.trim() ? title : summary; | ||
| return `<details${openAttr}>\n<summary>${summaryText}</summary>\n\n${content}\n</details>\n\n`; | ||
| } |
| const initEntry = renderEntries.find(entry => entry.type === "system" && entry.subtype === "init"); | ||
|
|
||
| if (initEntry && formatInitCallback) { | ||
| if (!addContent("## 🚀 Initialization\n\n")) { | ||
| return { markdown, commandSummary: [], sizeLimitReached }; | ||
| } | ||
| const initResult = formatInitCallback(initEntry); | ||
| if (typeof initResult === "string") { | ||
| if (!addContent(initResult)) { | ||
| return { markdown, commandSummary: [], sizeLimitReached }; | ||
| } | ||
| } else if (initResult && initResult.markdown) { | ||
| if (!addContent(initResult.markdown)) { | ||
| return { markdown, commandSummary: [], sizeLimitReached }; | ||
| } | ||
| } | ||
| if (!addContent("\n")) { | ||
| const initBody = typeof initResult === "string" ? initResult : initResult && initResult.markdown ? initResult.markdown : ""; | ||
| if (!addContent(buildStepSummaryDetailsSection("Initialization", "Show initialization details", initBody))) { | ||
| markdown += SIZE_LIMIT_WARNING; | ||
| return { markdown, commandSummary: [], sizeLimitReached }; | ||
| } | ||
| } | ||
|
|
||
| if (!addContent("\n## 🤖 Reasoning\n\n")) { | ||
| return { markdown, commandSummary: [], sizeLimitReached }; | ||
| } | ||
| let reasoningBody = ""; | ||
| let commandDetailsBody = ""; | ||
|
|
||
| for (const entry of renderEntries) { | ||
| if (sizeLimitReached) break; | ||
|
|
||
| if (entry.type === "assistant" && entry.message?.content) { | ||
| for (const content of entry.message.content) { | ||
| if (sizeLimitReached) break; | ||
| if (entry.type !== "assistant" || !entry.message?.content) { | ||
| continue; | ||
| } | ||
|
|
||
| if (content.type === "text" && content.text) { | ||
| let text = content.text.trim(); | ||
| text = unfenceMarkdown(text); | ||
| if (text && text.length > 0) { | ||
| if (!addContent(text + "\n\n")) { | ||
| break; | ||
| } | ||
| } | ||
| } else if (content.type === "thinking" && content.thinking) { | ||
| let text = content.thinking.trim(); | ||
| text = unfenceMarkdown(text); | ||
| if (text && text.length > 0) { | ||
| if (!addContent(`<sub>◐ <em>${text.replace(/\n/g, "<br>")}</em></sub>\n\n`)) { | ||
| break; | ||
| } | ||
| } | ||
| } else if (content.type === "tool_use") { | ||
| const toolResult = toolUsePairs.get(content.id); | ||
| const toolMarkdown = formatToolCallback(content, toolResult); | ||
| if (toolMarkdown) { | ||
| if (!addContent(toolMarkdown)) { | ||
| break; | ||
| } | ||
| } | ||
| for (const content of entry.message.content) { | ||
| if (content.type === "text" && content.text) { | ||
| let text = content.text.trim(); | ||
| text = unfenceMarkdown(text); | ||
| if (text) { | ||
| reasoningBody += text + "\n\n"; | ||
| } | ||
| } else if (content.type === "thinking" && content.thinking) { | ||
| let text = content.thinking.trim(); | ||
| text = unfenceMarkdown(text); | ||
| if (text) { | ||
| reasoningBody += `<sub><em>${text.replace(/\n/g, "<br>")}</em></sub>\n\n`; | ||
| } | ||
| } else if (content.type === "tool_use") { | ||
| const toolResult = toolUsePairs.get(content.id); | ||
| const toolMarkdown = formatToolCallback(content, toolResult); | ||
| if (toolMarkdown) { | ||
| commandDetailsBody += toolMarkdown; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (sizeLimitReached) { | ||
| if (!addContent(buildStepSummaryDetailsSection("Reasoning", "Show reasoning", reasoningBody))) { | ||
| markdown += SIZE_LIMIT_WARNING; | ||
| return { markdown, commandSummary: [], sizeLimitReached }; | ||
| } |
| /** | ||
| * Builds a step-summary section with a collapsible details body whose summary | ||
| * acts as the section header. | ||
| * @param {string} title | ||
| * @param {string} summary | ||
| * @param {string} body | ||
| * @returns {string} | ||
| */ | ||
| function buildStepSummaryDetailsSection(title, summary, body) { | ||
| const trimmedBody = typeof body === "string" ? body.trim() : ""; | ||
| const content = trimmedBody || "No details available."; | ||
| const summaryText = typeof title === "string" && title.trim() ? title : summary; | ||
| return `<details>\n<summary>${summaryText}</summary>\n\n${content}\n</details>\n\n`; | ||
| } |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (40 tests)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST CHANGES — 3 bugs must be fixed before merge
Blocking issues:
-
Broken test assertions ( lines 110, 131) — test now expects
### Antigravitybut the implementation produces<summary>Antigravity</summary>. These tests will fail in CI. -
Size-limit guard bypassed (
log_parser_format.cjs) — reasoning and command content is accumulated into unbounded strings before anyaddContent()check fires. A large log can OOM-allocate far past the step-summary limit before truncation kicks in. -
<summary>undefined</summary>rendered on invalid input (log_parser_shared.cjsandlog_parser_format.cjs) — thesummaryfallback parameter is used raw without type-checking; passingundefinedornullproduces broken HTML visible in the GitHub Actions UI.
Secondary concerns (non-blocking)
- Duplicate helper with narrower signature (
log_parser_format.cjsline 98): the local copy ofbuildStepSummaryDetailsSectionlacks theoptionsparameter (noopen, noemptyBodyMessage), creating a split that will diverge further. Recommend deleting the local copy and importing from the shared module. - Empty Reasoning block always emitted (
log_parser_format.cjs): when no reasoning entries exist, a<details><summary>Reasoning</summary>No details available.</details>section is still rendered, consuming budget and cluttering the UI. Guard withif (reasoningBody)before callingaddContent.
🔎 Code quality review by PR Code Quality Reviewer · 94.5 AIC · ⌖ 6.48 AIC · ⊞ 5.4K
Comment /review to run again
| const result = parseAntigravityLog(logContent); | ||
|
|
||
| expect(result.markdown).toContain("## 🤖 Antigravity"); | ||
| expect(result.markdown).toContain("### Antigravity"); |
There was a problem hiding this comment.
Broken test: asserts ### Antigravity but the implementation never produces that string.
buildStepSummaryDetailsSection("Antigravity", ...) emits <details>\n<summary>Antigravity</summary>.... There is no ### heading in that output, so this assertion — and the identical one at line 131 — will fail when tests run.
💡 Fix
Change both assertions to match the actual output:
// line 110
- expect(result.markdown).toContain("### Antigravity");
+ expect(result.markdown).toContain("<summary>Antigravity</summary>");
// line 131 — same change| if (!addContent("\n## 🤖 Reasoning\n\n")) { | ||
| return { markdown, commandSummary: [], sizeLimitReached }; | ||
| } | ||
| let reasoningBody = ""; |
There was a problem hiding this comment.
Size-limit guard bypassed: reasoning/command content is buffered unboundedly before any size check.
The old code called addContent() for every piece of reasoning/tool text, stopping as soon as the limit was hit. The new code appends freely into reasoningBody and commandDetailsBody for all entries, only calling addContent() after all content is assembled. For a large log this can allocate several megabytes more than the step-summary limit before the guard fires.
💡 Suggested fix
Enforce an upper bound on the buffers during accumulation, or restore incremental addContent() calls per entry. A simple guard:
for (const content of entry.message.content) {
if (content.type === "text" && content.text) {
// ...
if (reasoningBody.length < MAX_SECTION_SIZE) {
reasoningBody += text + "\n\n";
}
}
// ...
}Alternatively, check summaryTracker.getCurrentSize() inside the loop and break early.
| const openAttr = open ? " open" : ""; | ||
| const trimmedBody = typeof body === "string" ? body.trim() : ""; | ||
| const content = trimmedBody || emptyBodyMessage; | ||
| const summaryText = typeof title === "string" && title.trim() ? title : summary; |
There was a problem hiding this comment.
Unvalidated summary fallback renders <summary>undefined</summary> in the step UI.
When title is empty or not a non-empty string, summaryText falls back to the raw summary parameter with no type or emptiness guard. If a caller passes undefined or null as summary, the rendered HTML becomes <summary>undefined</summary>, which appears verbatim in the GitHub Actions step summary.
💡 Fix
- const summaryText = typeof title === "string" && title.trim() ? title : summary;
+ const summaryText =
+ (typeof title === "string" && title.trim()) ||
+ (typeof summary === "string" && summary.trim()) ||
+ "Details";The same bug exists in the local copy inside log_parser_format.cjs (line 101), which is another reason to consolidate to a single implementation.
| * @param {string} body | ||
| * @returns {string} | ||
| */ | ||
| function buildStepSummaryDetailsSection(title, summary, body) { |
There was a problem hiding this comment.
Duplicate buildStepSummaryDetailsSection with a divergent, narrower signature.
The shared module exports a full-featured buildStepSummaryDetailsSection(title, summary, body, options) with open and emptyBodyMessage support. This local copy inside createLogParserFormatters has signature (title, summary, body) — options is missing entirely. Future callers inside the factory who need open: true (e.g. Initialization sections that should start expanded) or a custom empty-body message will get the wrong behaviour silently, with no error.
💡 Fix
Import and use the shared implementation directly:
// At the top of createLogParserFormatters or its module scope:
const { buildStepSummaryDetailsSection } = require("./log_parser_shared.cjs");Then delete the local copy. The factory already has access to the shared module's other exports, so there is no circular dependency concern.
| } | ||
|
|
||
| if (sizeLimitReached) { | ||
| if (!addContent(buildStepSummaryDetailsSection("Reasoning", "Show reasoning", reasoningBody))) { |
There was a problem hiding this comment.
Empty reasoningBody always emits a Reasoning <details> block, even when no reasoning exists.
If no assistant entries with text or thinking content are present, reasoningBody stays "". The local buildStepSummaryDetailsSection then produces a <details><summary>Reasoning</summary>No details available.</details> block that is passed to addContent(), consuming step-summary budget and showing a vacuous section to users. The old code skipped the section entirely when there was nothing to render.
💡 Fix
Guard the wrapping call:
if (reasoningBody) {
if (!addContent(buildStepSummaryDetailsSection("Reasoning", "Show reasoning", reasoningBody))) {
markdown += SIZE_LIMIT_WARNING;
return { markdown, commandSummary: [], sizeLimitReached };
}
}Apply the same guard to commandDetailsBody.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /grill-with-docs — requesting changes on issues found in the migration.
📋 Key Themes & Highlights
Issues Found
- Duplicate helper (
log_parser_format.cjsline 98):buildStepSummaryDetailsSectionis re-implemented locally with fewer options than the canonical version inlog_parser_shared.cjs. Delete the copy and import the shared one. - Incomplete migration in
parse_codex_log.cjs: Three sections (parseCodexJsonl+parseCodexLogErrors + Reasoning) still use raw<details>string concatenation instead of the shared helper, leaving open/close tag pairs that an exception can split. - Inconsistent custom-log heading (
parse_custom_log.cjs): The two successful-parse paths return### Custom Engine Log (...)plain headings outside any<details>block, inconsistent with every other engine parser. - Confusing
title/summarypositional params: thesummaryparameter is a dead fallback — every call site passes a non-emptytitle. Consider a singleheadingparam or move the fallback intooptions. - Dead
sizeLimitReachedguard (log_parser_format.cjslines 182–185): unreachable after the refactor. - Size-limit coarsening: the buffering loop no longer checks the tracker budget per entry — the limit is now enforced at section boundaries only, not within a section.
- Test assertion mismatch (
parse_antigravity_log.test.cjsline 107): asserts"### Antigravity"but the helper emits<summary>Antigravity</summary>.
Positive Highlights
- ✅ The
buildStepSummaryDetailsSectionabstraction is well-designed —openandemptyBodyMessageoptions are thoughtful - ✅ Excellent test update discipline: every changed assertion mirrors the new HTML structure
- ✅ Emoji removal from section titles is the right call for a professional summary surface
- ✅ Removing the
◐thinking icon simplifies the rendering path nicely
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 123.1 AIC · ⌖ 6.22 AIC · ⊞ 6.6K
Comment /matt to run again
| * @param {string} body | ||
| * @returns {string} | ||
| */ | ||
| function buildStepSummaryDetailsSection(title, summary, body) { |
There was a problem hiding this comment.
[/codebase-design] buildStepSummaryDetailsSection is duplicated here from log_parser_shared.cjs — a private copy with fewer features (the open flag is absent) — and is not exported. This divergence means the two copies can drift independently.
💡 Suggestion
Remove the local copy and import the canonical version from log_parser_shared.cjs (it is already exported there). That keeps the open attribute, emptyBodyMessage, and any future options consistent across the whole codebase without per-file patches.
// top of log_parser_format.cjs closure
const { buildStepSummaryDetailsSection } = require('./log_parser_shared.cjs');Having two diverging implementations of the same helper is a classic deep-module smell: the shared module already exposes the full interface, so the private copy adds complexity with no benefit.
@copilot please address this.
| const errorInfo = extractCodexErrorMessages(lines); | ||
| if (errorInfo.hasErrors) { | ||
| markdown += "## ⚠️ Errors\n\n"; | ||
| markdown += "<details>\n<summary>Errors</summary>\n\n"; |
There was a problem hiding this comment.
[/codebase-design] The Errors block in parseCodexLog is the only section still using inline <details>/</details> string concatenation instead of the shared buildStepSummaryDetailsSection helper. This creates an inconsistency: the helper's emptyBodyMessage fallback and open support are absent here.
💡 Suggestion
Collect the error messages into a string variable first, then pass it to buildStepSummaryDetailsSection:
if (errorInfo.hasErrors) {
let errorsBody = errorInfo.messages.map(m => `> ${m}`).join('\n\n') + '\n\n';
if (errorInfo.reconnectCount > 0) {
errorsBody += `> Reconnect attempts: ${errorInfo.reconnectCount}/${errorInfo.maxReconnects}\n\n`;
}
markdown += buildStepSummaryDetailsSection('Errors', 'Show errors', errorsBody);
}This also eliminates the unclosed-tag risk if an exception occurs mid-concatenation.
@copilot please address this.
| // Build markdown so the parser returns a truthy result and core.info has a | ||
| // readable fallback. The step summary itself is rendered from logEntries. | ||
| let markdown = "## 🤖 Reasoning\n\n"; | ||
| let markdown = "<details>\n<summary>Reasoning</summary>\n\n"; |
There was a problem hiding this comment.
[/codebase-design] parseCodexJsonl manually concatenates <details> tags using raw string literals instead of the shared buildStepSummaryDetailsSection helper, bypassing the helper's guardrails (emptyBodyMessage, open, trim). This is the same pattern that was eliminated everywhere else in this PR.
💡 Suggestion
Buffer each section's body (reasoning, commands, information) into a variable, then call buildStepSummaryDetailsSection — exactly as generateConversationMarkdown now does. That removes the three raw string-literal <details> blocks starting at lines 417, 425, and 435 and makes this function consistent with the rest of the module.
@copilot please address this.
| const openAttr = open ? " open" : ""; | ||
| const trimmedBody = typeof body === "string" ? body.trim() : ""; | ||
| const content = trimmedBody || emptyBodyMessage; | ||
| const summaryText = typeof title === "string" && title.trim() ? title : summary; |
There was a problem hiding this comment.
[/codebase-design] The title/summary parameter relationship is confusing: when title is a non-empty string it is used; otherwise summary is the fallback. In every single call site, title is always non-empty, making the summary parameter a dead path with no test coverage.
💡 Suggestion
Simplify to one label parameter, or rename clearly so the intent is obvious at the call site. For example:
function buildStepSummaryDetailsSection(heading, body, options = {}) { ... }If a separate collapsed-state hint is needed, use an options.summaryLabel field rather than a positional parameter that silently shadows heading. This also removes the dead-code branch and the need for the type-guard on line 279.
@copilot please address this.
| } | ||
|
|
||
| if (sizeLimitReached) { | ||
| if (!addContent(buildStepSummaryDetailsSection("Reasoning", "Show reasoning", reasoningBody))) { |
There was a problem hiding this comment.
[/codebase-design] The sizeLimitReached check after addContent for the Reasoning block (line 182–185) can never be triggered. addContent already sets sizeLimitReached = true and returns false when the limit is reached — the early return on line 178–180 handles that case. The duplicated guard on line 182 is dead code.
💡 Suggestion
Remove lines 182–185:
// before — duplicate guard
if (!addContent(buildStepSummaryDetailsSection(...))) {
markdown += SIZE_LIMIT_WARNING;
return { ... };
}
if (sizeLimitReached) { // ← unreachable
markdown += SIZE_LIMIT_WARNING;
return { ... };
}The old code needed this guard because sizeLimitReached could be set inside the rendering loop. The refactored loop no longer touches addContent during buffering, so the separate check is redundant.
@copilot please address this.
| } | ||
| if (!addContent("\n")) { | ||
| const initBody = typeof initResult === "string" ? initResult : initResult && initResult.markdown ? initResult.markdown : ""; | ||
| if (!addContent(buildStepSummaryDetailsSection("Initialization", "Show initialization details", initBody))) { |
There was a problem hiding this comment.
[/codebase-design] The new buffering approach silently drops the size-limit guard inside the reasoning/command accumulation loops. Previously, addContent was called per-entry and could abort early mid-stream. Now the entire reasoning body is buffered unconditionally before being passed to addContent — so a very large agent conversation will buffer the full text in memory before the size limit is checked.
💡 Suggestion
Add a soft cap on the intermediate buffers (e.g., stop appending to reasoningBody once it exceeds the tracker's remaining budget), or document that this is intentional because the sections are expected to stay small. Without this, the size-limit protection now only fires at section boundaries rather than within a section.
@copilot please address this.
| @@ -25,7 +25,7 @@ function parseCustomLog(logContent) { | |||
| if (claudeResult && claudeResult.logEntries && claudeResult.logEntries.length > 0) { | |||
There was a problem hiding this comment.
[/grill-with-docs] The section header is now ### Custom Engine Log (Claude format) — a plain ### heading outside any <details> block — while every other engine parser wraps its heading inside a <summary> tag. This creates a visible inconsistency in the step summary rendering.
💡 Suggestion
Wrap the prefixed label in buildStepSummaryDetailsSection too, passing claudeResult.markdown as the body:
return {
...claudeResult,
markdown: buildStepSummaryDetailsSection(
'Custom Engine Log (Claude format)',
'Show custom agent log',
claudeResult.markdown
),
};Without this, the custom-engine path will render a naked ### heading above collapsed <details> blocks, which breaks the uniform progressive-disclosure model this PR is establishing.
@copilot please address this.
| let markdown = "\n## 📊 Information\n\n"; | ||
| let markdown = ""; | ||
|
|
||
| if (!lastEntry) { |
There was a problem hiding this comment.
[/codebase-design] generateInformationSection accumulates markdown into a local let markdown = "" variable and then passes it to buildStepSummaryDetailsSection at the end. But inside the function this variable is also used as the early-return value when !lastEntry — that path now returns the full buildStepSummaryDetailsSection directly on line 297. The remaining code never resets markdown before it starts appending, so there is an accidental empty-string lead that is harmless now but fragile if lines are reordered.
💡 Suggestion
Rename the accumulator to body (matching the parameter name in buildStepSummaryDetailsSection) for clarity:
let body = '';
// ... append to body ...
return buildStepSummaryDetailsSection('Information', 'Show run metadata', body, { emptyBodyMessage: 'No information available.' });This makes it immediately clear what the variable's role is, reduces the chance of someone accidentally prepending the wrapper heading into the body, and stays consistent with the naming in generateConversationMarkdown (reasoningBody, commandsBody).
@copilot please address this.
| } | ||
|
|
||
| markdown += "## 🤖 Reasoning\n\n"; | ||
| markdown += "<details>\n<summary>Reasoning</summary>\n\n"; |
There was a problem hiding this comment.
[/codebase-design] parseCodexLog still opens a <details> block for Reasoning on line 529, then appends content line-by-line inside the loop below, and closes it with </details> on line 664. If an exception is thrown anywhere between those two points (e.g., from formatCodexBashCall), the </details> tag is never emitted and the GitHub step summary will render broken HTML.
💡 Suggestion
Buffer the reasoning section body into a local variable first — exactly as generateConversationMarkdown does with reasoningBody — then emit the whole section at once via buildStepSummaryDetailsSection. This makes the open/close pair atomic and impossible to split by an exception.
@copilot please address this.
| @@ -107,7 +107,7 @@ describe("parse_antigravity_log.cjs", () => { | |||
|
|
|||
There was a problem hiding this comment.
[/grill-with-docs] The test asserts expect(result.markdown).toContain("### Antigravity"), but buildStepSummaryDetailsSection renders <details><summary>Antigravity</summary> — it never emits a ### heading. Either the test is checking the wrong string (it will pass vacuously if the markdown happens to contain a ### for another reason) or ### Antigravity is still expected to appear somewhere outside the <details> block.
💡 Suggestion
Update the assertion to match what the helper actually generates:
expect(result.markdown).toContain('<summary>Antigravity</summary>');Consistent with how the other engine parser tests were updated in this same PR (e.g., parse_gemini_log.test.cjs).
@copilot please address this.
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 135 AIC · ⌖ 6.45 AIC · ⊞ 4.8K
| const result = parseAntigravityLog(logContent); | ||
|
|
||
| expect(result.markdown).toContain("## 🤖 Antigravity"); | ||
| expect(result.markdown).toContain("### Antigravity"); |
There was a problem hiding this comment.
Bug: test assertion will fail — wrong expected string.
The test expects ### Antigravity (a markdown heading), but buildStepSummaryDetailsSection wraps content in a <details><summary>Antigravity</summary> block. The function never emits a ### heading.
The assertion on line 131 has the same problem.
Should be:
expect(result.markdown).toContain('<summary>Antigravity</summary>');@copilot please address this.
| const summaryText = typeof title === "string" && title.trim() ? title : summary; | ||
| return `<details>\n<summary>${summaryText}</summary>\n\n${content}\n</details>\n\n`; | ||
| } | ||
|
|
There was a problem hiding this comment.
Duplication: local buildStepSummaryDetailsSection diverges from the shared version.
This file defines its own copy of buildStepSummaryDetailsSection (lines 98–105) that is missing the open and emptyBodyMessage options supported by the canonical version in log_parser_shared.cjs.
Two problems:
- Drift risk — any future change to the shared function won't automatically apply here.
- Silent behavior difference — the local version hard-codes
"No details available."and never passesopen, so any caller that passes options will silently get the wrong behaviour.
Consider importing and using the exported buildStepSummaryDetailsSection from log_parser_shared.cjs instead. If a circular-dep constraint forces a local copy, at minimum add the missing options parameter and keep signatures in sync.
@copilot please address this.
| } | ||
|
|
||
| if (!addContent("## 🤖 Commands and Tools\n\n")) { | ||
| if (sizeLimitReached) { |
There was a problem hiding this comment.
Correctness: redundant sizeLimitReached guard that can never be true here.
After the refactor, reasoningBody and commandDetailsBody are accumulated into strings before any addContent call, so sizeLimitReached can only become true inside the addContent(buildStepSummaryDetailsSection("Reasoning", ...)) call on line 177. If that call returns false, the function already returns early on line 179.
This means the if (sizeLimitReached) check on line 182 is unreachable — sizeLimitReached will always be false at this point (the early-return on line 179 handles the only way it can be set to true up to here).
The block can safely be removed to avoid misleading readers into thinking something can set sizeLimitReached between lines 179 and 182.
@copilot please address this.
| // Build markdown so the parser returns a truthy result and core.info has a | ||
| // readable fallback. The step summary itself is rendered from logEntries. | ||
| let markdown = "## 🤖 Reasoning\n\n"; | ||
| let markdown = "<details>\n<summary>Reasoning</summary>\n\n"; |
There was a problem hiding this comment.
Inconsistency: raw <details> string concatenation instead of using the new helper.
This file builds <details>/</details> blocks via raw string literals (e.g. lines 417, 425, 435, 519, 529, 664, 791) while the stated goal of this PR is to centralise that pattern inside buildStepSummaryDetailsSection. Not using the helper here means:
- Sections can go out of sync with the helper's format (e.g. missing blank line after
<summary>, mismatched</details>). - The unclosed-section risk is real — if a section's logic returns early (e.g. before the matching
</details>), the HTML will be malformed.
Consider replacing these concatenation segments with calls to buildStepSummaryDetailsSection like the other parsers do.
@copilot please address this.
| const errorInfo = extractCodexErrorMessages(lines); | ||
| if (errorInfo.hasErrors) { | ||
| markdown += "## ⚠️ Errors\n\n"; | ||
| markdown += "<details>\n<summary>Errors</summary>\n\n"; |
There was a problem hiding this comment.
Potential unclosed <details> block when an error with reconnects exists.
The Errors section opens a <details> block on line 519 but the closing </details> on line 526 is inside the same conditional. That part is fine. However, the subsequent markdown += "<details>\n<summary>Reasoning</summary>\n\n"; on line 529 is unconditional and starts a <details> block that is only closed 135 lines later on line 664. If any early-return path between those lines is ever introduced, or if the loop accumulation is refactored, the block will be left unclosed.
More immediately, because the Reasoning <details> open is a bare string append without a corresponding helper, any whitespace/formatting inconsistency will silently diverge from the sections managed by buildStepSummaryDetailsSection.
@copilot please address this.
| return { | ||
| ...claudeResult, | ||
| markdown: "## Custom Engine Log (Claude format)\n\n" + claudeResult.markdown, | ||
| markdown: `### Custom Engine Log (Claude format)\n\n${claudeResult.markdown}`, |
There was a problem hiding this comment.
Inconsistency: wrapper heading uses ### directly instead of the new collapsible pattern.
Lines 28 and 44 prepend a raw ### Custom Engine Log (Claude format) / ### Custom Engine Log (Codex format) heading before the inner parser's markdown. The inner parsers now produce <details><summary>...</summary> blocks, so the outer ### heading creates a visible section title that is inconsistent with the rest of the step summary (which uses only collapsible sections, no bare headings).
Consider wrapping the combined output in buildStepSummaryDetailsSection as the fallback path already does (line 59), or at least use a <details> block here too so the custom-engine prefix stays visually consistent.
@copilot please address this.
|
@copilot please run the
|
Addressed both remaining reviewer threads in local commits (push blocked by
Budget tracking regression (
Local validation: |
|
@copilot please run the
|
…DetailsSection parity Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Both unresolved reviewer threads addressed in commits
Budget tracking regression — added Local: |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/28764674525
|
PR-finisher run complete.
Hand-off: push is blocked by the |
|
/souschef dismiss reviews |
|
✅ PR Sous Chef finished PR sous-chef nudges. |
|
Sous-chef was invoked and will triage this PR.
|
| * @param {{open?: boolean, emptyBodyMessage?: string}} [options] | ||
| * @returns {string} | ||
| */ | ||
| function buildStepSummaryDetailsSection(title, body, options = {}) { |
There was a problem hiding this comment.
@copilot there's multiple copies of this function. refactor into helper file.
There was a problem hiding this comment.
Done in the latest commit. Extracted buildStepSummaryDetailsSection into a new standalone log_parser_step_summary_builder.cjs file. Both log_parser_format.cjs and log_parser_shared.cjs now import from it, eliminating the duplication and resolving the circular-dependency constraint that previously prevented sharing the implementation.
…er file Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Step summaries were mixing emoji-heavy section titles with inconsistent heading levels and partially-expanded content, which made the report noisy and harder to scan. This change standardizes the summary surface so top-level sections read cleanly, and verbose content stays behind explicit disclosure.
Summary structure
###headingsProgressive disclosure
Initialization,Reasoning,Commands and Tools,Information) in<details>blocks with concise<summary>labelsParser/report alignment
Example of the new summary shape: