Render token table to core.info in parse_token_usage step#40227
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR makes the token usage table visible in the raw GitHub Actions step logs by rendering the markdown token-usage table into a more log-friendly plain-text form and emitting it via core.info, in addition to the existing step summary output.
Changes:
- Added
renderTokenTableAsPlainText(title, markdown)to convert the generated markdown table into log-friendly text. - Updated
main()inparse_token_usage.cjstocore.info()the rendered table before writing the step summary. - Added unit tests for the plain-text renderer and extended the
main()integration test to assertcore.inforeceives the table content.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/parse_token_usage.cjs | Adds a plain-text renderer and logs the token usage table via core.info during parsing. |
| actions/setup/js/parse_token_usage.test.cjs | Adds targeted tests for the renderer and updates main() integration expectations around core.info. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
| function renderTokenTableAsPlainText(title, markdown) { | ||
| const plainText = markdown | ||
| .replace(/^\|-+.*-+\|$/gm, "") // Remove table separator lines | ||
| .replace(/^\|/gm, "") // Remove leading pipe from table rows | ||
| .replace(/\|$/gm, "") // Remove trailing pipe from table rows | ||
| .replace(/\s*\|\s*/g, " | ") // Normalize remaining pipes to spaced separators | ||
| .replace(/\*\*(.*?)\*\*/g, "$1") // Remove bold markers | ||
| .replace(/\n{3,}/g, "\n\n") // Collapse excess blank lines | ||
| .trim(); | ||
| return `${title}\n\n${plainText}`; | ||
| } |
| expect(result).toContain("Token Usage"); | ||
| // separator line is removed | ||
| expect(result).not.toMatch(/\|--/); | ||
| // leading/trailing pipes are stripped | ||
| expect(result).not.toMatch(/^\|/m); |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40227 does not have the 'implementation' label and has 0 new lines of code in business logic directories (default_business_additions=0, threshold=100). Neither enforcement condition is met. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics & Test Classification (3 tests analyzed)
Go: 0 ( 📝 Test Notes
Missing edge cases — Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — requesting changes for one confirmed correctness bug plus test coverage gaps that mask it.
📋 Key Themes & Findings
❌ Confirmed Bug — Separator Regex (line 115)
The regex /^\|-+.*-+\|$/gm requires dashes immediately before the closing |, so it silently skips the production separator row:
|--:|-------|------:|-------:|-----------:|...
Every right-aligned column (---:|) ends with :|, not -|. The separator row therefore survives the strip step and lands in the step log as a garbage line like --: | ------- | ------: | -------:. Verifiable with:
node -e "const s='|--:|-------|------:|-------:|'; console.log(/^\\|-+.*-+\\|$/.test(s))" // falseSuggested fix: /^\|(?:[-:\s]+\|)+$/gm — matches any line whose pipe-delimited cells contain only -, :, and spaces.
⚠️ Test Coverage Gaps
- False-positive assertion (line 551):
not.toMatch(/\|--/)passes with the broken regex because the leading|was already stripped before this check runs. - Over-loose integration matchers (lines 199-200):
stringContaining("Alias")would pass even if the function was a no-op. - Missing production-format test: adding
expect(result).not.toContain("---")would immediately catch the separator bug.
✅ Positive Highlights
- Clean, focused, purely additive change — well-scoped to the rendering concern.
- Good JSDoc on the new function.
- The regex pipeline approach and
core.infousage is exactly right; only the first regex needs a fix. - Exporting the function for unit testing is a good practice.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 73.9 AIC · ⌖ 14.6 AIC · ⊞ 6.9K
| */ | ||
| function renderTokenTableAsPlainText(title, markdown) { | ||
| const plainText = markdown | ||
| .replace(/^\|-+.*-+\|$/gm, "") // Remove table separator lines |
There was a problem hiding this comment.
[/diagnose] Bug: this regex does not remove separator rows that use column-alignment notation (:---, ---:, :---:), which is exactly what the production table uses.
The production separator in parse_mcp_gateway_log.cjs is:
|--:|-------|------:|-------:|-----------:|...
The pattern ^\|-+.*-+\|$ requires dashes immediately before the closing |, but every right-aligned or center-aligned column ends with :| — so the separator survives stripping and appears verbatim in the step log.
💡 Suggested fix
Match any line whose cells contain only -, :, and spaces:
.replace(/^\|[-:\s|]+\|$/gm, "") // Remove table separator lines (including aligned columns)or more explicitly:
.replace(/^\|(?:[-:\s]+\|)+$/gm, "") // Remove table separator linesA quick verification:
node -e "
const sep = '|--:|-------|------:|-------:|';
console.log(/^\|(?:[-:\s]+\|)+$/.test(sep)); // true
"|
|
||
| expect(result).toContain("Token Usage"); | ||
| // separator line is removed | ||
| expect(result).not.toMatch(/\|--/); |
There was a problem hiding this comment.
[/tdd] This assertion gives a false positive — it passes even when the separator is NOT removed.
After .replace(/^\|/gm, "") strips the leading pipe, the separator row |--:|-------|------:|-------:| becomes --:|-------|------:|-------:|, which no longer matches /\|--/. The separator content is still present in the output; it just lost its leading |.
This means the test green-lights a buggy implementation (the separator regex above misses aligned columns).
💡 Stronger assertion
// Separator dashes should be completely absent (not just moved)
expect(result).not.toMatch(/^\s*--/m); // no line starting with dashes
// OR more precisely:
expect(result.split("\n")).not.toContain(expect.stringMatching(/^[-:\s|]+$/));| }); | ||
|
|
||
| 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 unit test uses the same separator format (|--:| ends with :|) that proves the production bug — but the test’s own assertions are not tight enough to catch the failure (see comment on line 551).
Also worth adding a test using the exact separator from parse_mcp_gateway_log.cjs line 172 to lock in the real-world contract:
💡 Suggested additional test input
const productionSeparator = "|--:|-------|------:|-------:|-----------:|------------:|-------------:|-----------:|---------:|";
const result = renderTokenTableAsPlainText("T", `| # |\n${productionSeparator}\n| 1 |`);
expect(result).not.toContain("---"); // separator fully goneThis would have caught the bug immediately.
| expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Token usage summary appended")); | ||
| // Token table should also be rendered to core.info | ||
| expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Token Usage")); | ||
| expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Alias")); |
There was a problem hiding this comment.
[/tdd] These matchers are too loose to verify the plain-text rendering actually works. stringContaining("Token Usage") and stringContaining("Alias") would both pass even if renderTokenTableAsPlainText returned the raw markdown unmodified (since | Alias | contains "Alias").
💡 Suggested stronger assertion
Add an assertion that proves markdown was stripped:
// Verify the output is plain text, not markdown
expect(mockCore.info).not.toHaveBeenCalledWith(expect.stringContaining("**"));
expect(mockCore.info).not.toHaveBeenCalledWith(expect.stringMatching(/^\|/m));| } | ||
| const markdown = generateTokenUsageSummary(summary); | ||
| if (markdown.length > 0) { | ||
| core.info(renderTokenTableAsPlainText(getSummaryTitle(), markdown)); |
There was a problem hiding this comment.
[/tdd] Minor: calling getSummaryTitle() twice (here and in appendStepSummarySection) is harmless, but if the env var ever becomes expensive to read, this could be extracted into a local variable once.
const title = getSummaryTitle();
core.info(renderTokenTableAsPlainText(title, markdown));
await appendStepSummarySection(title, markdown);This also makes the intent clearer — same title for both the log and the summary.
There was a problem hiding this comment.
REQUEST_CHANGES — two distinct correctness bugs must be fixed before merging.
Blocking issues
Separator regex never fires (high)
The regex /^\|-+.*-+\|$/gm on line 115 requires the separator line to end in one or more dashes followed by |. Every separator produced by generateTokenUsageSummary ends in :| (alignment marker), e.g.:
|--:|-------|------:|-------:|-----------:|------------:|-------------:|-----------:|---------:|
So the separator is never removed — it just gets its outer pipes stripped by steps 2–3, becoming --:|-------|...: noise visible in the raw log. Existing inline comment on L123 covers this; the fix needs to land.
Global pipe normalization corrupts non-table content (medium)
See inline comment on line 118 for the full analysis and a suggested row-scoped rewrite that also solves the separator regex in one pass.
Test blind spot
not.toMatch(/\|--/) at L551 passes even when the separator survives (the leading | is stripped by steps 2–3). The test gives false confidence that separator removal works. Existing inline comment on L551 covers this.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 133.3 AIC · ⌖ 9.68 AIC · ⊞ 5.1K
| .replace(/^\|-+.*-+\|$/gm, "") // Remove table separator lines | ||
| .replace(/^\|/gm, "") // Remove leading pipe from table rows | ||
| .replace(/\|$/gm, "") // Remove trailing pipe from table rows | ||
| .replace(/\s*\|\s*/g, " | ") // Normalize remaining pipes to spaced separators |
There was a problem hiding this comment.
Global pipe normalization is not scoped to table rows — it silently corrupts any | in legend text, inline code, or future non-table content.
💡 Details and suggested fix
After steps 2–3 strip only the outermost | from each table row, step 4's .replace(/\s*\|\s*/g, " | ") runs on the entire markdown string — including the Legend section and any inline code. If the legend (or any future content produced by generateTokenUsageSummary) ever contains a | character (e.g. Bitwise OR: \a | b``) it will be silently reformatted with no test catching the regression.
A safer approach scopes the transformation to table rows only, and also naturally fixes the separator regex bug (line 115) — a filter like /^\|[\|:\-\s]+\|$/ reliably matches any GFM alignment separator regardless of whether colons are present:
function renderTokenTableAsPlainText(title, markdown) {
const plainText = markdown
.split("\n")
.filter(line => !/^\|[\|:\-\s]+\|$/.test(line.trim())) // drop GFM separator rows reliably
.map(line =>
line.startsWith("|") && line.endsWith("|")
? line.slice(1, -1).replace(/\s*\|\s*/g, " | ") // normalize only table rows
: line
)
.join("\n")
.replace(/\*\*(.*?)\*\*/g, "$1")
.replace(/\n{3,}/g, "\n\n")
.trim();
return `${title}\n\n${plainText}`;
}This also eliminates the need for three separate chained .replace() calls with global regexes over the whole string.
| }); | ||
|
|
||
| test("renderTokenTableAsPlainText prefixes output with title", () => { | ||
| const result = renderTokenTableAsPlainText("My Token Usage", "| A |\n|---|\n| 1 |"); |
There was a problem hiding this comment.
The |---| separator in this fixture accidentally exercises the broken regex path — the test doesn't prove the function handles real alignment-decorated separators.
💡 Details
|---| ends with -|, so it does match the current (buggy) regex /^\|-+.*-+\|$/gm. This means renderTokenTableAsPlainText prefixes output with title silently passes even with the broken separator regex, creating the illusion of correctness.
Every real separator produced by generateTokenUsageSummary ends with :|, e.g. |--:|-------|------:|-------:|. Fix the fixture to match:
const result = renderTokenTableAsPlainText(
"My Token Usage",
"| A | B |\n|---|---:|\n| 1 | 2 |"
);With the current broken regex this test would then fail, correctly surfacing the bug in CI.
| } | ||
| const markdown = generateTokenUsageSummary(summary); | ||
| if (markdown.length > 0) { | ||
| core.info(renderTokenTableAsPlainText(getSummaryTitle(), markdown)); |
There was a problem hiding this comment.
getSummaryTitle() is called twice in the same conditional block — store it in a const to avoid a redundant env read and a theoretical TOCTOU on process.env.
💡 Suggested fix
const markdown = generateTokenUsageSummary(summary);
if (markdown.length > 0) {
const title = getSummaryTitle(); // single read
core.info(renderTokenTableAsPlainText(title, markdown));
await appendStepSummarySection(title, markdown);
}In the current code, if process.env.GH_AW_TOKEN_USAGE_SUMMARY_TITLE is mutated between the two calls (e.g. by a test that doesn't reset env cleanly), the core.info title and the step-summary section title will diverge silently.
|
@copilot run pr-finisher skill |
…tion Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Fixed both issues raised in the review:
All 23 JS tests pass locally. |
The token usage table was only written to the GitHub step summary (collapsible
<details>block), making it invisible in the raw step log output. Step 58 now also emits the table directly tocore.info.Changes
parse_token_usage.cjs: AddedrenderTokenTableAsPlainText(title, markdown)— strips table separator rows, leading/trailing pipes, and**bold**markers — and callscore.info()with the result before writing to the step summary.parse_token_usage.test.cjs: Tests forrenderTokenTableAsPlainText(separator removal, pipe stripping, bold removal, title prefix) and assertscore.inforeceives table content in themain()integration test.Example log output (step 58):