Skip to content

Render token table to core.info in parse_token_usage step #40227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
Jun 19, 2026
+51 −0
Merged

Render token table to core.info in parse_token_usage step #40227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03f6c99
Add token table rendering to core.info in parse_token_usage.cjs
Copilot Jun 19, 2026
12f6157
Fix separator regex to handle alignment colons; strengthen test asser…
Copilot Jun 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions actions/setup/js/parse_token_usage.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,26 @@ function buildStepSummarySection(title, markdown) {
return `### ${title}\n\n<details>\n<summary>Per-request AI credits and token totals</summary>\n\n${markdown}</details>\n\n`;
}

/**
* Renders the token usage markdown table as plain text for core.info output.
* Strips markdown table separators, pipes, and bold markers so the table is
* readable in the raw step log.
* @param {string} title
* @param {string} markdown
* @returns {string}
*/
function renderTokenTableAsPlainText(title, markdown) {
const plainText = markdown
.replace(/^\|(?:[-: ]+\|)+$/gm, "") // Remove table separator lines (handles alignment colons)
.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

@github-actions github-actions Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.replace(/\*\*(.*?)\*\*/g, "$1") // Remove bold markers
.replace(/\n{3,}/g, "\n\n") // Collapse excess blank lines
.trim();
return `${title}\n\n${plainText}`;
}

/**
* Appends the token usage section to GITHUB_STEP_SUMMARY when available.
* Falls back to the Actions summary API when the summary path is unavailable.
Expand Down Expand Up @@ -142,6 +162,7 @@ async function main() {
}
const markdown = generateTokenUsageSummary(summary);
if (markdown.length > 0) {
core.info(renderTokenTableAsPlainText(getSummaryTitle(), markdown));

@github-actions github-actions Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

@github-actions github-actions Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

await appendStepSummarySection(getSummaryTitle(), markdown);
}

Expand Down Expand Up @@ -199,6 +220,7 @@ if (typeof module !== "undefined" && module.exports) {
getSummaryTitle,
buildStepSummarySection,
appendStepSummarySection,
renderTokenTableAsPlainText,
TOKEN_USAGE_AUDIT_PATH,
TOKEN_USAGE_PATH,
TOKEN_USAGE_PATHS,
Expand Down
29 changes: 29 additions & 0 deletions actions/setup/js/parse_token_usage.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const {
readDedupedTokenUsage,
getSummaryTitle,
buildStepSummarySection,
renderTokenTableAsPlainText,
TOKEN_USAGE_AUDIT_PATH,
TOKEN_USAGE_PATH,
TOKEN_USAGE_PATHS,
Expand Down Expand Up @@ -194,6 +195,9 @@ describe("parse_token_usage", () => {
expect(mockCore.summary.addRaw).toHaveBeenCalledWith(expect.stringContaining("| Alias |"), true);
expect(mockCore.summary.write).toHaveBeenCalled();
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"));

@github-actions github-actions Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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));

});

test("uses custom summary title when configured", async () => {
Expand Down Expand Up @@ -536,5 +540,30 @@ describe("parse_token_usage", () => {
expect(section).toContain("<details>");
expect(section).toContain("<summary>Per-request AI credits and token totals</summary>");
});

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");

@github-actions github-actions Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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 gone

This would have caught the bug immediately.


const result = renderTokenTableAsPlainText("Token Usage", markdown);

expect(result).toContain("Token Usage");
// separator line is removed (no dash sequences that leak from separator rows)
expect(result).not.toMatch(/---/);
// leading/trailing pipes are stripped
expect(result).not.toMatch(/^\|/m);
expect(result).not.toMatch(/\|$/m);
// bold markers are removed
expect(result).not.toContain("**");
// data is preserved
expect(result).toContain("sonnet46");
expect(result).toContain("100");
expect(result).toContain("200");
expect(result).toContain("Legend:");
});

test("renderTokenTableAsPlainText prefixes output with title", () => {
const result = renderTokenTableAsPlainText("My Token Usage", "| A |\n|---|\n| 1 |");

@github-actions github-actions Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

expect(result.startsWith("My Token Usage")).toBe(true);
});
});
});
Loading