Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/contribution-check.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 1 addition & 5 deletions .github/workflows/contribution-check.md
Original file line number Diff line number Diff line change
Expand Up @@ -252,8 +252,4 @@ If any subagent call failed (❓), also apply `outdated`.
- Close the previous report issue when creating a new one (`close-older-issues: true`).
- Be constructive in assessments — these reports help maintainers prioritize, not gatekeep.

**Important**: If no action is needed after completing your analysis, you **MUST** call the `noop` safe-output tool with a brief explanation. Failing to call any safe-output tool is the most common cause of safe-output workflow failures.

```json
{"noop": {"message": "No action needed: [brief explanation of what was analyzed and why]"}}
```
{{#import shared/noop-reminder.md}}
4 changes: 1 addition & 3 deletions .github/workflows/daily-fact.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 1 addition & 3 deletions .github/workflows/smoke-claude.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions .github/workflows/smoke-crush.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 1 addition & 5 deletions .github/workflows/smoke-crush.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,4 @@ Add a **very brief** comment (max 5-10 lines) to the current pull request with:

If all tests pass, use the `add_labels` safe-output tool to add the label `smoke-crush` to the pull request.

**Important**: If no action is needed after completing your analysis, you **MUST** call the `noop` safe-output tool with a brief explanation. Failing to call any safe-output tool is the most common cause of safe-output workflow failures.

```json
{"noop": {"message": "No action needed: [brief explanation of what was analyzed and why]"}}
```
{{#import shared/noop-reminder.md}}
2 changes: 2 additions & 0 deletions .github/workflows/workflow-skill-extractor.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 1 addition & 5 deletions .github/workflows/workflow-skill-extractor.md
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,4 @@ Use this priority rubric:
- Keep recommendations concrete and actionable.
- If no action is needed, call `noop` with a brief explanation.

**Important**: If no action is needed after completing your analysis, you **MUST** call the `noop` safe-output tool with a brief explanation. Failing to call any safe-output tool is the most common cause of safe-output workflow failures.

```json
{"noop": {"message": "No action needed: [brief explanation of what was analyzed and why]"}}
```
{{#import shared/noop-reminder.md}}
11 changes: 2 additions & 9 deletions pkg/workflow/mcp_setup_generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -696,9 +696,6 @@ func (c *Compiler) generateMCPSetup(yaml *strings.Builder, tools map[string]any,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of --user \$\{MCP_GATEWAY_UID}:\$\{MCP_GATEWAY_GID} simplifies the docker command. Since the gateway now runs as the default container user, confirm that any files it writes to /tmp are still accessible by downstream steps (e.g., log redaction). A comment here noting the intended user context would help future readers.

var containerCmd strings.Builder
containerCmd.WriteString("docker run -i --rm --network host")
// Use runner UID/GID so gateway-created /tmp logs remain readable by downstream
// redaction/upload steps; keep a supplementary docker.sock group for daemon access.
containerCmd.WriteString(" --user ${MCP_GATEWAY_UID}:${MCP_GATEWAY_GID}")
containerCmd.WriteString(" --group-add ${DOCKER_SOCK_GID}")
containerCmd.WriteString(" -v /var/run/docker.sock:/var/run/docker.sock") // Enable docker-in-docker for MCP gateway
// Pass required gateway environment variables
Expand Down Expand Up @@ -905,15 +902,11 @@ func (c *Compiler) generateMCPSetup(yaml *strings.Builder, tools map[string]any,
}
}

// Compute the runner user/group IDs and Docker socket group ID before constructing
// the docker command so these values are expanded to literals in the exported command.
yaml.WriteString(" MCP_GATEWAY_UID=$(id -u 2>/dev/null || echo '0')\n")
yaml.WriteString(" MCP_GATEWAY_GID=$(id -g 2>/dev/null || echo '0')\n")
// Compute the Docker socket group ID so the MCPG container can access /var/run/docker.sock
yaml.WriteString(" DOCKER_SOCK_GID=$(stat -c '%g' /var/run/docker.sock 2>/dev/null || echo '0')\n")

// Build the export command with proper quoting that allows variable expansion
// We need to break out of quotes for shell variables like
// ${GITHUB_WORKSPACE}, ${MCP_GATEWAY_UID}, ${MCP_GATEWAY_GID}, and ${DOCKER_SOCK_GID}
// We need to break out of quotes for shell variables like ${GITHUB_WORKSPACE} and ${DOCKER_SOCK_GID}
cmdWithExpandableVars := buildDockerCommandWithExpandableVars(containerCmd.String())
yaml.WriteString(" export MCP_GATEWAY_DOCKER_COMMAND=" + cmdWithExpandableVars + "\n")
yaml.WriteString(" \n")
Expand Down
27 changes: 6 additions & 21 deletions pkg/workflow/mcp_setup_generator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -477,9 +477,9 @@ Test that TAVILY_API_KEY is passed to gateway container.
"Docker command should include -e TAVILY_API_KEY before the container image")
}

// TestMCPGatewayDockerCommandUsesRunnerIdentityAndSocketGroup verifies the gateway docker command
// computes and uses runner UID/GID and docker socket group values in the generated command.
func TestMCPGatewayDockerCommandUsesRunnerIdentityAndSocketGroup(t *testing.T) {
// TestMCPGatewayDockerCommandIncludesDockerSocketGroup verifies the gateway docker command
// adds the docker socket group as a supplementary group for non-root execution.
func TestMCPGatewayDockerCommandIncludesDockerSocketGroup(t *testing.T) {
frontmatter := `---
on: workflow_dispatch
engine: copilot
Expand Down Expand Up @@ -508,31 +508,16 @@ tools:
require.NoError(t, err, "Failed to read output file")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good cleanup — removing the --user assertions keeps the test focused on what actually matters now (socket GID). The remaining assertions clearly document the expected docker command shape.

yamlStr := string(content)

userSnippet := `--user '"${MCP_GATEWAY_UID}"':'"${MCP_GATEWAY_GID}"'`
groupAddSnippet := `--group-add '"${DOCKER_SOCK_GID}"'`
mountSnippet := `-v /var/run/docker.sock:/var/run/docker.sock`
uidComputeSnippet := `MCP_GATEWAY_UID=$(id -u 2>/dev/null || echo '0')`
runnerGIDComputeSnippet := `MCP_GATEWAY_GID=$(id -g 2>/dev/null || echo '0')`
socketGIDComputeSnippet := `DOCKER_SOCK_GID=$(stat -c '%g' /var/run/docker.sock 2>/dev/null || echo '0')`
require.Contains(t, yamlStr, uidComputeSnippet,
"Shell should compute MCP_GATEWAY_UID before docker command")
require.Contains(t, yamlStr, runnerGIDComputeSnippet,
"Shell should compute MCP_GATEWAY_GID before docker command")
require.Contains(t, yamlStr, userSnippet,
"Docker command should include runner UID/GID user mapping")
require.Contains(t, yamlStr, socketGIDComputeSnippet,
gidComputeSnippet := `DOCKER_SOCK_GID=$(stat -c '%g' /var/run/docker.sock 2>/dev/null || echo '0')`
require.Contains(t, yamlStr, gidComputeSnippet,
"Shell should compute DOCKER_SOCK_GID before docker command")
require.Contains(t, yamlStr, groupAddSnippet,
"Docker command should include docker socket supplementary group mapping")
require.Contains(t, yamlStr, mountSnippet,
"Docker command should mount the Docker socket")
require.Less(t, strings.Index(yamlStr, uidComputeSnippet), strings.Index(yamlStr, userSnippet),
"MCP_GATEWAY_UID should be computed before it is used in the docker command")
require.Less(t, strings.Index(yamlStr, runnerGIDComputeSnippet), strings.Index(yamlStr, userSnippet),
"MCP_GATEWAY_GID should be computed before it is used in the docker command")
require.Less(t, strings.Index(yamlStr, userSnippet), strings.Index(yamlStr, groupAddSnippet),
"Docker command should include user mapping before supplementary group mapping")
require.Less(t, strings.Index(yamlStr, socketGIDComputeSnippet), strings.Index(yamlStr, groupAddSnippet),
require.Less(t, strings.Index(yamlStr, gidComputeSnippet), strings.Index(yamlStr, groupAddSnippet),
"DOCKER_SOCK_GID should be computed before it is used in the docker command")
require.Less(t, strings.Index(yamlStr, groupAddSnippet), strings.Index(yamlStr, mountSnippet),
"Docker command should add supplementary group before mounting the Docker socket")
Expand Down
Loading