Skip to content

Commit 722a5c3

Browse files
Fix double-hyphen (--) parsing in CLI commands (#1990)
* fix double-hyphen * [autofix.ci] apply automated fixes * address comments --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
1 parent 10080ca commit 722a5c3

File tree

9 files changed

+521
-21
lines changed

9 files changed

+521
-21
lines changed

NOTICE

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ APACHE 2.0 LICENSED DEPENDENCIES
119119

120120
- github.com/aws/aws-sdk-go-v2/feature/s3/manager
121121
License: Apache-2.0
122-
URL: https://github.com/aws/aws-sdk-go-v2/blob/feature/s3/manager/v1.20.18/feature/s3/manager/LICENSE.txt
122+
URL: https://github.com/aws/aws-sdk-go-v2/blob/feature/s3/manager/v1.20.19/feature/s3/manager/LICENSE.txt
123123

124124
- github.com/aws/aws-sdk-go-v2/internal/configsources
125125
License: Apache-2.0
@@ -1126,7 +1126,7 @@ MIT LICENSED DEPENDENCIES
11261126

11271127
- github.com/charmbracelet/x/ansi
11281128
License: MIT
1129-
URL: https://github.com/charmbracelet/x/blob/ansi/v0.11.3/ansi/LICENSE
1129+
URL: https://github.com/charmbracelet/x/blob/ansi/v0.11.4/ansi/LICENSE
11301130

11311131
- github.com/charmbracelet/x/cellbuf
11321132
License: MIT
@@ -1146,7 +1146,7 @@ MIT LICENSED DEPENDENCIES
11461146

11471147
- github.com/clipperhouse/displaywidth
11481148
License: MIT
1149-
URL: https://github.com/clipperhouse/displaywidth/blob/v0.6.1/LICENSE
1149+
URL: https://github.com/clipperhouse/displaywidth/blob/v0.7.0/LICENSE
11501150

11511151
- github.com/clipperhouse/stringish
11521152
License: MIT

go.mod

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

go.sum

Lines changed: 6 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

internal/exec/workflow_utils_test.go

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"errors"
55
"os"
66
"path/filepath"
7+
"slices"
78
"strings"
89
"testing"
910

@@ -1542,6 +1543,120 @@ func TestEnsureWorkflowToolchainDependencies_EmptyWorkflowDef(t *testing.T) {
15421543
assert.Empty(t, path)
15431544
}
15441545

1546+
// TestDoubleHyphenStackInsertion tests the stack insertion logic with double-hyphen separator.
1547+
// This is the fix for GitHub issue #1967 where commands with -- separator were incorrectly parsed.
1548+
func TestDoubleHyphenStackInsertion(t *testing.T) {
1549+
tests := []struct {
1550+
name string
1551+
command string
1552+
workflowStack string
1553+
expectedArgs []string
1554+
}{
1555+
{
1556+
name: "insert stack before double-hyphen",
1557+
command: "terraform plan vpc -- -consolidate-warnings=false",
1558+
workflowStack: "nonprod",
1559+
expectedArgs: []string{
1560+
"terraform", "plan", "vpc", "-s", "nonprod", "--", "-consolidate-warnings=false",
1561+
},
1562+
},
1563+
{
1564+
name: "command with stack already - workflow stack should be added",
1565+
command: "terraform plan vpc --stack dev -- -consolidate-warnings=false",
1566+
workflowStack: "nonprod",
1567+
expectedArgs: []string{
1568+
"terraform", "plan", "vpc", "--stack", "dev", "-s", "nonprod", "--", "-consolidate-warnings=false",
1569+
},
1570+
},
1571+
{
1572+
name: "no double-hyphen - append at end",
1573+
command: "terraform plan vpc",
1574+
workflowStack: "nonprod",
1575+
expectedArgs: []string{
1576+
"terraform", "plan", "vpc", "-s", "nonprod",
1577+
},
1578+
},
1579+
{
1580+
name: "multiple args after double-hyphen",
1581+
command: "terraform plan vpc -- -var=foo=bar -var=baz=qux",
1582+
workflowStack: "nonprod",
1583+
expectedArgs: []string{
1584+
"terraform", "plan", "vpc", "-s", "nonprod", "--", "-var=foo=bar", "-var=baz=qux",
1585+
},
1586+
},
1587+
{
1588+
name: "complex component name with hyphens",
1589+
command: "terraform plan 10-static-web-app-no-frontdoor -- -consolidate-warnings=false",
1590+
workflowStack: "trialintelx-eastus-sbx",
1591+
expectedArgs: []string{
1592+
"terraform", "plan", "10-static-web-app-no-frontdoor", "-s", "trialintelx-eastus-sbx", "--", "-consolidate-warnings=false",
1593+
},
1594+
},
1595+
{
1596+
name: "empty workflow stack - no insertion",
1597+
command: "terraform plan vpc -- -consolidate-warnings=false",
1598+
workflowStack: "",
1599+
expectedArgs: []string{
1600+
"terraform", "plan", "vpc", "--", "-consolidate-warnings=false",
1601+
},
1602+
},
1603+
}
1604+
1605+
for _, tt := range tests {
1606+
t.Run(tt.name, func(t *testing.T) {
1607+
// Parse command using shell.Fields (same as workflow code).
1608+
args, err := shell.Fields(tt.command, nil)
1609+
require.NoError(t, err)
1610+
1611+
// Apply the same stack insertion logic as in workflow_utils.go line 374-383.
1612+
// Uses slices.Index to stay in sync with production code.
1613+
if tt.workflowStack != "" {
1614+
if idx := slices.Index(args, "--"); idx != -1 {
1615+
// Insert before the "--".
1616+
args = append(args[:idx], append([]string{"-s", tt.workflowStack}, args[idx:]...)...)
1617+
} else {
1618+
// Append at the end.
1619+
args = append(args, "-s", tt.workflowStack)
1620+
}
1621+
}
1622+
1623+
assert.Equal(t, tt.expectedArgs, args)
1624+
})
1625+
}
1626+
}
1627+
1628+
// TestDoubleHyphenIssue1967 is a specific test for GitHub issue #1967.
1629+
// It reproduces the exact scenario from the bug report where the stack value was corrupted.
1630+
func TestDoubleHyphenIssue1967(t *testing.T) {
1631+
// This is the exact command from the bug report.
1632+
command := "terraform plan 10-static-web-app-no-frontdoor --stack trialintelx-eastus-sbx -- -consolidate-warnings=false"
1633+
1634+
// Parse using shell.Fields.
1635+
args, err := shell.Fields(command, nil)
1636+
require.NoError(t, err)
1637+
1638+
// Verify the parsed args are correct.
1639+
expectedParsedArgs := []string{
1640+
"terraform", "plan", "10-static-web-app-no-frontdoor",
1641+
"--stack", "trialintelx-eastus-sbx",
1642+
"--", "-consolidate-warnings=false",
1643+
}
1644+
assert.Equal(t, expectedParsedArgs, args, "shell.Fields should correctly parse the command")
1645+
1646+
// Verify each argument doesn't have corruption.
1647+
for i, arg := range args {
1648+
t.Logf("arg[%d] = %q (len=%d)", i, arg, len(arg))
1649+
}
1650+
1651+
// Specifically check the stack value is preserved.
1652+
assert.Equal(t, "--stack", args[3], "fourth arg should be --stack")
1653+
assert.Equal(t, "trialintelx-eastus-sbx", args[4], "fifth arg should be the stack value")
1654+
1655+
// Verify the terraform flag after -- is preserved.
1656+
assert.Equal(t, "--", args[5], "sixth arg should be --")
1657+
assert.Equal(t, "-consolidate-warnings=false", args[6], "seventh arg should be -consolidate-warnings=false")
1658+
}
1659+
15451660
// TestEnsureWorkflowToolchainDependencies_WithToolVersionsFile tests with .tool-versions file present.
15461661
func TestEnsureWorkflowToolchainDependencies_WithToolVersionsFile(t *testing.T) {
15471662
tempDir := t.TempDir()

pkg/flags/compat/compatibility_flags.go

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ const (
2727
const (
2828
// FlagPrefix is the single dash prefix for flags.
2929
flagPrefix = "-"
30+
// EndOfOptionsMarker is the POSIX standard "--" that separates options from positional arguments.
31+
endOfOptionsMarker = "--"
3032
)
3133

3234
// CompatibilityFlag defines how a single compatibility flag should be handled.
@@ -87,7 +89,7 @@ func (t *CompatibilityFlagTranslator) ValidateNoConflicts(cmd *cobra.Command) er
8789

8890
for compatFlag := range t.flagMap {
8991
// Only check single-dash flags (potential shorthands).
90-
if !strings.HasPrefix(compatFlag, flagPrefix) || strings.HasPrefix(compatFlag, "--") {
92+
if !strings.HasPrefix(compatFlag, flagPrefix) || strings.HasPrefix(compatFlag, endOfOptionsMarker) {
9193
continue
9294
}
9395

@@ -125,7 +127,7 @@ func (t *CompatibilityFlagTranslator) ValidateTargetsInArgs(cmd *cobra.Command,
125127
}
126128

127129
// Skip already-modern flags (--).
128-
if strings.HasPrefix(arg, "--") {
130+
if strings.HasPrefix(arg, endOfOptionsMarker) {
129131
continue
130132
}
131133

@@ -177,26 +179,40 @@ func (t *CompatibilityFlagTranslator) Translate(args []string) (atmosArgs []stri
177179
for i := 0; i < len(args); i++ {
178180
arg := args[i]
179181

180-
// Not a flag - it's a positional arg
182+
// Not a flag - it's a positional arg.
181183
if !strings.HasPrefix(arg, flagPrefix) {
182184
atmosArgs = append(atmosArgs, arg)
183185
continue
184186
}
185187

186-
// Already modern format (--flag) - pass to Atmos
187-
if strings.HasPrefix(arg, "--") {
188+
// Handle "--" end-of-options marker per POSIX convention.
189+
// Everything after "--" is passed through to the subprocess, not parsed by Cobra.
190+
// This fixes issue #1967 where args like "-consolidate-warnings=false" after "--"
191+
// were incorrectly parsed by Cobra/pflag.
192+
// We include the "--" in atmosArgs so Cobra knows to stop parsing flags,
193+
// but everything after it goes to separatedArgs to bypass Cobra completely.
194+
if arg == endOfOptionsMarker {
195+
atmosArgs = append(atmosArgs, endOfOptionsMarker)
196+
if i+1 < len(args) {
197+
separatedArgs = append(separatedArgs, args[i+1:]...)
198+
}
199+
break
200+
}
201+
202+
// Already modern format (--flag) - pass to Atmos.
203+
if strings.HasPrefix(arg, endOfOptionsMarker) {
188204
atmosArgs = append(atmosArgs, arg)
189205
continue
190206
}
191207

192-
// Single-dash flag - check for compatibility flag
208+
// Single-dash flag - check for compatibility flag.
193209
translated, consumed := t.translateSingleDashFlag(args, i)
194210

195-
// Add translated args to appropriate destination
211+
// Add translated args to appropriate destination.
196212
atmosArgs = append(atmosArgs, translated.atmosArgs...)
197213
separatedArgs = append(separatedArgs, translated.separatedArgs...)
198214

199-
// Skip consumed args
215+
// Skip consumed args.
200216
i += consumed
201217
}
202218

pkg/flags/compat/compatibility_flags_test.go

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -449,6 +449,107 @@ func TestCompatibilityFlagTranslator_EdgeCases(t *testing.T) {
449449
}
450450
}
451451

452+
// TestCompatibilityFlagTranslator_DoubleHyphenSeparator tests the "--" end-of-options handling.
453+
// This is the fix for GitHub issue #1967 where args after "--" were incorrectly parsed.
454+
func TestCompatibilityFlagTranslator_DoubleHyphenSeparator(t *testing.T) {
455+
tests := []struct {
456+
name string
457+
input []string
458+
expected translationResult
459+
}{
460+
{
461+
name: "double-hyphen with terraform flags after",
462+
input: []string{"terraform", "plan", "vpc", "--stack", "nonprod", "--", "-consolidate-warnings=false"},
463+
expected: translationResult{
464+
atmosArgs: []string{"terraform", "plan", "vpc", "--stack", "nonprod", "--"},
465+
separatedArgs: []string{"-consolidate-warnings=false"},
466+
},
467+
},
468+
{
469+
name: "double-hyphen with multiple args after",
470+
input: []string{"terraform", "plan", "vpc", "-s", "nonprod", "--", "-var=foo=bar", "-var=baz=qux"},
471+
expected: translationResult{
472+
atmosArgs: []string{"terraform", "plan", "vpc", "-s", "nonprod", "--"},
473+
separatedArgs: []string{"-var=foo=bar", "-var=baz=qux"},
474+
},
475+
},
476+
{
477+
name: "double-hyphen at end (no args after)",
478+
input: []string{"terraform", "plan", "vpc", "--stack", "nonprod", "--"},
479+
expected: translationResult{
480+
atmosArgs: []string{"terraform", "plan", "vpc", "--stack", "nonprod", "--"},
481+
separatedArgs: []string{},
482+
},
483+
},
484+
{
485+
name: "no double-hyphen",
486+
input: []string{"terraform", "plan", "vpc", "--stack", "nonprod"},
487+
expected: translationResult{
488+
atmosArgs: []string{"terraform", "plan", "vpc", "--stack", "nonprod"},
489+
separatedArgs: []string{},
490+
},
491+
},
492+
{
493+
name: "double-hyphen with compat flag after (should go to separated)",
494+
input: []string{"terraform", "plan", "vpc", "--", "-var", "foo=bar"},
495+
expected: translationResult{
496+
atmosArgs: []string{"terraform", "plan", "vpc", "--"},
497+
separatedArgs: []string{"-var", "foo=bar"},
498+
},
499+
},
500+
{
501+
name: "complex component name with hyphens (issue 1967 exact scenario)",
502+
input: []string{"terraform", "plan", "10-static-web-app-no-frontdoor", "--stack", "trialintelx-eastus-sbx", "--", "-consolidate-warnings=false"},
503+
expected: translationResult{
504+
atmosArgs: []string{"terraform", "plan", "10-static-web-app-no-frontdoor", "--stack", "trialintelx-eastus-sbx", "--"},
505+
separatedArgs: []string{"-consolidate-warnings=false"},
506+
},
507+
},
508+
{
509+
name: "double-hyphen with -s flag after (should NOT be parsed as stack)",
510+
input: []string{"terraform", "plan", "vpc", "--stack", "prod", "--", "-s", "wrongstack"},
511+
expected: translationResult{
512+
atmosArgs: []string{"terraform", "plan", "vpc", "--stack", "prod", "--"},
513+
separatedArgs: []string{"-s", "wrongstack"},
514+
},
515+
},
516+
{
517+
name: "multiple double-hyphens (only first matters)",
518+
input: []string{"terraform", "plan", "vpc", "--", "-var=a", "--", "-var=b"},
519+
expected: translationResult{
520+
atmosArgs: []string{"terraform", "plan", "vpc", "--"},
521+
separatedArgs: []string{"-var=a", "--", "-var=b"},
522+
},
523+
},
524+
{
525+
name: "double-hyphen as only arg after command",
526+
input: []string{"terraform", "plan", "--"},
527+
expected: translationResult{
528+
atmosArgs: []string{"terraform", "plan", "--"},
529+
separatedArgs: []string{},
530+
},
531+
},
532+
{
533+
name: "double-hyphen first in args",
534+
input: []string{"--", "-var=foo"},
535+
expected: translationResult{
536+
atmosArgs: []string{"--"},
537+
separatedArgs: []string{"-var=foo"},
538+
},
539+
},
540+
}
541+
542+
for _, tt := range tests {
543+
t.Run(tt.name, func(t *testing.T) {
544+
translator := buildTestCompatibilityTranslator()
545+
atmosArgs, separatedArgs := translator.Translate(tt.input)
546+
547+
assert.Equal(t, tt.expected.atmosArgs, atmosArgs, "atmosArgs mismatch")
548+
assert.Equal(t, tt.expected.separatedArgs, separatedArgs, "separatedArgs mismatch")
549+
})
550+
}
551+
}
552+
452553
func TestCompatibilityFlagTranslator_ValidateTargets(t *testing.T) {
453554
tests := []struct {
454555
name string

0 commit comments

Comments
 (0)