compiler: render evals results as progressive disclosure details section in step summary - #45661
Conversation
- Create render_evals_summary.cjs: reads redacted evals.jsonl and renders BinEval results as a <details> progressive disclosure section using buildStepSummaryDetailsSection (same pattern as observability/AWF reflect) - Update run_evals.cjs parseMain(): remove inline addHeading/addTable summary writing; rendering is now handled by the dedicated render step that runs after secret redaction so the published summary is always clean - Add buildRenderEvalsSummaryStep() in evals_steps.go: generates the Actions step that calls render_evals_summary.cjs with if: always() + continue-on-error - Update buildEvalsJobSteps(): insert render step after redact (step 10) and before upload (step 11) - Add TestBuildEvalsJobStepsRenderSummary: verifies step presence and ordering Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Renders evals results as progressive disclosure in step summary. Draft PR, good description. Batch with similar compiler output PRs for review.
|
There was a problem hiding this comment.
Pull request overview
Moves BinEval summary rendering to a dedicated post-redaction, collapsible step.
Changes:
- Removes inline summary rendering from eval parsing.
- Adds a progressive-disclosure renderer and compiler step.
- Tests redact → render → upload ordering.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/evals_steps.go |
Adds the summary-rendering step. |
pkg/workflow/evals_steps_test.go |
Tests step presence and ordering. |
actions/setup/js/run_evals.cjs |
Removes inline summary output. |
actions/setup/js/render_evals_summary.cjs |
Parses redacted results and renders the details section. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Medium
|
|
||
| steps := []string{ | ||
| " - name: Render evals results to step summary\n", | ||
| " if: always()\n", |
| const model = results[0]?.model; | ||
| if (model) { | ||
| lines.push(`**model**: ${escapeMarkdownCell(model)}`); |
| * @param {Array<{id: string, question: string, answer: string, model: string, timestamp: string}>} results | ||
| * @returns {string} | ||
| */ | ||
| function buildEvalsBody(results) { |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #45661 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (78 additions). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
Run: https://github.com/github/gh-aw/actions/runs/29397228615
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
| Test | File | Classification | Issues | Verdict
Build tag References: §29396120473 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The PR correctly moves step-summary rendering to after secret redaction and uses the existing buildStepSummaryDetailsSection pattern. Three issues are worth addressing:
-
always()condition on render step (existing comment, line 343evals_steps.go) — if redaction fails, the published summary could still contain secrets. Considerif: success()or check that the redact step succeeded. -
Model source ambiguity (existing comment, line 88
render_evals_summary.cjs) —results[0].modelmay reflect a different model than what actually ran the evals. -
No JS unit tests for
render_evals_summary.cjs(existing comment, line 67) — the rendering logic (escaping, tallies, empty/malformed JSONL) has no test coverage.
Additional note: EVALS_OUTPUT_PATH is duplicated between run_evals.cjs (line 40) and render_evals_summary.cjs (line 20). Extract it to a shared constants module to prevent drift.
Overall the approach is sound; the always() + redaction-failure scenario is the most actionable concern.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 14.5 AIC · ⌖ 4.35 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — no blocking issues, but the existing Copilot comments plus one new finding are worth addressing before merge.
📋 Key Themes & Highlights
Key Themes
- Duplicated constant:
EVALS_OUTPUT_PATHis defined identically in bothrun_evals.cjsand the newrender_evals_summary.cjs— extract to a shared module. always()+continue-on-error: The render step runs even when redaction failed (see existing Copilot comment onevals_steps.go:343).- Missing JS tests:
render_evals_summary.cjshas no unit tests for JSONL parsing or edge cases (see existing comment on line 67). - Model attribution ambiguity:
results[0].modelmay not reflect the actual eval model (see existing comment on line 88).
Positive Highlights
- ✅ Clean extraction into a dedicated post-redaction step.
- ✅
escapeMarkdownCellprotects table cells from pipe injection. - ✅ Test verifies
redact → render → uploadordering. - ✅ PR description accurately describes every changed file.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 31.5 AIC · ⌖ 4.46 AIC · ⊞ 6.7K
Comment /matt to run again
| const fs = require("fs"); | ||
| const { buildStepSummaryDetailsSection } = require("./log_parser_step_summary_builder.cjs"); | ||
|
|
||
| const EVALS_OUTPUT_PATH = "/tmp/gh-aw/evals.jsonl"; |
There was a problem hiding this comment.
[/codebase-design] EVALS_OUTPUT_PATH is duplicated — it is already defined in run_evals.cjs (line 40). Two sources of truth for the same path will drift.
💡 Suggestion
Extract the constant to a shared file (e.g. evals_constants.cjs) and require it from both modules:
// evals_constants.cjs
module.exports = { EVALS_OUTPUT_PATH: "/tmp/gh-aw/evals.jsonl" };This way a path change only needs to happen in one place.
@copilot please address this.
There was a problem hiding this comment.
Review summary
Two low-to-medium issues found; none are blockers, but both are worth fixing before this ships.
Findings
Answer case normalisation (medium) — record.answer is coerced to a string but never uppercased/trimmed. A value like "yes" silently falls into the UNKNOWN bucket and the YES/NO tallies undercount. One-liner fix: add .toUpperCase().trim() at coercion time.
escapeMarkdownCell incomplete (low) — \r characters and backticks are not escaped, which can produce broken table rows or unintended code spans in the step summary.
Pre-existing comments (#3585200129, #3585200160, #3585200178) on the always() redaction-ordering risk, model label accuracy, and missing JS tests remain valid and are not duplicated here.
🔎 Code quality review by PR Code Quality Reviewer · 28 AIC · ⌖ 4.52 AIC · ⊞ 5.6K
Comment /review to run again
| }); | ||
| } | ||
| } catch { | ||
| // skip malformed lines |
There was a problem hiding this comment.
Silent mis-tallying when answer is not uppercase: answers are coerced with String(record.answer ?? 'UNKNOWN') but never normalised, so "yes" or "Yes" will not match the === 'YES' checks, will render as ❓ UNKNOWN in the table, and will contribute nothing to yesCount.
💡 Suggested fix
Apply .toUpperCase().trim() at coercion time so that any capitalisation variant is counted correctly:
answer: String(record.answer ?? 'UNKNOWN').toUpperCase().trim(),The writer in run_evals.cjs always produces uppercase today, but normalising defensively here means the renderer is not silently broken if that ever changes or if the JSONL is produced by a different tool.
| /** | ||
| * Main entry point: reads evals results and writes the step summary section. | ||
| * @returns {Promise<void>} | ||
| */ |
There was a problem hiding this comment.
escapeMarkdownCell does not escape \r or backticks: carriage returns pass through unchanged, and backtick sequences in questions/IDs can produce unintended code spans in the rendered table.
💡 Suggested fix
function escapeMarkdownCell(text) {
return text
.replace(/[\r\n]/g, " ")
.replace(/\|/g, "\\|")
.replace(/`/g, "\\`");
}The original .replace(/\n/g, " ") leaves \r\n endings with a trailing \r, which some Markdown renderers treat inconsistently.
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>
Addressed in The follow-up fixes now fail closed on eval redaction before rendering/upload, record the resolved execution model in eval results, add Local validation passed with |
| uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 | ||
| with: | ||
| script: | | ||
| const { setupGlobals } = require('${{ runner.temp }}/gh-aw/actions/setup_globals.cjs'); |
There was a problem hiding this comment.
@copilot refactor JavaScript into file in actions/setup/js
There was a problem hiding this comment.
Addressed in 722dd63 (refactor into actions/setup/js/redact_evals_results.cjs) with follow-up coverage in 7d69971.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Score breakdown: Impact 30 + Urgency 13 + Quality 15 Rationale: Adds progressive disclosure rendering for evals results in step summaries — improves UX consistency with other summary sections. Adds new
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| setupGlobals(core, github, context, exec, io, getOctokit); | ||
| const { main } = require('${{ runner.temp }}/gh-aw/actions/redact_evals_results.cjs'); | ||
| await main(); | ||
| } |
There was a problem hiding this comment.
Fixed in 0f8dbb6 by removing the stray } from the eval redaction step generator and recompiling the affected .lock.yml files.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Run: https://github.com/github/gh-aw/actions/runs/29420425558
|
Current HEAD I rechecked the unresolved threads against the current files and reran local validation on this HEAD: GitHub checks on the agent HEAD still need a maintainer re-trigger if you want fresh remote status, since agent pushes do not start workflows. |
|
🎉 This pull request is included in a new release. Release: |
The evals job wrote BinEval results directly to the step summary as a flat heading+table during the parse phase — before secret redaction, and inconsistent with how other sections (token usage, AWF reflect, observability) are rendered.
Changes
render_evals_summary.cjs(new) — reads the redactedevals.jsonland renders a<details>collapsible section viabuildStepSummaryDetailsSection, matching the pattern of other summary sections. Shows ID/Question/Answer (✅/❌/❓), YES/NO/UNKNOWN tallies, and model name.run_evals.cjs— removes inlineaddHeading/addTablefromparseMain(); summary rendering is now delegated to the dedicated step that runs post-redaction.evals_steps.go— addsbuildRenderEvalsSummaryStep()and inserts it between the existing redact step and the upload step inbuildEvalsJobSteps():evals_steps_test.go— addsTestBuildEvalsJobStepsRenderSummaryverifying step presence,render_evals_summary.cjsreference, and redact→render→upload ordering.