Render grader values in collapsible step summary - #55823
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds collapsible grader summaries with textual statuses, computed values, and sanitized table cells.
Changes:
- Adds grader summary rendering and progressive disclosure.
- Adds tests for values, statuses, and escaping.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/trace_graders.cjs |
Builds and renders the collapsible grader table. |
actions/setup/js/trace_graders.test.cjs |
Tests summary values and sanitization. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
| ...rows, | ||
| ]); | ||
| } | ||
| core.summary.addDetails("Graders", buildGradersSummaryBody(results)); |
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has 0 new lines of code in business logic directories (default_business_additions=0).
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This summary rewrite introduces two rendering/correctness regressions in the changed lines: the <details> body is emitted in a form GitHub won't render as a table, and the new table-cell escaping is still bypassable by backslash-prefixed pipes.
Blocking themes
- The step summary markup is malformed for GitHub's Markdown parser, so grader results lose their tabular rendering.
- The new sanitizer does not fully neutralize untrusted table-cell content, so crafted grader metadata can still break the table layout.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 7.34 AIC · ⌖ 7 AIC · ⊞ 7K
Comment /review to run again
| ...rows, | ||
| ]); | ||
| } | ||
| core.summary.addDetails("Graders", buildGradersSummaryBody(results)); |
There was a problem hiding this comment.
This summary table is still broken in the step summary: @actions/core's addDetails() does not insert the blank lines GitHub-flavored Markdown needs before a table, so the rows render as literal text instead of a table and the closing </details> can end up glued to the last row.
💡 Why this matters
The previous addTable() implementation produced a valid table automatically. This replacement regresses the UI for every run, which defeats the purpose of surfacing grader results in a readable summary.
A safe fix is to wrap the generated body with blank lines, or build the details block manually:
core.summary
.addRaw("<details><summary>Graders</summary>\n\n")
.addRaw(buildGradersSummaryBody(results))
.addRaw("\n\n</details>\n");Alternatively, make buildGradersSummaryBody() return leading/trailing blank lines and add a regression test around the final summary markup.
| function sanitizeSummaryText(value) { | ||
| return String(value ?? "") | ||
| .replace(/\r?\n/g, " ") | ||
| .replace(/\|/g, "\\|") |
There was a problem hiding this comment.
The Markdown escaping here is incomplete: if a cell already contains a backslash before a pipe (for example A\\|B), this replacement turns it into A\\\\|B, which GitHub Markdown reduces back to an escaped backslash plus a real column separator. That lets untrusted grader data still split the table and corrupt the summary.
💡 Why this matters
This helper is meant to harden untrusted grader names/sources/units before putting them into a Markdown table. Right now it only handles the simple | case, so crafted input can still break the table layout.
Escape backslashes before pipes (or avoid Markdown tables entirely for untrusted content), and add a regression test for an input like "A\\|B":
return String(value ?? "")
.replace(/\\/g, "\\\\")
.replace(/\|/g, "\\|")
.replace((r/redacted)?\n/g, " ")
...
There was a problem hiding this comment.
Impeccable Review — critique + audit
The refactor to wrap graders in a collapsible <details> block is a good UX improvement that reduces noise in the step summary. Two existing comments already flag the blocking issues; one additional observation below.
Summary of findings
| Severity | Issue | Status |
|---|---|---|
| 🔴 Blocking | addDetails renders raw markdown inside <details> — table may not render correctly without surrounding blank lines |
Existing comment (line 838) |
| 🔴 Blocking | sanitizeSummaryText escapes | without first escaping existing backslashes — bypass possible |
Existing comment (line 463) |
| 🟡 Non-blocking | statusLabels["error"] and statusLabels["unavailable"] are dead keys — they can never appear in the output |
New (see below) |
Dead keys in statusLabels
buildGradersSummaryBody filters with hasComputedValue, keeping only results where typeof result.value === "number". In practice, "error" and "unavailable" results carry value: null, so "Error" and "Unavailable" labels are unreachable. The fallback || "Unknown" would trigger first for any future unknown status. Consider removing those dead keys or adding a comment documenting the assumption.
The two blocking issues in the existing comments should be resolved before merging.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 20.5 AIC · ⌖ 9.14 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — two prior comments (from Copilot) already cover the main bugs; one new comment added for test coverage.
📋 Key Themes & Highlights
Findings
- Escaping order bug (line 463, existing comment):
|is escaped before<>&, so a cell containing\|still leaks. Fix: escape backslashes first, or escape in a single pass. addDetailsblank-line requirement (line 838, existing comment): GitHub renders inline HTML table as literal text without blank lines around the content string. Add\n\nbefore and after the table inbuildGradersSummaryBody.- Missing empty-results test (new comment): the
"No grader values available."branch has no test.
Positive Highlights
- ✅
hasComputedValuetype guard is a clean, reusable predicate. - ✅
buildGradersSummaryBodyis properly unit-tested and exported. - ✅ Textual status labels (
Pass/Fail/Error) are more accessible than emoji-only indicators.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 25.1 AIC · ⌖ 9.93 AIC · ⊞ 7.6K
Comment /matt to run again
| expect(summary).toContain("Custom \\| <grader>"); | ||
| expect(summary).toContain("inline\\|source"); | ||
| expect(summary).toContain("unit\\|&"); | ||
| }); |
There was a problem hiding this comment.
[/tdd] Missing test for the empty-results branch — buildGradersSummaryBody([]) should return "No grader values available.", but there's no assertion covering that path.
💡 Suggested test
it("returns a message when no grader has a computed value", () => {
const summary = buildGradersSummaryBody([
{ id: "no-value", name: "No value", value: null, unit: "", status: "unavailable", source: "builtin" },
]);
expect(summary).toBe("No grader values available.");
});Without this test the fallback message can silently regress.
@copilot please address this.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the pr-finisher pass. Addressed both open review threads in
Local validation: |
|
🎉 This pull request is included in a new release. Release: |
Graders now use progressive disclosure in the step summary and display every computed value, including built-in grader results, without emoji status indicators.
Summary rendering
<details>section.Pass,Fail, andError.Value coverage
Safety