Skip to content

Commit 4f07e4a

Browse files
milldrmilldr
authored
Merge branch 'main' into fix/describe-affected-deleted-crash
2 parents 360a5df + 990b763 commit 4f07e4a

File tree

7 files changed

+1839
-553
lines changed

7 files changed

+1839
-553
lines changed

‎docs/fixes/2026-03-20-processargsandflags-refactor.md‎

Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
1+
# `processArgsAndFlags` Refactor Audit — Findings and Fixes
2+
3+
**Date:** 2026-03-20
4+
5+
**Related PR:** #2225 — `refactor(cli_utils): DRY processArgsAndFlags with table-driven flag parsing, 100% unit test coverage`
6+
7+
**Severity:** Low–Medium — correctness improvements and missing test coverage
8+
9+
---
10+
11+
## What PR #2225 Did
12+
13+
PR #2225 refactored `processArgsAndFlags` — the highest-cyclomatic-complexity function in Atmos
14+
(~67 → ~15) — from 25+ copy-paste if/else chains into a table-driven design. It also fixed five
15+
real bugs:
16+
17+
1. **Boolean flags silently dropping adjacent Terraform pass-through args** — `--dry-run`,
18+
`--skip-init`, `--affected`, `--all`, `--process-templates`, `--process-functions` all
19+
unconditionally stripped `args[i+1]`, silently dropping flags like `--refresh=false`.
20+
21+
2. **`strings.Split(arg, "=")` rejecting values containing `=`** — e.g.,
22+
`--query=.tags[?env==prod]` errored. Fixed with `strings.SplitN(arg, "=", 2)`.
23+
24+
3. **`strings.HasPrefix(arg+"=", flag)` prefix collision** — logically inverted; e.g.,
25+
`--terraform-command-extra` would match `--terraform-command`. Fixed to
26+
`strings.HasPrefix(arg, flag+"=")`.
27+
28+
4. **`--from-plan` and `--identity` unconditionally consuming `args[i+1]`** — even when the
29+
next arg was another flag. Fixed to only consume when next arg doesn't start with `-`.
30+
31+
5. **`--settings-list-merge-strategy` leaking into Terraform pass-through args** — added to
32+
`commonFlags`.
33+
34+
### New Architecture
35+
36+
- **`stringFlagDefs`** — 26-entry table mapping flags to `ArgsAndFlagsInfo` field setters.
37+
- **`parseFlagValue`** — single helper for both `--flag value` and `--flag=value` forms.
38+
- **`parseIdentityFlag`** / **`parseFromPlanFlag`** — dedicated helpers for optional-value
39+
flag semantics.
40+
- **`valueTakingCommonFlags`** — set distinguishing value-taking from boolean flags for
41+
correct stripping.
42+
43+
---
44+
45+
## Audit Findings
46+
47+
### Finding 1: `valueTakingCommonFlags` stripping doesn't bounds-check `i+1`
48+
49+
**Severity:** Low (harmless at runtime, inconsistent with other checks)
50+
51+
The optional-value flags `--from-plan` and `--identity` bounds-check before stripping `i+1`:
52+
53+
```go
54+
if len(inputArgsAndFlags) > i+1 && !strings.HasPrefix(inputArgsAndFlags[i+1], "-") {
55+
indexesToRemove = append(indexesToRemove, i+1)
56+
}
57+
```
58+
59+
But the `valueTakingCommonFlags` branch does not:
60+
61+
```go
62+
} else if valueTakingCommonFlags[f] {
63+
indexesToRemove = append(indexesToRemove, i+1) // No bounds check
64+
}
65+
```
66+
67+
For flags in `stringFlagDefs`, `parseFlagValue` returns an error before stripping runs, so
68+
the OOB index is never reached. But flags only in `valueTakingCommonFlags` (e.g., `--stack`,
69+
`-s`, `--global-options`, `--kubeconfig-path`, profiler flags, `--skip`) silently add an OOB
70+
index. The OOB index is harmless (never matches during the second pass), but the inconsistency
71+
should be fixed.
72+
73+
**Fix:** Add `len(inputArgsAndFlags) > i+1` guard to the `valueTakingCommonFlags` branch.
74+
75+
### Finding 2: No test for `--global-options` stripping from `AdditionalArgsAndFlags`
76+
77+
**Severity:** Medium (coverage gap)
78+
79+
`TestProcessArgsAndFlags_GlobalOptions` verifies `GlobalOptions` is populated but doesn't
80+
assert that `--global-options` and its value are stripped from pass-through args.
81+
82+
**Fix:** Add assertions for `AdditionalArgsAndFlags` in the global options test.
83+
84+
### Finding 3: No test for equals-form boolean flag stripping
85+
86+
**Severity:** Medium (coverage gap)
87+
88+
Boolean flags like `--process-templates` can appear in equals form (`--process-templates=false`).
89+
The stripping logic handles this via `HasPrefix(arg, f+"=")`, but no test exercises the path.
90+
91+
**Fix:** Add test cases for equals-form boolean flag stripping.
92+
93+
### Finding 4: No test for flag prefix collision safety
94+
95+
**Severity:** Medium (regression prevention)
96+
97+
Real prefix collisions exist between flags:
98+
99+
| Prefix flag | Collides with |
100+
|----------------|----------------------------------------------------------------------|
101+
| `--heatmap` | `--heatmap-mode` |
102+
| `--profile` | `--profiler-enabled`, `--profiler-host/port/file/type` |
103+
| `--skip` | `--skip-init`, `--skip-planfile` |
104+
105+
The new `HasPrefix(arg, f+"=")` pattern is safe (e.g., `--heatmap-mode=foo` doesn't start
106+
with `--heatmap=`), but no test proves this. A future change could regress.
107+
108+
**Fix:** Add explicit prefix collision safety tests.
109+
110+
### Finding 5: Three parallel flag lists with no compile-time sync
111+
112+
**Severity:** Medium (maintainability)
113+
114+
Three lists must stay in sync: `commonFlags` (49 entries), `stringFlagDefs` (26 entries),
115+
`valueTakingCommonFlags` (36 entries). A flag added to one but missing from another causes
116+
silent misbehavior (parsed but not stripped, or stripped incorrectly).
117+
118+
**Fix:** Add `TestFlagListConsistency` that validates:
119+
- Every `stringFlagDefs` flag is in `commonFlags`.
120+
- Every `stringFlagDefs` flag is in `valueTakingCommonFlags`.
121+
- Every `valueTakingCommonFlags` entry is in `commonFlags`.
122+
123+
### Finding 6: Space-separated values that look like flags are double-processed
124+
125+
**Severity:** Low (pre-existing behavior, not a regression)
126+
127+
When a value-taking flag's value happens to look like another flag, both parsing paths fire:
128+
129+
```sh
130+
atmos terraform plan vpc --logs-level --logs-file /tmp/out
131+
```
132+
133+
At `i=2`: `parseFlagValue("--logs-level", ..., 2)` matches, reads `args[3]` = `"--logs-file"`.
134+
Sets `LogsLevel = "--logs-file"`.
135+
136+
At `i=3`: `parseFlagValue("--logs-file", ..., 3)` matches, reads `args[4]` = `"/tmp/out"`.
137+
Sets `LogsFile = "/tmp/out"`.
138+
139+
So `--logs-file` is consumed as the VALUE of `--logs-level` AND as a flag name. `LogsLevel`
140+
ends up as `"--logs-file"`, `LogsFile` as `"/tmp/out"`. This is the same pre-existing behavior
141+
from the old code. The `--from-plan` and `--identity` handlers correctly check if the next arg
142+
starts with `-`, but the generic `parseFlagValue` does not.
143+
144+
**Status:** Pre-existing behavior preserved by the refactor. Not fixed — would require a
145+
breaking change to `parseFlagValue` semantics. Documented for awareness.
146+
147+
### Finding 7: `parseFlagValue` match doesn't short-circuit remaining checks
148+
149+
**Severity:** Info (style, no functional impact)
150+
151+
When `parseFlagValue` matches a flag in the `stringFlagDefs` inner loop, it `break`s the inner
152+
loop but the outer `for i, arg` loop continues. The matched arg still runs through
153+
`parseIdentityFlag`, `parseFromPlanFlag`, the boolean switch, and `commonFlags` stripping.
154+
These don't match (the arg is a string flag, not identity/from-plan/boolean), so it's just
155+
unnecessary work — not a bug. Acceptable for the typical 10–20 arg invocations.
156+
157+
### Finding 8: `--help` alone triggers single-arg early return
158+
159+
**Severity:** Low (pre-existing, Cobra handles upstream)
160+
161+
```go
162+
inputArgsAndFlags: []string{"--help"}
163+
// wantSubCommand: "--help"
164+
// wantNeedHelp: false
165+
```
166+
167+
The single-arg early return (line 559) sets `SubCommand` to the literal `"--help"` and doesn't
168+
set `NeedHelp`. This is surprising but intentional — Cobra processes `--help` as a flag before
169+
this code runs. The behavior is documented in the existing test but worth noting for future
170+
maintainers.
171+
172+
---
173+
174+
## Changes Made
175+
176+
### `internal/exec/cli_utils.go`
177+
178+
- Add bounds check (`len(inputArgsAndFlags) > i+1`) to the `valueTakingCommonFlags` stripping
179+
branch, matching the pattern used by `--from-plan` / `--identity`.
180+
181+
### `internal/exec/cli_utils_helpers_test.go`
182+
183+
- Add `TestFlagListConsistency` — validates all three flag lists are in sync.
184+
- Add `TestProcessArgsAndFlags_PrefixCollisionSafety` — verifies flags with shared prefixes
185+
don't interfere during stripping or parsing.
186+
- Add `TestProcessArgsAndFlags_BooleanFlagEqualsFormStripping` — verifies equals-form boolean
187+
flags are stripped without affecting adjacent args.
188+
- Add `TestProcessArgsAndFlags_GlobalOptionsStripping` — verifies `--global-options` and its
189+
value are stripped from pass-through args.
190+
191+
---
192+
193+
## References
194+
195+
- PR #2225: `refactor(cli_utils): DRY processArgsAndFlags with table-driven flag parsing`
196+
- `internal/exec/cli_utils.go` — main implementation
197+
- `internal/exec/cli_utils_helpers_test.go` — helper unit tests
198+
- `pkg/config/const.go` — flag constant definitions

0 commit comments

Comments
 (0)