Render WSRF in step-summary token usage with progressive disclosure - #54591
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based 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.
|
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
The new WSRF summary wiring introduces a markdown-structure bug in the step summary: it nests a <details> block inside the existing token-usage <details>, which GitHub does not render reliably.
Blocking theme
- The progressive-disclosure UI is implemented with nested
<details>, so the new summary can render malformed or lose its collapsed behavior depending on the GitHub surface.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 4.71 AIC · ⌖ 6.88 AIC · ⊞ 7K
Comment /review to run again
|
|
||
| return [ | ||
| "<details>", | ||
| `<summary>Working-Set Rebuild Factor (WSRF): ${displayFactor} (${measurementState})</summary>`, |
There was a problem hiding this comment.
This nests a <details> block inside another <details> block in the step summary, and GitHub’s markdown renderer does not handle nested disclosure widgets reliably, so the WSRF section can render as broken or permanently expanded instead of being progressively hidden.
💡 Why this blocks
The outer token-usage section is already a <details> element. buildWorkingSetDetailsSection() now injects a second <details> inside it, which is exactly the kind of nesting GitHub markdown tends to flatten or render inconsistently across summary surfaces. That means the new UX goal—“collapsed WSRF details inside collapsed token usage”—is not actually guaranteed, and in the worst case the summary becomes malformed.
Use a non-nested disclosure pattern here instead, e.g. keep the outer <details> and render the WSRF line plus bullets directly inside it, or split WSRF into a sibling section rather than a child <details>.
return [
`**Working-Set Rebuild Factor (WSRF):** ${displayFactor} (${measurementState})`,
"",
`- State: \`${measurementState}\``,
...
].join("\n");That preserves valid markdown structure and avoids renderer-dependent breakage.
There was a problem hiding this comment.
The implementation is clean and correct. buildWorkingSetDetailsSection guards every numeric field with Number.isFinite, handles null/non-object input, and the new tests cover both the measured and unavailable states. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 28.2 AIC · ⌖ 8.76 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Ponytail review (over-engineering only): one small shrink opportunity noted inline (repeated formatting ternaries could be extracted into a helper). No other bloat found.
net: -3 lines possible.
Generated by ✂️ Ponytail Reviewer for #54591 · auto · 17.8 AIC · ⌖ 4.47 AIC · ⊞ 7.3K
Comment /ponytail to run again
Comments that could not be inline-anchored
actions/setup/js/parse_token_usage.cjs:19
shrink: four repeated Number.isFinite(x) ? x.toLocaleString() : "0" ternaries. Extract const fmt = n => Number.isFinite(n) ? n.toLocaleString() : "0"; and call it for each field, ~4 lines shorter.
There was a problem hiding this comment.
Pull request overview
Integrates WSRF metrics into the existing token-usage step summary using progressive disclosure.
Changes:
- Computes WSRF from deduplicated token-usage records.
- Renders measured and unavailable metric details.
- Adds renderer and integration tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/parse_token_usage.cjs |
Computes and renders WSRF in the step summary. |
actions/setup/js/parse_token_usage.test.cjs |
Tests WSRF rendering and summary integration. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| "", | ||
| ].join("\n"); |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /diagnosing-bugs, and /codebase-design — requesting changes on a correctness risk and minor test gaps.
📋 Key Themes & Highlights
Key Themes
- Correctness risk:
calculateWorkingSetFromJSONLis called outside themarkdown.length > 0guard; a throw there will silently abort the step-summary write. (See inline comment on line 202.) - Test coverage gaps:
"partial"measurement state andnull/undefinedinputs forbuildWorkingSetDetailsSectionare unexercised by the new tests. - UI ordering: WSRF details are placed before the token table, reversing the information hierarchy implied by the PR description.
Positive Highlights
- ✅ Clean extraction of
buildWorkingSetDetailsSection— composable, independently testable, and null-safe. - ✅
Number.isFiniteguards on all numeric fields prevent NaN/Infinity leaking into output. - ✅
toLocaleString()for token counts is a nice ergonomic touch. - ✅ Two meaningful unit tests (measured + unavailable) plus an integration assertion were added with the feature — good discipline.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 36.2 AIC · ⌖ 10.2 AIC · ⊞ 7.8K
Comment /matt to run again
| @@ -168,9 +202,10 @@ async function main() { | |||
| return; | |||
There was a problem hiding this comment.
[/diagnosing-bugs] calculateWorkingSetFromJSONL is called before the if (markdown.length > 0) guard — if it throws, the token-usage summary is silently lost.
💡 Suggested fix
Move the call inside the guard so a WSRF failure cannot suppress the token table:
const markdown = generateTokenUsageSummary(summary);
if (markdown.length > 0) {
const workingSet = calculateWorkingSetFromJSONL(content).workingSet;
core.info(renderTokenTableAsPlainText(getSummaryTitle(), markdown));
await appendStepSummarySection(getSummaryTitle(), markdown, workingSet);
}Alternatively, wrap the call in a try/catch and fall back to null so WSRF failures are non-fatal.
@copilot please address this.
| }); | ||
|
|
||
| test("renderTokenTableAsPlainText strips table separator lines and pipes", () => { | ||
| const markdown = ["| # | Alias | Input | Output |", "|--:|-------|------:|-------:|", "| 1 | sonnet46 | 100 | 200 |", "| **Total** | | **100** | **200** |", "", "Legend: `Alias` is the model shorthand.", ""].join("\n"); |
There was a problem hiding this comment.
[/tdd] The "partial" measurement state is mentioned in the PR body but has no dedicated test. Both buildWorkingSetDetailsSection tests cover "measured" and "unavailable"; a "partial" case would complete the contract.
💡 Suggested test
test("buildWorkingSetDetailsSection renders partial state", () => {
const section = buildWorkingSetDetailsSection({
measurement_state: "partial",
rebuild_factor: 2.0,
cumulative_input_tokens: 200000,
peak_input_tokens: 100000,
rebuild_excess_tokens: 100000,
invocations: 2,
});
expect(section).toContain("Working-Set Rebuild Factor (WSRF): 2.00× (partial)");
expect(section).toContain("- State: `partial`");
});@copilot please address this.
| @@ -549,6 +552,37 @@ describe("parse_token_usage", () => { | |||
| expect(section).toContain("Per-request AI credits and token totals"); | |||
There was a problem hiding this comment.
[/tdd] buildWorkingSetDetailsSection(null) and buildWorkingSetDetailsSection(undefined) are not explicitly tested — only the guard's early-return (return "") is implied. A test for falsy input would pin the contract.
💡 Suggested test
test("buildWorkingSetDetailsSection returns empty string for null or undefined", () => {
expect(buildWorkingSetDetailsSection(null)).toBe("");
expect(buildWorkingSetDetailsSection(undefined)).toBe("");
expect(buildWorkingSetDetailsSection("string")).toBe("");
});@copilot please address this.
| function buildStepSummarySection(title, markdown) { | ||
| return `<details>\n<summary>${title}</summary>\n\nPer-request AI credits and token totals\n\n${markdown}</details>\n\n`; | ||
| function buildStepSummarySection(title, markdown, workingSet = null) { | ||
| const workingSetSection = buildWorkingSetDetailsSection(workingSet); |
There was a problem hiding this comment.
[/codebase-design] The WSRF details block is rendered before the token table (${workingSetSection}${markdown}), but the PR description says "WSRF visible at a glance alongside the token table." Placing WSRF above the table inverts the information hierarchy — users land on WSRF first, then scroll to credits/totals.
Consider reversing the order (${markdown}${workingSetSection}) or adding a blank line between them to keep the token table as the primary surface.
@copilot please address this.
Run: https://github.com/github/gh-aw/actions/runs/32515777962
|
|
🎉 This pull request is included in a new release. Release: |
This updates step-summary reporting to surface Working-Set Rebuild Factor (WSRF) directly in the existing token-usage section. WSRF is now visible at a glance, with detailed metric breakdown collapsed behind
<details>/<summary>.Summary UI integration
parse_token_usage.cjsstep-summary composition to include WSRF in the same token-usage block (no separate report path).Progressive disclosure for WSRF
measured/partial/unavailable)N.NN×when present)Computation wiring
calculateWorkingSetFromJSONL) and fed its output into the step-summary renderer, keeping WSRF semantics aligned with existing usage-activity logic.