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: 2 additions & 2 deletions .github/workflows/smoke-gemini.lock.yml

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

20 changes: 19 additions & 1 deletion pkg/workflow/engine_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,13 +210,31 @@ func FormatStepWithCommandAndEnv(stepLines []string, command string, env map[str

for _, key := range envKeys {
value := env[key]
stepLines = append(stepLines, fmt.Sprintf(" %s: %s", key, value))
stepLines = append(stepLines, fmt.Sprintf(" %s: %s", key, yamlStringValue(value)))
}
}

return stepLines
}

// yamlStringValue returns a YAML-safe representation of a string value.
// If the value starts with a YAML flow indicator ('{' or '[') or other characters
// that would cause it to be misinterpreted by YAML parsers, it wraps the value
// in single quotes. Any embedded single quotes are escaped by doubling them (' becomes ”).
Comment on lines +221 to +223
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

The docstring is inaccurate/confusing: it says values may be misinterpreted due to “other characters”, but the implementation only quotes when the first byte is '{' or '['. It also describes escaping single quotes as "(' becomes ”)", which is incorrect (YAML single-quote escaping is done by doubling the single quote: ''), and the current text uses a curly quote character.

Suggested change
// If the value starts with a YAML flow indicator ('{' or '[') or other characters
// that would cause it to be misinterpreted by YAML parsers, it wraps the value
// in single quotes. Any embedded single quotes are escaped by doubling them (' becomes ).
// If the value starts with a YAML flow indicator ('{' or '['), it wraps the value
// in single quotes. Any embedded single quotes are escaped by doubling them ('
// becomes '').

Copilot uses AI. Check for mistakes.
func yamlStringValue(value string) string {
if len(value) == 0 {
return value
}
// Values starting with YAML flow indicators need quoting to be treated as strings.
// '{' would be parsed as a YAML flow mapping, '[' as a YAML flow sequence.
first := value[0]
if first != '{' && first != '[' {
return value
}
// Single-quote the value, escaping any embedded single quotes by doubling them.
return "'" + strings.ReplaceAll(value, "'", "''") + "'"
}

// FilterEnvForSecrets filters environment variables to only include allowed secrets.
// This is a security measure to ensure that only necessary secrets are passed to the execution step.
//
Expand Down
78 changes: 78 additions & 0 deletions pkg/workflow/engine_helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -363,3 +363,81 @@ func TestGetNpmBinPathSetup_NoGorootDoesNotBreakChain(t *testing.T) {
t.Errorf("Expected command chain to continue when GOROOT is empty, got: %q", result)
}
}

func TestYamlStringValue(t *testing.T) {
tests := []struct {
name string
input string
expected string
}{
{
name: "plain string unchanged",
input: "hello world",
expected: "hello world",
},
{
name: "empty string unchanged",
input: "",
expected: "",
},
{
name: "github actions expression unchanged",
input: "${{ secrets.TOKEN }}",
expected: "${{ secrets.TOKEN }}",
},
{
name: "json object gets single-quoted",
input: `{"key":"value"}`,
expected: `'{"key":"value"}'`,
},
{
name: "json array gets single-quoted",
input: `["a","b"]`,
expected: `'["a","b"]'`,
},
{
name: "json object with embedded single quote gets escaped",
input: `{"key":"it's"}`,
expected: `'{"key":"it''s"}'`,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := yamlStringValue(tt.input)
if result != tt.expected {
t.Errorf("yamlStringValue(%q) = %q, want %q", tt.input, result, tt.expected)
}
})
}
}

func TestFormatStepWithCommandAndEnvYAMLSafe(t *testing.T) {
t.Run("json env var is single-quoted for valid YAML", func(t *testing.T) {
stepLines := []string{" - name: Test step"}
env := map[string]string{
"MY_JSON": `{"key":"value","nested":{"a":1}}`,
}
result := FormatStepWithCommandAndEnv(stepLines, "echo test", env)
output := strings.Join(result, "\n")

// The JSON value must be single-quoted so YAML treats it as a string
if !strings.Contains(output, `MY_JSON: '{"key":"value","nested":{"a":1}}'`) {
t.Errorf("Expected single-quoted JSON in env, got:\n%s", output)
}
})

t.Run("github expression env var is not quoted", func(t *testing.T) {
stepLines := []string{" - name: Test step"}
env := map[string]string{
"MY_TOKEN": "${{ secrets.TOKEN }}",
}
result := FormatStepWithCommandAndEnv(stepLines, "echo test", env)
output := strings.Join(result, "\n")

// GitHub Actions expressions should not be wrapped in extra quotes
if !strings.Contains(output, "MY_TOKEN: ${{ secrets.TOKEN }}") {
t.Errorf("Expected unquoted github expression in env, got:\n%s", output)
}
})
}
12 changes: 12 additions & 0 deletions pkg/workflow/gemini_engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -513,4 +513,16 @@ func TestGenerateGeminiSettingsStep(t *testing.T) {
assert.Contains(t, content, "write_file", "Should include write_file for edit tool")
assert.Contains(t, content, "replace", "Should include replace for edit tool")
})

t.Run("GH_AW_GEMINI_BASE_CONFIG env var is single-quoted for valid YAML", func(t *testing.T) {
workflowData := &WorkflowData{
Name: "test-workflow",
Tools: map[string]any{},
}
step := engine.generateGeminiSettingsStep(workflowData)
content := strings.Join(step, "\n")

// The JSON value must be single-quoted so YAML doesn't treat it as an object
assert.Contains(t, content, "GH_AW_GEMINI_BASE_CONFIG: '", "JSON env var value must be single-quoted for valid YAML")
Comment on lines +525 to +526
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

This assertion only checks for the presence of "GH_AW_GEMINI_BASE_CONFIG: '" and would still pass if the value is missing a closing quote (or otherwise malformed). Consider strengthening the test to verify that the env var value is properly single-quoted as a whole (e.g., the line starts with the key and contains a closing single quote at the end, or by parsing the generated YAML).

Suggested change
// The JSON value must be single-quoted so YAML doesn't treat it as an object
assert.Contains(t, content, "GH_AW_GEMINI_BASE_CONFIG: '", "JSON env var value must be single-quoted for valid YAML")
// The JSON value must be single-quoted so YAML doesn't treat it as an object.
// Find the specific line and verify the entire value is enclosed in single quotes.
var envLine string
for _, line := range strings.Split(content, "\n") {
if strings.Contains(line, "GH_AW_GEMINI_BASE_CONFIG:") {
envLine = line
break
}
}
require.NotEmpty(t, envLine, "GH_AW_GEMINI_BASE_CONFIG line should be present in step")
trimmed := strings.TrimSpace(envLine)
assert.True(t, strings.HasPrefix(trimmed, "GH_AW_GEMINI_BASE_CONFIG: '"),
"JSON env var value must start with a single quote for valid YAML")
assert.True(t, strings.HasSuffix(trimmed, "'"),
"JSON env var value must end with a single quote for valid YAML")

Copilot uses AI. Check for mistakes.
})
}
Loading