fix: prevent completions from mutating os.Args via append side effect

[veeceey]

Summary

• Fixes a bug where getCompletions() accidentally mutates os.Args by writing "--" into the shared backing array via an append with spare capacity
• Uses a three-index slice expression (finalArgs[:len:len]) to cap the slice capacity, forcing append to allocate a new array
• Adds a regression test that confirms the args slice is not corrupted during shell completion

Details

When shell completions are requested (e.g., prog __complete x), getCompletions() calls:

_ = finalCmd.ParseFlags(append(finalArgs, "--"))

finalArgs is a sub-slice of the original args (derived from os.Args[1:]), and if it has spare capacity in its backing array, append writes "--" directly into the shared memory rather than allocating a new slice. This corrupts os.Args, which users may be inspecting in their ValidArgsFunction.

The fix uses finalArgs[:len(finalArgs):len(finalArgs)] to limit the capacity to the length, guaranteeing that append allocates a fresh backing array.

The bug is most visible with TraverseChildren: true, since Traverse() returns sub-slices that preserve the original backing array's capacity.

Fixes #2257

Test plan

• Added TestCompletionDoesNotMutateArgs that fails without the fix and passes with it
• Full test suite passes (go test ./...)

[CLAassistant]

All committers have signed the CLA.

[marckhouzam]

Wow that’s interesting! Would it be doable to have a test that actually checks that os.Args is unchanged (but wrongly changed without the fix)?

[veeceey]

Good call! I reworked the test so it checks os.Args directly instead of going through SetArgs with a crafted slice.

The new TestCompletionDoesNotMutateOsArgs saves os.Args, overrides it with []string{"prog", "__completeNoDesc", "x"} (the program name has to be something other than cobra.test to bypass the test guard in ExecuteC), then runs completions without calling SetArgs — so it goes through the real os.Args[1:] path. After execution it checks that all three elements of os.Args are still intact.

Verified locally:

• With the fix: test passes, os.Args untouched
• Without the fix (reverted the three-index slice): test fails with os.Args[2] was mutated: expected "x", got "--" — exactly what the issue reporter was seeing
• Full suite still green (go test ./...)

[veeceey]

Friendly ping - just checking if the updated test approach looks good. Let me know if you'd like any further changes!

[ccoVeille]

This is about that:

Fixes a bug where getCompletions() accidentally mutates os.Args by writing "--" into the shared backing array via an append with spare capacity
Uses a three-index slice expression (finalArgs[:len:len]) to cap the slice capacity, forcing append to allocate a new array

This sounds complicated

This could be used

argsCopy := make([]string, len(finalArgs))
copy(argsCopy, finalArgs)
_ = finalCmd.ParseFlags(append(argsCopy, "--"))

Or this, that's less obvious, but pretty simple to read.

_ = finalCmd.ParseFlags(append([]string{}, finalArgs, "--"))

I feel like your solution is the most performant but it sounds a bit cryptic

[veeceey]

Yeah fair point, it is a bit cryptic if you're not used to the three-index slice idiom. The reason I went with finalArgs[:len(finalArgs):len(finalArgs)] is that it caps the capacity to the length, so when append adds "--" it's forced to allocate a new backing array instead of writing into the original one (which is os.Args). The make+copy approach works too but it's an extra allocation + copy of the whole slice when we really just need to prevent the append from mutating the original. The three-index slice is a single expression that does exactly that. That said, if the maintainers find it less readable I'm happy to switch to make+copy -- either way the bug fix is the same.

[marckhouzam]

@veeceey Agreed on it being a bit too cryptic.
I like @ccoVeille second suggestion, but the syntax is not quite right.
What do you guys think of:
_ = finalCmd.ParseFlags(append(append([]string{}, finalArgs...), "--"))
If you don't like this, the explicit array copy is also fine.

Shell completion needs to be fast but remains a user-facing operation so does not need to be optimized perfectly: copying a two or three element array is fine.

Pease put a comment before it mentioning we first copy the finalArgs slice to a new array to avoid risking modifying os.Args which currently backs finalArgs.

[alexandear]

How about copying the trimmedArgs slice at the beginning of the function getCompletions?

Instead of

	trimmedArgs := args[:len(args)-1]

use

	trimmedArgs := make([]string, len(args)-1)
	copy(trimmedArgs, args[:len(args)-1])

In the future, we can change to slices.Clone.

[marckhouzam]

Nit: please use "root" instead of "prog" just to be consistent with the name of the root command below. I has no impact but may just slow down someone looking at it.

[veeceey]

@alexandear good call, that's much cleaner — copying trimmedArgs at the top of getCompletions means we don't have to worry about any downstream appends mutating the original. Went with your make+copy approach and reverted the three-index slice back to a plain append(finalArgs, "--").

Also renamed "prog" to "root" per @marckhouzam's nit.

[veeceey]

@alexandear good idea — using slices.Clone (or the copy approach for now) is much safer since it prevents any future mutation of the original args. I'll update it. Thanks!