[rendering-scripts] fix: normalize pi engine bash tool name for correct log rendering - #49304
Conversation
Pi's CLI emits bash tool calls with the lowercase name "bash", but the shared markdown formatter (log_parser_format.cjs) only special-cases the exact string "Bash" for compact command bullets and single-line collapsible summaries. This caused pi-engine step summaries to show bare "bash" bullets with no command text, and corrupted the detailed <summary> tags with raw multi-line command dumps. Add normalizePiToolName() in parse_pi_log.cjs, applied at both the legacy flat-schema and v3-streaming-schema tool-call sites, following the same "bash" -> "Bash" normalization already used by parse_codex_log.cjs and parse_copilot_log.cjs. Verified against real production log data (run 30616598129, Issue Monster workflow, pi engine): rendered markdown length increased from 3469 to 5049 chars, <details>/<summary>/fence counts remain balanced, and bash commands now render as single-line backtick-wrapped snippets consistent with other engines.
github-actions
Bot
added
automated-fix
javascript
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Fixes Pi engine bash tool-call rendering by aligning tool names with shared formatter expectations.
Changes:
- Normalizes
bashto canonicalBashin legacy and v3 Pi schemas. - Updates assertions for normalized names and rendered commands.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/parse_pi_log.cjs |
Normalizes Pi bash tool names before formatting. |
actions/setup/js/parse_pi_log.test.cjs |
Verifies normalized and rendered bash calls. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories. |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report🔶 Test Quality Score: 50/100 — Needs Improvement
📊 Metrics (2 tests)
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is clean and well-reasoned; one minor observation on test coverage.
📋 Key Themes & Highlights
Key Themes
- Normalisation follows existing convention:
normalizePiToolName()mirrors the pattern already used inparse_codex_log.cjsandparse_copilot_log.cjs— consistent, low-risk. - Both schema branches covered: both the legacy flat-schema
tool_usepath (line 162) and the v3-streamingtransformPiV3Entriespath (line 292) are fixed — root cause fully addressed. - Test assertions updated correctly: the two updated assertions now encode the expected normalised value (
"Bash",ls) rather than the old buggy output. - Minor gap:
normalizePiToolNameis exported but has no dedicated unit test. An isolated test would catch future variants (e.g.,"BASH","bash_20250124") before they reach rendering.
Positive Highlights
- ✅ Root cause identified and fixed, not just the symptom.
- ✅ Production verification with real log data (5 MB, run 30616598129) with before/after evidence.
- ✅ Structural HTML balance validated (details/summary counts unchanged).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 23.9 AIC · ⊞ 7K
Comment /matt to run again
| expect(entries[0].message.content[0].type).toBe("tool_use"); | ||
| expect(entries[0].message.content[0].name).toBe("bash"); | ||
| expect(entries[0].message.content[0].name).toBe("Bash"); | ||
| expect(entries[0].message.content[0].id).toBe("t1"); |
There was a problem hiding this comment.
[/tdd] No direct unit test for normalizePiToolName — the function is exported but only covered implicitly through integration-level assertions.
Adding a dedicated test would guard against future regressions (e.g., if Pi starts emitting "BASH" or another variant) and documents the full contract.
💡 Suggested test
describe('normalizePiToolName', () => {
it('maps lowercase bash to Bash', () => {
expect(normalizePiToolName('bash')).toBe('Bash');
});
it('passes through already-normalized names unchanged', () => {
expect(normalizePiToolName('Bash')).toBe('Bash');
expect(normalizePiToolName('str_replace_editor')).toBe('str_replace_editor');
});
});@copilot please address this.
There was a problem hiding this comment.
Added in 9f65de6. A dedicated describe('normalizePiToolName', ...) block covers lowercase bash, uppercase/mixed-case variants, and non-bash tool names (idempotency + no mutation).
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The fix is correct and well-scoped. normalizePiToolName is applied consistently in both transform paths, and the tests are properly updated.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 12.7 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: minor hardening suggestion, no blockers
Small, well-scoped fix that correctly follows the existing parse_codex_log.cjs/parse_copilot_log.cjs normalization convention and comes with updated tests.
Themes
- The added
normalizePiToolNamecorrectly fixes the observed lowercase"bash"rendering bug and is exercised by updated unit tests against a real production log. - One non-blocking hardening note posted: the match is case-sensitive and only covers exact
"bash", so any future casing variance ("BASH"," bash") would silently reintroduce the same bug with no diagnostic signal. Suggested atrim().toLowerCase()comparison plus idempotency test coverage. - Scope is minimal and change is low-risk; no correctness, security, or concurrency issues found in the diff.
🔎 Code quality review by PR Code Quality Reviewer · auto · 28 AIC · ⊞ 7.8K
Comment /review to run again
| * formatters (e.g. log_parser_format.cjs special-cases "Bash", not "bash"). | ||
| * @param {string} name - Raw tool name as emitted by Pi | ||
| * @returns {string} Normalized tool name | ||
| */ |
There was a problem hiding this comment.
Case-sensitive match only handles exact lowercase "bash"; if Pi ever emits a different casing the bug silently resurfaces with no diagnostic.
💡 Details
normalizePiToolName does name === "bash" ? "Bash" : name. This is fine for the currently observed payload, but it's a brittle point-fix rather than a true normalization: if Pi's API ever emits "BASH", " bash", or changes casing in a future release, this silently falls through unnormalized and reintroduces the exact rendering bug this PR is fixing — with no test or logging to catch the regression.
Suggested hardening:
function normalizePiToolName(name) {
return typeof name === "string" && name.trim().toLowerCase() === "bash" ? "Bash" : name;
}Also add a test case for an already-correct "Bash" input and an unrelated tool name (e.g. "read") to confirm idempotency and that non-bash names are never mutated.
There was a problem hiding this comment.
Addressed in 9f65de6. normalizePiToolName now uses typeof name === "string" && name.trim().toLowerCase() === "bash" to handle all casing variants robustly.
|
@copilot run pr-finisher skill |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Hey 👋 — thanks for the automated fix! This is a well-targeted rendering defect fix in the pi engine log parser. A quick note on process: This repository's CONTRIBUTING.md states that non-core team members should not open traditional pull requests directly, and should instead discuss via issues. Since this PR originates from an internal automated agentic workflow bot rather than an external contributor, this appears to be an accepted exception — but flagging for maintainer awareness. What looks great:
This PR looks ready for review and merge!
|
Both items were already addressed in 9f65de6:\n- |
|
🎉 This pull request is included in a new release. Release: |
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Summary
Normalizes the Pi engine's raw bash tool name to the canonical
"Bash"string so the shared log formatter renders bash tool calls correctly, instead of falling through to generic rendering.Problem
The Pi engine emits
tool_useentries where the bash tool's name arrives as lowercase"bash"(fromraw.tool_name/part.name). The shared formatterlog_parser_format.cjsspecial-cases the exact string"Bash"(capitalized) to apply bash-specific log rendering. A lowercase"bash"name fails this match, causing incorrect/generic rendering of bash tool calls in Pi engine logs.Changes
actions/setup/js/parse_pi_log.cjsnormalizePiToolName(name)helper: returns"Bash"when the trimmed, lowercased input equals"bash", otherwise returns the name unchanged.tool_useconstruction sites:transformPiEntries(schema v1/v2) andtransformPiV3Entries(schema v3).normalizePiToolNamefor unit testing.actions/setup/js/parse_pi_log.test.cjsnormalizePiToolName, covering case-insensitivity (e.g."bash","Bash","BASH", with surrounding whitespace) and passthrough for non-bash tool names."Bash"naming in transformed entries.Impact
Testing
parse_pi_log.test.cjsverifynormalizePiToolNamebehavior and updated transform output expectations.