Add diagnostic logging to MCP logs tool error path#13489
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…hs and exit code Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
🎉 Yo ho ho! Changeset Generator found the treasure and completed successfully! ⚓💰 |
Agent Container Tool Check
Result: 10/12 tools available Missing Tools:
All essential development tools (shell, git, JSON/YAML processing, HTTP, GitHub CLI, and core runtimes like Node, Python, Go) are available. Java and .NET runtimes are not installed in this container.
|
|
PR titles: Remove shared gh-aw import and inline MCP config in agentic workflows; Update Codex to 0.94.0 and Sandbox Runtime to 0.0.34
|
Smoke Test Results - Run 21632095567PRs Tested:
Overall Status: PASS (Playwright pending) cc @pelikhan
|
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
There was a problem hiding this comment.
Pull request overview
Adds diagnostic logging and richer error reporting for the MCP logs tool so failures include actionable execution context (command, exit info, stdout/stderr) instead of a generic message.
Changes:
- Logs the
logstool command parameters prior to execution. - Switches from
CombinedOutput()toOutput()to keep JSON stdout separate from stderr. - Enriches the JSON-RPC error message/data with command details, exit code, stdout, stderr, timeout, and workflow name.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Log the command being executed for debugging | ||
| mcpLog.Printf("Executing logs tool: workflow=%s, count=%d, firewall=%v, no_firewall=%v, timeout=%d, command_args=%v", | ||
| args.WorkflowName, args.Count, args.Firewall, args.NoFirewall, timeoutValue, cmdArgs) |
There was a problem hiding this comment.
The pre-exec log prints count=%d using args.Count, but when the MCP client omits count this will log 0 even though the underlying CLI has its own default (currently gh aw logs --count defaults to 10 in pkg/cli/logs_command.go:212). This can be misleading, and it also means the MCP-described default (schema sets 100) is not actually enforced by the server. Consider computing an explicit effectiveCount (apply the MCP default when args.Count==0) and always passing -c effectiveCount, and log that effective value.
| var stderr string | ||
| var exitCode int | ||
| if exitErr, ok := err.(*exec.ExitError); ok { | ||
| stderr = string(exitErr.Stderr) | ||
| exitCode = exitErr.ExitCode() | ||
| } |
There was a problem hiding this comment.
When cmd.Output() fails for reasons other than *exec.ExitError (e.g., context deadline exceeded / process killed), exitCode remains 0, which can incorrectly suggest success in both logs and jsonrpc error data. Consider initializing exitCode to a sentinel (e.g., -1) or omitting the field unless err is an *exec.ExitError.
| // Build detailed error data | ||
| errorData := map[string]any{ | ||
| "error": err.Error(), | ||
| "command": strings.Join(cmdArgs, " "), | ||
| "exit_code": exitCode, | ||
| "stdout": outputStr, | ||
| "stderr": stderr, | ||
| "timeout": timeoutValue, | ||
| "workflow": args.WorkflowName, | ||
| } |
There was a problem hiding this comment.
errorData includes full stdout and stderr strings. These streams can be very large (or contain sensitive content), which can bloat the JSON-RPC error payload and potentially leak data to clients/logs. Consider truncating stdout/stderr to a bounded size (and/or providing separate stdout_len/stderr_len fields), and optionally sanitizing before returning them in Data.
MCP logs tool failures returned only "failed to download workflow logs" with no diagnostic context, making debugging impossible without access to the runner environment.
Changes
Pre-execution logging
mcpLog.PrintfPost-failure logging
CombinedOutput()toOutput()for separate stdout/stderr handlingEnhanced error response
"failed to download workflow logs: exit status 1"vs"failed to download workflow logs"Pattern matches existing compile tool error handling in same file.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.