[rendering-scripts] fix(copilot-parser): render bash command from data.command in SDK events#43750
Conversation
Copilot SDK tool.execution_start events carry the executed command as a top-level data.command field, not nested under data.input/data.parameters. The events->legacy normalizer only read data.input || data.parameters, so bash commands were dropped from the rendered step-summary (tool name and output showed, but not the command that ran). Add a buildToolInput helper that falls back to (and merges in) data.command, and a regression test covering a bash tool.execution_start with data.command. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
automated-fix
javascript
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43750 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Fixes Copilot SDK (events.jsonl) parsing so tool.execution_start events for bash no longer drop the executed command when rendering the step summary, by populating the legacy tool_use.input from data.command when data.input / data.parameters are absent.
Changes:
- Add
buildToolInput(data)inconvertCopilotEventsToLegacyLogEntries()to fall back todata.command(and merge it into structured input when needed). - Use
buildToolInput(data)at bothtool_useconstruction sites fortool.execution_startand thetool.execution_completefallback path. - Add a regression test asserting the bash command from
data.commandappears in the rendered markdown.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/log_parser_shared.cjs | Preserves bash command text from Copilot SDK tool events by building tool_use.input from data.command. |
| actions/setup/js/parse_copilot_log.test.cjs | Adds coverage for rendering data.command from SDK tool.execution_start events into the markdown summary. |
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: Low
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (1 test)
Verdict
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
The fix correctly addresses the root cause: buildToolInput now falls back to data.command when data.input and data.parameters are absent, and merges data.command into structured input when the key is missing. Both call sites in convertCopilotEventsToLegacyLogEntries are updated consistently.
Logic is sound:
Array.isArrayguard correctly prevents treating array-valueddata.inputas an object to spread.- Merge path only fires when
base.commandis not already set, so existing structured inputs are not clobbered. tool.execution_completeorphan-entry fallback also receives the fix.
Test coverage is appropriate: the new test exercises the real event shape from the production log, asserting that the command and output both appear in the rendered markdown.
No security, correctness, or reliability issues found in the changed lines.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 27.1 AIC · ⌖ 5.96 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — fix is correct and targeted; approving with two minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Orphaned-completion path:
buildToolInputis called at line 948 withtool.execution_completedata, but completion events never carrydata.command. The call always resolves to{}correctly, but intent would be clearer with a comment or plain{}. - Missing merge-branch test: the regression test covers the pure
data.commandfallback path but not the branch where bothdata.input/data.parametersanddata.commandcoexist and the command needs to be merged in.
Positive Highlights
- ✅ Root cause precisely identified; fix is minimal — single helper replaces two identical inline expressions.
- ✅
buildToolInputhandles all three cases (merge, bare command, empty) with clear conditional structure. - ✅ Regression test uses the exact real-world SDK event format from the triggering run — good fidelity.
- ✅ PR description includes a thorough static-trace walkthrough, making the fix easy to audit.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 38.6 AIC · ⌖ 5.45 AIC · ⊞ 6.7K
Comment /matt to run again
| type: "assistant", | ||
| message: { | ||
| content: [{ type: "tool_use", id: resolvedToolId, name: toolName, input: data.input || data.parameters || {} }], | ||
| content: [{ type: "tool_use", id: resolvedToolId, name: toolName, input: buildToolInput(data) }], |
There was a problem hiding this comment.
[/tdd] The buildToolInput(data) call here uses data from a tool.execution_complete event (orphaned completion path). Real SDK completion events do not carry a command field, so this will always return {} — the logic is harmless but could mislead future maintainers.
💡 Suggestion
Consider making the intent explicit with an inline comment or a plain {}:
// Orphaned completion: no matching start event; command is unavailable
content: [{ type: "tool_use", id: resolvedToolId, name: toolName, input: {} }],A small test for the orphaned-completion path would also document this behaviour as a spec.
@copilot please address this.
| const result = parseCopilotLog(eventsLog); | ||
|
|
||
| expect(result.markdown).toContain("bash"); | ||
| expect(result.markdown).toContain("cat /tmp/gh-aw/agent/candidates.txt"); |
There was a problem hiding this comment.
[/tdd] The regression test covers the happy path (command present on tool.execution_start), but misses the merge case: when both data.input (or data.parameters) and data.command are present and base.command is undefined. Adding a test for that branch would prevent a silent regression if the merge logic changes.
💡 Suggested additional test
it('merges data.command into existing data.input when base.command is absent', () => {
const eventsLog = [
'{"type":"user.message","data":{}}',
'{"type":"tool.execution_start","data":{"toolName":"bash","mcpServerName":"","input":{"cwd":"/tmp"},"command":"ls"}}',
'{"type":"tool.execution_complete","data":{"toolName":"bash","mcpServerName":"","success":true,"result":{"content":"out"}}}',
'{"type":"assistant.message","data":{"content":"Done"}}',
].join('\n');
const result = parseCopilotLog(eventsLog);
expect(result.markdown).toContain('ls');
expect(result.markdown).toContain('/tmp'); // cwd from input preserved
});@copilot please address this.
Summary
The Copilot engine log parser drops bash commands from the rendered step-summary. Tool calls show the tool name (
bash) and their output, but not the command that actually ran — losing the most important detail of every bash step.Trigger
Daily Syntax Error Quality CheckRoot cause
Real Copilot SDK events (serialised to
events.jsonland echoed toagent-stdio.log) carry the executed command as a top-leveldata.commandfield ontool.execution_start:{"type":"tool.execution_start","data":{"toolName":"bash","mcpServerName":"","command":"cat /tmp/gh-aw/agent/candidates.txt"}}But
convertCopilotEventsToLegacyLogEntriesinlog_parser_shared.cjsbuilt thetool_useinput withdata.input || data.parameters || {}— neither of which exists for these events — soinputwas{}and the command was never rendered. The existing event-format tests only usedtool.execution_startevents without acommandfield, so the gap was untested.Changes
buildToolInput(data)helper that falls back todata.command(and merges it into structured input when absent), used at bothtool_useconstruction sites in the events→legacy normalizer.tool.execution_startwithdata.commandrenders the command in the markdown summary.Verification method
The parser harness and JS test suite could not be executed in this scheduled sandbox run —
node,npx, andmakerequire interactive approval that is unavailable here. The defect and fix were validated by static tracing of the parser against the realrun-28779380421agent-stdio.log:parseLogEntriescorrectly extracts the JSONL events (infra[INFO]/[copilot-harness]lines are skipped because they do not start with{).isCopilotEventLogEntriesdetects thetool./assistant./user.event prefixes.convertCopilotEventsToLegacyLogEntriesmappedtool.execution_start→tool_usewithinput = data.input || data.parameters || {}={}(command dropped).formatToolUsethen renders onlybashwith output, no command.With the fix,
input = { command: "cat ..." }, and theformatToolUsedefault case rendersbash: cat ...plus a Parameters section. CI must run the full JS test suite to confirm no regressions.Test Results
renders the bash command from data.command in Copilot CLI events.jsonl— to be validated by CIReferences: §28779380421