Run safe-outputs MCP in the gh-aw node container#39100
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
💫 TO BE CONTINUED... Smoke Claude failed to deliver outputs! Our hero faces unexpected challenges... |
|
🚀 Smoke Antigravity MISSION COMPLETE! Antigravity has spoken. ✨ |
|
❌ Design Decision Gate 🏗️ failed to deliver outputs during design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
|
|
|
🚀 Smoke Pi MISSION COMPLETE! Pi delivered. 🥧 |
|
🌑 The shadows whisper... Smoke Codex failed to deliver outputs. The oracle requires further meditation... |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
📰 BREAKING: Smoke Copilot - AOAI (Entra) is now investigating this pull request. Sources say the story is developing... |
|
📰 BREAKING: Smoke Copilot - AOAI (apikey) is now investigating this pull request. Sources say the story is developing... |
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
Warning The 🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (233 new lines across 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
Smoke Test: Codex - 27471836485
Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
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. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
💥 Smoke Test: Claude — Run 27471836462Core #1-12: ✅✅✅✅✅✅✅✅✅✅✅✅ Overall: PARTIAL (all run; #19 close-PR skipped — no safe test PR). Claude engine nominal! 🚀 Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.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.
💥 Automated smoke test review - all systems nominal!
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · 84.9 AIC · ⌖ 7.94 AIC · ⊞ 8.1K
| // Using node:lts-alpine provides the latest LTS version with minimal footprint | ||
| const DefaultNodeAlpineLTSImage = "node:lts-alpine" | ||
|
|
||
| // DefaultGhAwNodeImage is the published gh-aw Node container image used for |
There was a problem hiding this comment.
Nice — documenting DefaultGhAwNodeImage with the rationale for needing Node.js + git is helpful for future maintainers. Consider noting the expected tag/version policy here too.
| const DefaultWorkspaceMount = "\\${GITHUB_WORKSPACE}:\\${GITHUB_WORKSPACE}:rw" | ||
|
|
||
| // DefaultSafeOutputsMount is the mount path for safe-outputs runtime files | ||
| // (config.json, tools.json, outputs.jsonl, upload-artifacts/) in the runner temp directory. |
There was a problem hiding this comment.
The safe-outputs mount constants are clear. Good use of GhAwRootDirShell for consistency across mounts.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs, /improve-codebase-architecture, /tdd, and /zoom-out — commenting with improvement suggestions; no blocking correctness issues found.
📋 Key Themes & Highlights
Key Themes
- Stale comments: The
renderSafeOutputsTOMLfunction comment still says "HTTP transport instead of stdio" — the exact opposite of what the PR implements. - Test name drift:
TestSafeOutputsAPIKeyImmediateMaskingwas not renamed despite its body and doc comment being completely rewritten to test the absence of an API key. - Dual sources of truth for env vars: TOML renderer hardcodes a raw
env_varsstring; JSON renderer uses a typed struct slice. They agree today across 14 vars, but will silently diverge on the next addition. - Untagged container image:
ghcr.io/github/gh-aw-nodewith no version tag contrasts with the gateway pin (v0.3.25). Worth aligning policy or documenting the intent. - Unexplained escaping asymmetry in TOML:
argsuses$GITHUB_WORKSPACEwhilemounts(viaDefaultWorkspaceMount) uses\${GITHUB_WORKSPACE}— a brief comment would prevent future confusion.
Positive Highlights
- ✅ Clean removal of ~53 lines of HTTP server bootstrapping (port, API key, bash
exportblock) — significant complexity reduction. - ✅
RUNNER_TEMPis now correctly threaded through to the gateway docker command (golden file updates confirm this). - ✅
buildSafeOutputsConfigRuntimeEnvVars/buildSafeOutputsConfigRuntimeDataare still used for config.json generation — the regression concern (dynamicGH_AW_INPUT_*vars) is correctly handled by pre-resolving into config.json before the container starts. - ✅ New mount constants (
DefaultSafeOutputsMount,DefaultSafeOutputsLogMount) are well-named and follow existing conventions. - ✅ Tests properly assert the new behaviour (container image, mounts, absence of HTTP artefacts) and the renamed
TestSafeOutputsMCPContainerUsesRuntimePathsis a good improvement.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 585.4 AIC · ⌖ 40.9 AIC · ⊞ 29.4K
| yaml.WriteString(" \n") | ||
| yaml.WriteString(" [mcp_servers." + constants.SafeOutputsMCPServerID.String() + ".headers]\n") | ||
| yaml.WriteString(" Authorization = \"$GH_AW_SAFE_OUTPUTS_API_KEY\"\n") | ||
| yaml.WriteString(" container = \"" + constants.DefaultGhAwNodeImage + "\"\n") |
There was a problem hiding this comment.
[/grill-with-docs] Stale function comment one line above: // Now uses HTTP transport instead of stdio, similar to mcp-scripts now says the exact opposite of what the function does.
💡 Suggested fix
Change the comment to:
// renderSafeOutputsTOML generates Safe Outputs MCP configuration in TOML format.
// Uses containerized stdio transport running in the published gh-aw node image.Left unchanged, this misleads the next reader into thinking this function configures HTTP, which is the old approach being replaced.
| // is masked immediately after generation, before any other operations. | ||
| // TestSafeOutputsNoLongerGeneratesHTTPAPIKey verifies that safe-outputs no longer | ||
| // creates an HTTP server API key now that it runs as a stdio MCP container. | ||
| func TestSafeOutputsAPIKeyImmediateMasking(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] Test name TestSafeOutputsAPIKeyImmediateMasking no longer describes what it tests. The updated doc comment above says it now verifies the absence of an HTTP API key, but the function name still implies it is testing masking behaviour.
💡 Suggested rename
func TestSafeOutputsNoLongerGeneratesHTTPAPIKey(t *testing.T) {Test names are the first thing a failing CI shows. A reader hitting a failure in TestSafeOutputsAPIKeyImmediateMasking will look for API-key masking logic that no longer exists.
| yaml.WriteString(" container = \"" + constants.DefaultGhAwNodeImage + "\"\n") | ||
| yaml.WriteString(" mounts = [\"" + constants.DefaultWorkspaceMount + "\", \"" + constants.DefaultSafeOutputsMount + "\", \"" + constants.DefaultSafeOutputsLogMount + "\"]\n") | ||
| yaml.WriteString(" args = [\"-w\", \"$GITHUB_WORKSPACE\"]\n") | ||
| yaml.WriteString(" env_vars = [\"DEBUG\", \"DEFAULT_BRANCH\", \"GH_AW_ASSETS_ALLOWED_EXTS\", \"GH_AW_ASSETS_BRANCH\", \"GH_AW_ASSETS_MAX_SIZE_KB\", \"GH_AW_MCP_LOG_DIR\", \"GH_AW_SAFE_OUTPUTS\", \"GH_AW_SAFE_OUTPUTS_CONFIG_PATH\", \"GH_AW_SAFE_OUTPUTS_TOOLS_PATH\", \"GITHUB_REPOSITORY\", \"GITHUB_SERVER_URL\", \"GITHUB_TOKEN\", \"GITHUB_WORKSPACE\", \"RUNNER_TEMP\"]\n") |
There was a problem hiding this comment.
[/improve-codebase-architecture] The TOML renderer hardcodes env_vars as a raw string literal, while the JSON renderer (renderSafeOutputsMCPConfigWithOptions) drives the same list from a typed struct slice. These two sources currently agree on 14 vars, but they will drift the moment someone adds a var to one and forgets the other.
💡 Suggested approach
Extract a shared slice in mcp_renderer_builtin.go:
var safeOutputsEnvVarNames = []string{
"DEBUG", "DEFAULT_BRANCH", "GH_AW_ASSETS_ALLOWED_EXTS",
"GH_AW_ASSETS_BRANCH", "GH_AW_ASSETS_MAX_SIZE_KB",
"GH_AW_MCP_LOG_DIR", "GH_AW_SAFE_OUTPUTS",
"GH_AW_SAFE_OUTPUTS_CONFIG_PATH", "GH_AW_SAFE_OUTPUTS_TOOLS_PATH",
"GITHUB_REPOSITORY", "GITHUB_SERVER_URL", "GITHUB_TOKEN",
"GITHUB_WORKSPACE", "RUNNER_TEMP",
}The TOML renderer builds its env_vars line from this slice; the JSON renderer iterates the same slice. A future add is then a single-line change with no risk of format-specific drift.
| // DefaultGhAwNodeImage is the published gh-aw Node container image used for | ||
| // JavaScript-based MCP servers that need Node.js and git plus workspace-mounted | ||
| // repository scripts at runtime. | ||
| const DefaultGhAwNodeImage = "ghcr.io/github/gh-aw-node" |
There was a problem hiding this comment.
[/zoom-out] ghcr.io/github/gh-aw-node has no version tag, implying latest. The gateway image is pinned (ghcr.io/github/gh-aw-mcpg:v0.3.25). For a safety-critical MCP server that handles write operations (issues, PRs, assets), floating on latest means a silent image change could alter tool behaviour between runs.
💡 Options
- Pin a version tag (preferred for production stability):
"ghcr.io/github/gh-aw-node:v1.2.3"— same pattern as the gateway. - Document the intent if
latestis deliberate (e.g. the image is always forward-compatible and the team controls both the image and this constant together).
Either way, a comment explaining the versioning policy would help reviewers of future PRs understand why this differs from the gateway pin.
| yaml.WriteString(" Authorization = \"$GH_AW_SAFE_OUTPUTS_API_KEY\"\n") | ||
| yaml.WriteString(" container = \"" + constants.DefaultGhAwNodeImage + "\"\n") | ||
| yaml.WriteString(" mounts = [\"" + constants.DefaultWorkspaceMount + "\", \"" + constants.DefaultSafeOutputsMount + "\", \"" + constants.DefaultSafeOutputsLogMount + "\"]\n") | ||
| yaml.WriteString(" args = [\"-w\", \"$GITHUB_WORKSPACE\"]\n") |
There was a problem hiding this comment.
[/grill-with-docs] The TOML args field uses unescaped $GITHUB_WORKSPACE, while the JSON args at line 241 always uses \${GITHUB_WORKSPACE} (backslash-escaped) regardless of engine. Meanwhile, TOML mounts uses the backslash-escaped DefaultWorkspaceMount constant. The asymmetry within TOML (escaped mounts, unescaped args) is not explained and could mislead the next person editing either renderer.
💡 Suggestion
Add a short inline comment documenting the intent:
// TOML: args expand shell vars directly; mounts use \${...} for gateway-side expansion.
yaml.WriteString(" args = [\"-w\", \"$GITHUB_WORKSPACE\"]\n")Or if the two renderers should use identical escaping for args, align them so the behaviour is consistent and auditable from a single place.
|
Run safe-outputs MCP in the gh-aw node container Warning Firewall blocked 5 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 83/100 — Excellent
📊 Metrics & Test Classification (12 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
i️ Notes — No Action Required (2 items)Test Inflation (mechanical penalty applied, migration context)
|
|
@copilot review all comments and address unresolved review feedback.
|
|
@copilot recompile all lock files |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — recompiled all 246 lock files. Also fixed a |
|
@copilot review all comments and address unresolved review feedback. Please resolve the open TOML escaping thread, then rebase/update from main and rerun checks.
|
|
@copilot merge main |
…e-outputs-mcp-server Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Merged |
|
``
|
This changes safe-outputs from a separately started HTTP sidecar into a first-class MCP server entry. The generated workflow now runs safe-outputs inside the
gh-aw-nodecontainer with write access to the workspace, safe-outputs runtime files, and the safe-outputs log directory.What changed
ghcr.io/github/gh-aw-node.${RUNNER_TEMP}/gh-aw/safeoutputs/tmp/gh-aw/mcp-logs/safeoutputsWorkflow generation
config.json,tools.json) but made those files inputs to the MCP container instead of a host-side server process.MCP server model
Generated shape
{ "safeoutputs": { "container": "ghcr.io/github/gh-aw-node", "mounts": [ "${GITHUB_WORKSPACE}:${GITHUB_WORKSPACE}:rw", "${RUNNER_TEMP}/gh-aw/safeoutputs:${RUNNER_TEMP}/gh-aw/safeoutputs:rw", "/tmp/gh-aw/mcp-logs/safeoutputs:/tmp/gh-aw/mcp-logs/safeoutputs:rw" ], "args": ["-w", "${GITHUB_WORKSPACE}"] } }Related updates
Changeset\n\n- Type: patch\n- Description: Run safe-outputs as a containerized MCP server inside the runtime instead of a separate HTTP sidecar.
✨ PR Review Safe Output Test - Run 27471836462
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.comSee Network Configuration for more information.