Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/fde5e67f-c171-4f4e-b85e-2e0f32fd98ab Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This reverts commit d1c9aea. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/fde5e67f-c171-4f4e-b85e-2e0f32fd98ab Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix CLI bridge type coercion bugs for integer and array params
Fix MCP CLI bridge arg coercion, audit error envelopes, and logs cache readability
Apr 18, 2026
Closed
Contributor
🧪 Test Quality Sentinel ReportTest Quality Score: 83/100✅ Excellent test quality
Test Classification DetailsView All 4 Tests
Flagged Tests — Improvement Opportunities
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes regressions in the MCP CLI tooling by aligning CLI wrapper argument typing and MCP error semantics with the tool schema, and by making the logs cache paths readable/usable by callers.
Changes:
- Added schema-aware CLI arg coercion (numbers/integers/arrays/booleans) and exported bridge functions for unit testing.
- Updated bridge + privileged tools to honor MCP
result.isError/CallToolResult.IsErrorfor in-band JSON error envelopes. - Normalized logs cache directory/file permissions and added guardrails around cache path shape.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/mcp_cli_bridge.cjs |
Schema-aware arg coercion, isError handling, and module exports for testing. |
actions/setup/js/mcp_cli_bridge.test.cjs |
Regression tests for coercion and isError response handling. |
pkg/cli/mcp_tools_privileged.go |
Marks audit JSON error envelopes as MCP errors via IsError: true. |
pkg/cli/mcp_tools_privileged_test.go |
Adds test coverage verifying audit error envelopes set IsError=true. |
pkg/cli/mcp_logs_guardrail.go |
Makes logs cache directory/files readable and validates cache dir is not a symlink/non-dir. |
pkg/cli/mcp_logs_guardrail_test.go |
Adds tests asserting cache directory/file permission bits are readable. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 4
Comment on lines
82
to
84
| // Write with O_EXCL to avoid following symlinks or races. | ||
| f, err := os.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600) | ||
| f, err := os.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, mcpLogsCacheFilePerm) | ||
| if err != nil { |
There was a problem hiding this comment.
The cache file permissions are only forced to mcpLogsCacheFilePerm in the “file already exists” path. When creating a new file, the effective mode is still subject to the process umask, so callers may still get a non-world-readable file (e.g., 0600) even though mcpLogsCacheFilePerm is 0644. Consider chmod’ing the file after a successful write/close (and handling errors) so readability is guaranteed regardless of umask.
Comment on lines
+75
to
+79
| if _, err := os.Lstat(filePath); err == nil { | ||
| if chmodErr := os.Chmod(filePath, mcpLogsCacheFilePerm); chmodErr != nil { | ||
| mcpLogsGuardrailLog.Printf("Failed to update logs cache file permissions: %v", chmodErr) | ||
| return buildLogsFileErrorResponse(fmt.Sprintf("failed to set logs cache file permissions: %v", chmodErr)) | ||
| } |
There was a problem hiding this comment.
In the “file already exists” branch, the code does an Lstat but then unconditionally calls os.Chmod(filePath, …). os.Chmod follows symlinks, so if filePath is ever a symlink (or not a regular file), this can change permissions on an unexpected target. Use the Lstat result to reject non-regular files (and symlinks) before chmod’ing.
Comment on lines
361
to
364
| return &mcp.CallToolResult{ | ||
| IsError: true, | ||
| Content: []mcp.Content{&mcp.TextContent{Text: string(jsonBytes)}}, | ||
| }, nil, nil |
There was a problem hiding this comment.
This change correctly marks the audit JSON error envelope as IsError=true, but other privileged tools in this file (e.g., audit-diff) still return JSON error envelopes without setting IsError. With the CLI bridge now honoring result.isError, those tools will still exit 0 on failures. Consider applying the same IsError=true behavior consistently to any tool that returns an error envelope in-band.
Comment on lines
+259
to
+261
| mockExecCmd := func(ctx context.Context, args ...string) *exec.Cmd { | ||
| return exec.CommandContext(ctx, "sh", "-c", "echo '✗ failed to fetch run metadata' 1>&2; exit 1") | ||
| } |
There was a problem hiding this comment.
This test shells out to sh -c ..., which will fail on Windows environments where sh isn’t available. Earlier tests in this file intentionally avoid Unix-specific commands; consider using a cross-platform helper process pattern (or another approach that doesn’t depend on sh) to simulate a command that writes to stderr and exits non-zero.
Collaborator
|
@copilot merge main and review all comments. Recompile |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b04fc52e-3927-4cf6-aedc-15758981e6f6 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This reverts commit 754bf75. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…e-type-coercion-bugs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…opes Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b04fc52e-3927-4cf6-aedc-15758981e6f6 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Merged 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:
|
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.
Exploratory CLI-tools testing found four regressions: MCP CLI wrappers were sending typed params as strings (
logs --count,compile --workflows),auditreturned error payloads withisError: false, andlogsreturned file paths under an unreadable cache directory. This PR aligns bridge/tool behavior with schema and MCP error semantics.Bridge: schema-aware CLI arg coercion (
actions/setup/js/mcp_cli_bridge.cjs)inputSchema.propertiesat parse time.integer/numbervalues are emitted as numeric JSON.arrayvalues support single values, comma-separated values, and JSON array input.Bridge: honor MCP tool error envelope
formatResponsenow treatsresult.isError === trueas a command error:tool_erroreventPrivileged MCP tools: correct audit error signaling (
pkg/cli/mcp_tools_privileged.go)registerAuditToolnow setsCallToolResult.IsError = truewhen returning JSON error envelopes for CLI execution failures.Logs cache accessibility (
pkg/cli/mcp_logs_guardrail.go)file_pathis usable by callers:07550644Focused regression tests
actions/setup/js/mcp_cli_bridge.test.cjsfor:isErrorresponse handlingIsError=true