feat: auto-generate --allow-host-service-ports from services: port mappings#23760
feat: auto-generate --allow-host-service-ports from services: port mappings#23760
Conversation
…ppings (#23756) When a workflow uses GitHub Actions services: with port mappings, the agent running inside AWF's isolated network cannot reach service containers without explicit port configuration. The compiler now automatically detects service ports and emits ${{ job.services.<id>.ports['<port>'] }} expressions as --allow-host-service-ports in the AWF command. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds compiler support to auto-generate AWF --allow-host-service-ports arguments from services:<id>.ports mappings by parsing port specs and emitting GitHub Actions runtime expressions.
Changes:
- Introduces service port parsing + expression generation (
${{ job.services.<id>.ports['<containerPort>'] }}) with range expansion and UDP warnings. - Plumbs generated expressions through
WorkflowDataand service-merge compilation flow. - Emits the generated
--allow-host-service-portsargument into the AWF command as a raw (non-shell-escaped) argument.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/service_ports.go | New parsing/extraction logic for services ports and expression generation. |
| pkg/workflow/service_ports_test.go | New unit tests covering port syntax variants and ordering behavior. |
| pkg/workflow/compiler_types.go | Adds ServicePortExpressions to WorkflowData. |
| pkg/workflow/compiler_orchestrator_workflow.go | Extracts/records service port expressions after services merge. |
| pkg/workflow/awf_helpers.go | Appends --allow-host-service-ports to the AWF command when expressions exist. |
Comments suppressed due to low confidence (3)
pkg/workflow/service_ports.go:100
- The expression uses dot-notation with the raw service ID (job.services....). GitHub Actions expression dot access breaks for IDs containing characters like '-' and can also become unsafe if IDs contain characters that interfere with quoting. Consider generating bracket-notation for non-identifier service IDs (job.services['']) and validating/escaping IDs before embedding them into the command string.
for _, portSpec := range portsList {
containerPorts, portWarnings := parsePortSpec(portSpec)
for _, w := range portWarnings {
warnings = append(warnings, fmt.Sprintf("service %q: %s", serviceID, w))
}
for _, cp := range containerPorts {
expr := fmt.Sprintf("${{ job.services.%s.ports['%d'] }}", serviceID, cp)
expressions = append(expressions, expr)
}
pkg/workflow/service_ports.go:133
- Numeric port specs: float64 is truncated to int without checking that it’s an integer value, and no range validation is performed. This can silently accept invalid YAML like 5432.5 or ports outside 1-65535. Consider rejecting non-integer floats and validating the port range; also consider handling other numeric types (int64/uint64) that YAML decoders commonly produce.
func parsePortSpec(spec any) ([]int, []string) {
var portStr string
switch v := spec.(type) {
case int:
return []int{v}, nil
case float64:
// YAML may parse unquoted numbers as float64
return []int{int(v)}, nil
case string:
pkg/workflow/service_ports.go:153
- Protocol parsing only treats "udp" specially; any other suffix (e.g., "sctp") will be treated as TCP, which conflicts with the stated "AWF only supports TCP" behavior. Consider warning+skipping any protocol that isn’t empty/"tcp".
// Check for protocol suffix
protocol := "tcp"
if idx := strings.LastIndex(portStr, "/"); idx != -1 {
protocol = strings.ToLower(portStr[idx+1:])
portStr = portStr[:idx]
}
if protocol == "udp" {
return nil, []string{fmt.Sprintf("UDP port %q skipped; AWF only supports TCP", portStr)}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/workflow/service_ports.go
Outdated
| "strings" | ||
|
|
||
| "github.com/github/gh-aw/pkg/logger" | ||
| goyaml "gopkg.in/yaml.v3" |
There was a problem hiding this comment.
This file uses gopkg.in/yaml.v3 for parsing, but the rest of pkg/workflow consistently uses github.com/goccy/go-yaml for YAML (e.g., pkg/workflow/frontmatter_extraction_yaml.go:11). Mixing YAML implementations can lead to different decoded Go types (e.g., int vs int64/uint64) and inconsistent behavior; consider switching this to goccy/go-yaml for consistency.
| goyaml "gopkg.in/yaml.v3" | |
| goyaml "github.com/goccy/go-yaml" |
| // Extract service port expressions for AWF --allow-host-service-ports | ||
| if workflowData.Services != "" { | ||
| expressions, warnings := ExtractServicePortExpressions(workflowData.Services) | ||
| workflowData.ServicePortExpressions = expressions | ||
| for _, w := range warnings { | ||
| orchestratorWorkflowLog.Printf("Warning: %s", w) | ||
| } |
There was a problem hiding this comment.
These are meant to be compile-time warnings, but they’re currently only emitted via the debug logger (visible only when DEBUG/ACTIONS_RUNNER_DEBUG is enabled). To match existing warning behavior in this package (e.g., pkg/workflow/action_pins.go prints console-formatted warnings to stderr), consider emitting these warnings through the same user-visible warning mechanism instead of (or in addition to) debug logs.
| func TestExtractServicePortExpressions(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| servicesYAML string | ||
| expectedResult string | ||
| expectedWarnings []string | ||
| }{ | ||
| { | ||
| name: "empty services YAML", | ||
| servicesYAML: "", | ||
| expectedResult: "", | ||
| }, | ||
| { | ||
| name: "single service with single port", | ||
| servicesYAML: `services: | ||
| postgres: | ||
| image: postgres:15 | ||
| ports: | ||
| - 5432:5432 | ||
| `, | ||
| expectedResult: "${{ job.services.postgres.ports['5432'] }}", | ||
| }, | ||
| { | ||
| name: "multiple services with ports", | ||
| servicesYAML: `services: | ||
| postgres: | ||
| image: postgres:15 | ||
| ports: | ||
| - 5432:5432 | ||
| redis: | ||
| image: redis:7 | ||
| ports: | ||
| - 6379:6379 | ||
| `, | ||
| expectedResult: "${{ job.services.postgres.ports['5432'] }},${{ job.services.redis.ports['6379'] }}", | ||
| }, | ||
| { |
There was a problem hiding this comment.
Consider adding a test case for service IDs that require bracket-notation in GitHub Actions expressions (e.g., a service named "my-postgres"), since the generated expression differs from the simple dot-notation cases covered here.
pkg/workflow/service_ports_test.go
Outdated
| func servicePortsContains(s, substr string) bool { | ||
| return len(s) >= len(substr) && servicePortsSearchSubstring(s, substr) | ||
| } | ||
|
|
||
| func servicePortsSearchSubstring(s, substr string) bool { | ||
| for i := 0; i <= len(s)-len(substr); i++ { | ||
| if s[i:i+len(substr)] == substr { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
The helper functions servicePortsContains/servicePortsSearchSubstring reimplement substring search; using strings.Contains would be simpler and less error-prone. This would also let you remove a small amount of custom code from the test file.
- Switch from gopkg.in/yaml.v3 to goccy/go-yaml for consistency with the rest of pkg/workflow, adding uint64/int64 type cases for goccy's integer decoding behavior - Replace strings.Index with strings.Cut to satisfy the modernize linter - Surface invalid ports format as a user-visible warning instead of only logging at debug level - Replace custom substring search helpers with strings.Contains - Add test cases for hyphenated service IDs and invalid ports format Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Emit service port warnings through console.FormatWarningMessage to stderr, matching the existing warning pattern used by action_pins.go and other compilation warnings in the package. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds a smoke-service-ports workflow that validates the --allow-host-service-ports feature end-to-end. The workflow declares a Redis service container and instructs the agent to connect to Redis on localhost:6379, verifying PING/PONG, SET/GET round-trip, and INFO server queries. Closes part of #23756 (smoke test validation). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
pkg/workflow/service_ports.go:159
- Protocol parsing currently treats any unknown suffix as supported (e.g. "5432/sctp" will be accepted as TCP). GitHub Actions only supports tcp/udp here; please validate the suffix and emit a warning + skip (or default explicitly to tcp only when suffix is empty or "tcp").
// Check for protocol suffix
protocol := "tcp"
if idx := strings.LastIndex(portStr, "/"); idx != -1 {
protocol = strings.ToLower(portStr[idx+1:])
portStr = portStr[:idx]
}
if protocol == "udp" {
return nil, []string{fmt.Sprintf("UDP port %q skipped; AWF only supports TCP", portStr)}
}
pkg/workflow/service_ports.go:139
- Numeric port specs are cast to int without validating range or type-safety. This can silently accept invalid values (e.g. float64(5432.9) truncates to 5432) or overflow when converting uint64/int64 to int. Consider validating the value is an integer and within 1-65535 (and rejecting negatives/zero) before returning it as a port.
switch v := spec.(type) {
case int:
return []int{v}, nil
case uint64:
// goccy/go-yaml decodes unquoted integers as uint64
return []int{int(v)}, nil
case int64:
return []int{int(v)}, nil
case float64:
// Some YAML libraries parse unquoted numbers as float64
return []int{int(v)}, nil
case string:
pkg/workflow/service_ports.go:199
- String port parsing doesn’t validate the resulting port numbers are within the valid TCP/UDP range (1-65535). As written, specs like "0", "70000", or negative ranges will produce expressions that can’t resolve correctly at runtime. Add bounds checks for both single ports and each expanded range value, and return a warning when out of range.
// Check for port range (e.g., "6000-6010")
if startStr, endStr, found := strings.Cut(containerPart, "-"); found {
start, err1 := strconv.Atoi(startStr)
end, err2 := strconv.Atoi(endStr)
if err1 != nil || err2 != nil {
return nil, []string{fmt.Sprintf("invalid port range %q", containerPart)}
}
if end < start {
return nil, []string{fmt.Sprintf("invalid port range %q: end < start", containerPart)}
}
count := end - start + 1
if count > maxPortRangeExpansion {
return nil, []string{fmt.Sprintf("port range %q expands to %d ports, exceeding cap of %d", containerPart, count, maxPortRangeExpansion)}
}
ports := make([]int, 0, count)
for p := start; p <= end; p++ {
ports = append(ports, p)
}
return ports, nil
}
// Single port
port, err := strconv.Atoi(containerPart)
if err != nil {
return nil, []string{fmt.Sprintf("invalid port number %q", containerPart)}
}
return []int{port}, nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/workflow/service_ports.go
Outdated
| warnings = append(warnings, fmt.Sprintf("service %q: %s", serviceID, w)) | ||
| } | ||
| for _, cp := range containerPorts { | ||
| expr := fmt.Sprintf("${{ job.services.%s.ports['%d'] }}", serviceID, cp) |
There was a problem hiding this comment.
GitHub Actions expression dot-notation will break for service IDs that aren’t valid identifiers (e.g. IDs containing '-' or starting with a digit). The generated expression should use bracket notation for the service key (and escape any embedded single quotes) so it works for all valid workflow service IDs: ${{ job.services['<id>'].ports['<port>'] }}.
This issue also appears in the following locations of the same file:
- line 128
- line 150
- line 169
| expr := fmt.Sprintf("${{ job.services.%s.ports['%d'] }}", serviceID, cp) | |
| escapedServiceID := strings.ReplaceAll(serviceID, "'", "''") | |
| expr := fmt.Sprintf("${{ job.services['%s'].ports['%d'] }}", escapedServiceID, cp) |
pkg/workflow/service_ports_test.go
Outdated
| ports: | ||
| - 5432:5432 | ||
| `, | ||
| expectedResult: "${{ job.services.my-postgres.ports['5432'] }}", |
There was a problem hiding this comment.
This test codifies dot-notation for a hyphenated service ID, but GitHub Actions expressions require bracket notation for keys containing '-' (e.g. job.services['my-postgres']...). Update the expected expression (and the generator) so service IDs with '-' are handled correctly.
| expectedResult: "${{ job.services.my-postgres.ports['5432'] }}", | |
| expectedResult: "${{ job.services['my-postgres'].ports['5432'] }}", |
| workflowData.ServicePortExpressions = expressions | ||
| for _, w := range warnings { | ||
| orchestratorWorkflowLog.Printf("Warning: %s", w) | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(w)) |
There was a problem hiding this comment.
Warnings are emitted to stderr here but the compiler’s warning counter isn’t incremented. To keep summary/exit behavior consistent with other warnings (e.g. pkg/workflow/compiler.go), call c.IncrementWarningCount() for each emitted warning.
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(w)) | |
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(w)) | |
| c.IncrementWarningCount() |
There was a problem hiding this comment.
Agreed — incrementing c.IncrementWarningCount() is important for consistent warning summary behavior across the compiler. Good catch! 🔍
📰 BREAKING: Report filed by Smoke Copilot
|
❌ Service ports validation failed! Smoke Service Ports could not reach Redis: failed |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
This comment has been minimized.
This comment has been minimized.
…ning count - Use bracket notation (job.services['id']) for all service IDs to handle hyphens and other special characters in GitHub Actions expressions - Add port range validation (1-65535) for all port spec types - Reject non-integer float64 values and unknown protocols (not just UDP) - Increment compiler warning count for service port warnings - Add test cases for out-of-range ports, unknown protocols, non-integer floats - Recompile smoke-service-ports lock file with updated expression format Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Smoke Test 23817988098 — @Mossaka
Overall:
|
There was a problem hiding this comment.
Smoke test review of PR #23760. Key observations: service IDs with hyphens need bracket notation in expressions, and warning counter should be incremented when warnings are emitted to stderr.
📰 BREAKING: Report filed by Smoke Copilot
pkg/workflow/service_ports.go
Outdated
| warnings = append(warnings, fmt.Sprintf("service %q: %s", serviceID, w)) | ||
| } | ||
| for _, cp := range containerPorts { | ||
| expr := fmt.Sprintf("${{ job.services.%s.ports['%d'] }}", serviceID, cp) |
There was a problem hiding this comment.
Service IDs containing hyphens or other non-identifier characters (e.g., my-postgres) will produce invalid GitHub Actions expressions with dot notation. Consider using bracket notation: $\{\{ job.services['id'].ports['port'] }} for all service IDs to ensure correctness.
| workflowData.ServicePortExpressions = expressions | ||
| for _, w := range warnings { | ||
| orchestratorWorkflowLog.Printf("Warning: %s", w) | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(w)) |
There was a problem hiding this comment.
Warnings emitted here should also call c.IncrementWarningCount() to keep the warning counter consistent with the rest of the compiler's warning tracking behavior.
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
💥 [THE END] — Illustrated by Smoke Claude
pkg/workflow/service_ports.go
Outdated
| continue | ||
| } | ||
|
|
||
| ports, hasPorts := svcMap["ports"] |
There was a problem hiding this comment.
🤖 Smoke test review comment #1: The silent skip on invalid ports mapping is a good candidate for a user-visible warning. The warnings slice is already in scope — appending a message here would surface this misconfiguration during gh aw compile.
| for _, w := range warnings { | ||
| orchestratorWorkflowLog.Printf("Warning: %s", w) | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(w)) | ||
| c.IncrementWarningCount() |
There was a problem hiding this comment.
🤖 Smoke test review comment #2: Debug-only log here means users won't see the warning unless ACTIONS_RUNNER_DEBUG is set. Consider also emitting to stderr so this is visible during normal compilation, consistent with other warnings in this package.
There was a problem hiding this comment.
Agreed — incrementing c.IncrementWarningCount() is important for consistent warning summary behavior across the compiler. Good catch! 🔍
📰 BREAKING: Report filed by Smoke Copilot
The --allow-host-service-ports flag was added to AWF's main branch but not yet released in v0.25.1. The smoke test needs AWF built from source to validate this feature. Replaces install_awf_binary.sh with a checkout-and-build step, and locally builds container images tagged as 0.25.1 so --skip-pull uses them. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
|
Hey This PR looks ready for maintainer review. 🎉 Contribution check: 🟢 Aligned · tests ✅ · description ✅ · focused ✅ · no new deps
|
|
@copilot review changes, review comments |
All review comments have been addressed across commits
CI is passing on the latest commit. |
…7-438a-9fa7-b3a86d41180a Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| schedule: every 12h | ||
| pull_request: | ||
| types: [labeled] | ||
| names: ["smoke"] |
There was a problem hiding this comment.
Done in 816a57a — removed pull_request trigger and changed the schedule to daily.
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/bee817a0-958d-47c6-9430-42675e50b67e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review comments |
All review comments are addressed. Here's the current state:
All 33 unit tests pass. |
|
…or service ports Agent-Logs-Url: https://github.com/github/gh-aw/sessions/1a8413b1-7fb1-4e16-ba81-98cae2550ebb Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in Typed structs ( Test fixture ( Integration tests (
|
Summary
Closes #23756
services:configuration and generates${{ job.services['<id>'].ports['<port>'] }}expressions for AWF's--allow-host-service-portsflagjob.services['my-postgres']) with single-quote escaping, ensuring correctness for hyphenated names and other non-identifier characters5432:5432), dynamic host ports (5432), remapped ports (49152:5432), TCP protocol suffix, port ranges with expansion cap, and integer valuesservicesYAMLWrapper/serviceContainerConfig) withyaml:field annotations rather thanmap[string]anytype assertionsFiles changed
pkg/workflow/service_ports.gopkg/workflow/service_ports_test.gopkg/workflow/compiler_types.goServicePortExpressionsfield toWorkflowDatapkg/workflow/compiler_orchestrator_workflow.goIncrementWarningCount()pkg/workflow/awf_helpers.go--allow-host-service-portsas raw arg in AWF commandpkg/cli/workflows/test-service-ports.mdpkg/cli/compile_service_ports_integration_test.go.github/workflows/smoke-service-ports.mdExample
Input services:
Compiled AWF command includes:
Test plan
pkg/cli/compile_service_ports_integration_test.go): multi-service lock file output, no-services case, hyphenated ID bracket notationsmoke-service-ports.md) runs daily against a live Redis service containermake buildpassesmake test-unitpassesmake fmtpassesmake lintpasses for changed files