Conversation
…uts in logs/audit/compile tools Agent-Logs-Url: https://github.com/github/gh-aw/sessions/4b2dea1f-e4c0-4f4d-88e3-a09eb6b003b2 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix MCP tools to return structured JSON instead of exceptions
Return structured JSON instead of MCP protocol errors for invalid inputs in logs/audit/compile tools
Apr 2, 2026
Collaborator
|
@copilot Implement fixes |
…or cases Agent-Logs-Url: https://github.com/github/gh-aw/sessions/89d314cb-76c3-41b7-a9c0-a4b7bc50b92d Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Implemented in commit 8d99388. The three fixes are:
The integration test for audit was also updated to assert JSON content is returned rather than an MCP protocol error. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts MCP tool behavior so logs, audit, and compile return structured JSON for certain invalid/error inputs instead of emitting MCP protocol errors, improving response consistency for callers.
Changes:
logs: return an emptyLogsDataJSON payload (with a message) when the workflow name is invalid/not found.audit: return a JSON error envelope (error,run_id_or_url) in tool content instead of-32603.compile: when Docker prep fails, return per-workflow[]ValidationResultentries withconfig_errorinstead of-32603; update integration test expectations.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/mcp_tools_readonly.go | Changes compile tool to return JSON validation results when Docker image prep fails; adds helper to generate per-workflow error results. |
| pkg/cli/mcp_tools_privileged.go | Changes logs and audit tools to return JSON payloads instead of MCP protocol errors for specific invalid/error inputs. |
| pkg/cli/mcp_server_error_codes_test.go | Updates integration test to assert audit returns JSON content (not MCP errors) and adds helper for CI permission-error skipping. |
| pkg/cli/logs_report.go | Adds an optional message field to LogsData for conveying “workflow not found” without breaking existing callers. |
| .github/workflows/stale-repo-identifier.lock.yml | Adjusts a job-level concurrency group to include a per-input discriminator. |
| .github/workflows/slide-deck-maintainer.lock.yml | Adjusts a job-level concurrency group to include a per-input discriminator. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/cli/mcp_tools_readonly.go
Outdated
| results := buildDockerErrorResults(args.Workflows, err.Error()) | ||
| jsonBytes, jsonErr := json.Marshal(results) | ||
| if jsonErr != nil { | ||
| return nil, nil, newMCPError(jsonrpc.CodeInternalError, err.Error(), nil) |
There was a problem hiding this comment.
In the json.Marshal failure path, the error message passed to newMCPError uses the Docker-prep error (err.Error()) rather than the marshal error (jsonErr). This makes debugging misleading; return an InternalError that references jsonErr (and ideally a clear message like "failed to marshal docker error results").
Suggested change
| return nil, nil, newMCPError(jsonrpc.CodeInternalError, err.Error(), nil) | |
| return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to marshal docker error results", jsonErr.Error()) |
| // Determine which workflow names to report | ||
| var workflowNames []string | ||
| if len(requestedWorkflows) > 0 { | ||
| for _, w := range requestedWorkflows { |
There was a problem hiding this comment.
buildDockerErrorResults derives workflow names via filepath.Base(w). When callers pass workflow IDs without an extension (common in MCP tests and CLI usage, e.g. "test1"), this will emit "test1" instead of the usual "test1.md" seen in normal compile ValidationResult outputs (compileWorkflowFile uses filepath.Base(resolvedFile) after resolveWorkflowFile appends .md). Consider normalizing requestedWorkflows the same way (append .md when missing, then Base) so error results match standard output.
Suggested change
| for _, w := range requestedWorkflows { | |
| for _, w := range requestedWorkflows { | |
| // Normalize workflow identifiers so they match the standard compile output. | |
| // If the caller passed an ID without an extension (e.g. "test1"), | |
| // treat it as a markdown workflow file ("test1.md") before taking the basename. | |
| if filepath.Ext(w) == "" { | |
| w = w + ".md" | |
| } |
pkg/cli/mcp_tools_privileged.go
Outdated
Comment on lines
+97
to
+102
| // Use an explicit empty slice for Runs so JSON marshaling produces "runs":[] | ||
| // rather than "runs":null (nil slice). | ||
| emptyData := LogsData{ | ||
| Runs: []RunData{}, | ||
| Message: err.Error(), | ||
| } |
There was a problem hiding this comment.
The invalid-workflow early return only sets Runs (empty) and Message. Because LogsData.Episodes and LogsData.Edges are non-omitempty slices, they will marshal as null here (and Summary.TotalDuration will be ""), while normal logs output uses empty slices ([]) and a formatted duration string (timeutil.FormatDuration(0) => "0ns"). To keep the JSON shape consistent for callers, initialize Summary (at least TotalDuration) and set Episodes/Edges to empty slices as well.
Suggested change
| // Use an explicit empty slice for Runs so JSON marshaling produces "runs":[] | |
| // rather than "runs":null (nil slice). | |
| emptyData := LogsData{ | |
| Runs: []RunData{}, | |
| Message: err.Error(), | |
| } | |
| // Use explicit empty slices so JSON marshaling produces "runs":[], etc., | |
| // rather than null (nil slices). | |
| emptyData := LogsData{ | |
| Runs: []RunData{}, | |
| Episodes: []EpisodeData{}, | |
| Edges: []EdgeData{}, | |
| Message: err.Error(), | |
| } | |
| // Ensure Summary.TotalDuration matches the normal zero-duration formatting. | |
| emptyData.Summary.TotalDuration = "0ns" |
Collaborator
|
@copilot Review comments |
…w IDs, initialize empty LogsData slices Agent-Logs-Url: https://github.com/github/gh-aw/sessions/94274ad0-c46c-4ff1-a6dd-82902d3516e6 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Addressed all three review comments in commit 95fd580:
|
Copilot stopped work on behalf of
pelikhan due to an error
April 2, 2026 12:18
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three MCP tools throw protocol errors (
-32602/-32603) for certain invalid inputs instead of returning structured JSON, breaking callers that expect consistent JSON responses.Changes
logs— workflow not found → empty result set-32602whenvalidateMCPWorkflowNamefails, return a validLogsDataJSON response withruns: [], zeroedsummary, and amessagefield indicating the workflow was not foundMessage stringfield (omitempty) toLogsDataso the reason is conveyed without breaking existing callers"runs":[](notnull) in JSON outputaudit— run not found → JSON error envelope-32603, return a JSON error object{"error": "...", "run_id_or_url": "..."}as tool contentcompile— Docker unavailable → per-workflow validation errors-32603whenCheckAndPrepareDockerImagesfails, enumerate the target workflows and return a[]ValidationResultwithvalid: falseand aconfig_errorentry per workflowbuildDockerErrorResults()helper that discovers workflow files automatically when no specific workflows are requested[{"workflow": "my-workflow", "valid": false, "errors": [{"type": "config_error", "message": "Docker is not available..."}], "warnings": []}]Integration test update
TestMCPServer_ErrorCodes_InternalErrorto assert thatauditreturns JSON content witherrorandrun_id_or_urlfields rather than an MCP protocol errorisTestEnvPermissionErrorhelper to deduplicate CI-skip logic