Expose identifiable MCP tool calls in logs JSON - #59579
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The implementation consistently propagates the new fields with appropriate coverage and preserves sensitive payload exclusions.
Pull request overview
Extends compact usage artifacts so gh aw logs --json exposes identifiable MCP tool calls without payload data.
Changes:
- Captures timestamps and server/tool names for MCP calls.
- Maps and serializes the new fields in logs JSON.
- Adds generation, mapping, serialization tests, and schema documentation.
File summaries
| File | Description |
|---|---|
pkg/cli/logs_usage_activity.go |
Maps MCP call identity fields into logs data. |
pkg/cli/logs_usage_activity_test.go |
Tests usage artifact parsing and mapping. |
pkg/cli/logs_json_test.go |
Tests MCP identities in JSON output. |
docs/src/content/docs/reference/artifacts.md |
Documents expanded tool-call records. |
actions/setup/js/generate_usage_activity_summary.test.cjs |
Tests identity capture and payload exclusion. |
actions/setup/js/generate_usage_activity_summary.cjs |
Records timestamp, server, and tool per call. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
🎉 Thanks for this contribution, Copilot! This PR looks ready for review. The changes cleanly expose MCP tool call identity (timestamp, server name, tool name) in the logs JSON output, which directly addresses the gap in ✅ Compact artifact generation — Records timestamp, server name, and tool name per MCP call The PR preserves opaque synthetic call IDs without exposing arguments, results, or gateway identifiers—exactly the right balance for observability and safety.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
@copilot Please take the next forward-progress pass on PR #59579.
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"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.
Ponytail pass focused on deletion/shrink only. Two opportunities to reduce plumbing and assertion duplication while keeping behavior unchanged.
net: -14 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #59579 · codex · gpt53codex · 4.14 AIC · ⌖ 3.15 AIC · ⊞ 13.9K
Comment /ponytail to run again
| timestamp, | ||
| server_name: serverName, | ||
| tool_name: toolName, | ||
| request_size: inputSize, |
There was a problem hiding this comment.
actions/setup/js/generate_usage_activity_summary.cjs:L429: shrink: pass full gatewayCall object instead of threading timestamp as a new positional arg. Derive timestamp inside recordGatewayToolCall from the object.
| assert.Equal(t, 5, summary.Gateway.TotalCalls, "gateway total_calls should be parsed from JSON") | ||
| require.Len(t, summary.Gateway.ToolCalls, 1, "gateway tool_calls should be parsed from JSON") | ||
| assert.Equal(t, usageActivityGatewayCall{ToolCallID: "call-1", RequestSize: 100, ResponseSize: 200, DurationMS: 25, Outcome: "success"}, summary.Gateway.ToolCalls[0]) | ||
| assert.Equal(t, usageActivityGatewayCall{ToolCallID: "call-1", Timestamp: "2026-09-09T00:00:00Z", ServerName: "github", ToolName: "issue_read", RequestSize: 100, ResponseSize: 200, DurationMS: 25, Outcome: "success"}, summary.Gateway.ToolCalls[0]) |
There was a problem hiding this comment.
pkg/cli/logs_usage_activity_test.go:L54: shrink: full-struct equality duplicates every field in multiple tests. Assert only newly added Timestamp/ServerName/ToolName fields and reuse existing size/outcome checks.
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.
|
There was a problem hiding this comment.
Verdict
No blocking issues in the changed lines.
Reviewed themes
I checked the new MCP tool-call identity fields across summary generation, Go backfill/mapping, JSON serialization, tests, and docs. The only candidate concern was backward-compatibility for older summaries missing these fields, but the added fields already deserialize to zero values in Go and the code path tolerates empty strings, so that is not an actionable defect.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 47.1 AIC · ⌖ 7.32 AIC · ⊞ 21.8K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Small, well-scoped plumbing change: threads timestamp, server_name, and tool_name through the JS gateway/RPC parsers into the existing MCPToolCall struct (which already had these JSON fields defined but unpopulated for gateway-sourced calls). Tests were updated at both the JS (generate_usage_activity_summary.test.cjs) and Go (logs_usage_activity_test.go, logs_json_test.go) layers to assert the new fields round-trip correctly, and docs (artifacts.md) were updated to match the new schema example.
📋 Notes (non-blocking)
String(entry.timestamp || "")defaults to an empty string when missing, which is consistent with howserverName/toolNamealready handle absent fields elsewhere in the file — no new class of bug introduced.- No PII/secret redaction concerns:
server_name/tool_nameare tool identifiers, not arguments/results, matching the PR description's intent ("Retain synthetic call IDs without exposing arguments, results, or gateway identifiers"). Existing secret-redaction tests (not.toContain("secret-tool-id")) still pass per the diff. - Test coverage follows TDD principles reasonably well: assertions cover both the gateway JSONL and RPC-message JSONL code paths, and verify the empty-timestamp fallback case.
No actionable issues found. Approving.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 31.1 AIC · ⌖ 14.6 AIC · ⊞ 10.3K
Comment /matt to run again
|
🎉 This pull request is included in a new release. Release: |
Compact usage artifacts omitted MCP call identity, leaving
gh aw logs --jsonconsumers unable to determine which server and tool produced each call.Usage capture
Logs output
mcp_tool_usage.tool_calls.{ "timestamp": "2026-09-09T00:00:00Z", "server_name": "github", "tool_name": "issue_read", "status": "success" }Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.