Allow sandbox agents to reach declared service ports - #51842
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
services: ports from sandboxed agent|
❌ Test Quality Sentinel failed during test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
|
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
There was a problem hiding this comment.
Pull request overview
Adds service-derived and explicit host-port allowlists for sandboxed agents.
Changes:
- Parses, validates, sorts, and emits host ports.
- Adds
sandbox.agent.allow-host-ports. - Updates tests, documentation, schema, autocomplete, and generated workflow.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/service_ports.go |
Extracts host ports from services. |
pkg/workflow/sandbox.go |
Stores explicit host ports. |
pkg/workflow/sandbox_validation.go |
Validates port ranges. |
pkg/workflow/sandbox_validation_test.go |
Tests validation. |
pkg/workflow/frontmatter_extraction_security.go |
Parses the new setting. |
pkg/workflow/frontmatter_extraction_security_test.go |
Tests parsing. |
pkg/workflow/awf_command_builder.go |
Emits the AWF allowlist. |
pkg/workflow/awf_command_builder_test.go |
Tests command generation. |
pkg/parser/schemas/main_workflow_schema.json |
Defines the schema. |
docs/src/content/docs/reference/sandbox.md |
Documents service access. |
docs/src/content/docs/reference/frontmatter-full.md |
Documents frontmatter syntax. |
docs/src/content/docs/guides/upgrading.md |
Adds upgrade guidance. |
docs/public/editor/autocomplete-data.json |
Adds editor autocomplete. |
.github/workflows/smoke-service-ports.lock.yml |
Regenerates the smoke workflow. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 14/14 changed files
- Comments generated: 5
- Review effort level: Balanced
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all 5 review threads by reverting |
|
/review |
|
❌ Test Quality Sentinel failed during test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /codebase-design, and /grill-with-docs — requesting changes on three moderate issues.
📋 Key Themes & Highlights
Issues
-
Misleading test fixtures (
frontmatter_extraction_security_test.go) — extraction test uses5432and9200, which are inawfDangerousHostPorts. Parsing-layer tests are correct, but the examples imply these ports are valid end-to-end values for users. -
Missing defense-in-depth in
collectAllowedHostPorts— dangerous ports are only filtered in validation, not in the builder. If validation is ever bypassed or the call order changes, a dangerous port could silently appear in--allow-host-ports. -
Undocumented drift risk in
awfDangerousHostPorts— the hardcoded list mirrors an upstream firewall list with no automated sync anchor. A version/commit reference comment or a minimum unit test would reduce the risk of silent drift. -
PR description vs. docs contradiction (
sandbox.mdline 68) — the docs correctly state that reaching service ports requireslegacy-security: enable, but the PR description's top-level example omits it. This is a minor documentation inconsistency that could confuse users reading the description later.
Positive Highlights
- ✅ Clear separation between
--allow-host-service-ports(runtime, dynamic) and--allow-host-ports(static, legacy-only) — the architecture choice is well-reasoned. - ✅ Deterministic port output (dedup + sort) is a good correctness property.
- ✅ Version-gate warning with specific minimum version is excellent UX.
- ✅ Test coverage is thorough: strict-mode warning, version-too-old path, and merge-and-deduplicate all covered.
- ✅
awfDangerousHostPortslist is appropriately conservative and well-commented.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 45.9 AIC · ⌖ 10.4 AIC · ⊞ 7.1K
Comment /matt to run again
Reconciled all 5 threads against the current branch — all are superseded, no further action needed:
|
|
@copilot quick triage: your last reconciliation says the remaining host-port review threads are superseded, but they still appear unresolved from a maintainer triage perspective. Please run the pr-finisher skill, reply directly on each still-open thread with the specific superseding code path or resolution status, and return a concise maintainer-facing summary once the thread state matches the branch. Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31454952282
|
Replied directly on each of the 5 threads with the specific superseding code path:
Branch verified: |
|
@copilot quick triage: the branch now appears aligned with the superseding fixes you described, but the PR is still not easy for a maintainer to clear quickly because the thread state and blocking review state lag the code. Please run the pr-finisher skill, verify every still-open review thread has a direct maintainer-friendly resolution on-thread, confirm whether all remaining change-request signals can now be cleared, and return a concise summary of what is resolved vs what still needs human action. Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31456898570
|
Verified current state, no further code changes needed:
Still needed from a human: click "Resolve conversation" on the 5 threads (content already answered) and re-approve/re-run required checks to clear the |
|
@copilot quick triage: CI is currently failing on Please fix the failing check and then run the
Failed checks: Branch refresh was requested.
|
Fixed in |
Status:
|
Add two new terms based on recent commits (#51842, #51870): - Host Service Ports (services:) — AWF sandbox access to GitHub Actions services containers via --allow-host-service-ports - MCP Gateway Mount-Roots Allowlist (MCP_GATEWAY_ALLOWED_MOUNT_ROOTS) — compiler-computed mount policy allowlist for MCP backend containers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Sandboxed agents could not reach GitHub Actions
services:ports because--allow-host-portswas only emitted in legacy security mode and only with a hardcoded default list. Workflows with databases or other service containers failed silently inside the agent loop.Compiler behavior
services:port mappings.--allow-host-portsin strict security mode when service or explicit host ports are present.--enable-host-accesslegacy-only.Explicit escape hatch
sandbox.agent.allow-host-portsfor host daemons not declared asservices:.1..65535.Deterministic output
--allow-host-ports.Example:
Compiles to an allowlist including
5432and9200without enabling legacy host access.services:ports are unreachable from the sandboxed agent —--allow-host-portsis hardcoded andallowHostServicePortsis never emitted #51433Run URL: https://github.com/github/gh-aw/actions/runs/31450173394> Generated by 👨🍳 PR Sous Chef · gpt54 · 24.1 AIC · ⌖ 5.21 AIC · ⊞ 6.1K · ◷
Run: https://github.com/github/gh-aw/actions/runs/31454952282> Generated by 👨🍳 PR Sous Chef · gpt54 · 15.9 AIC · ⌖ 5.21 AIC · ⊞ 8.5K · ◷