Azure / Azure/mapotf

`--mptf-dir=PATH` (equals form) silently drops the flag, transform no-ops with exit 0

Open
#108 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
60
Forks
19
Avg merge
2h 2m
Merged PRs (30d)
4

Description

## Summary

`mapotf transform --mptf-dir=PATH` (equals form) silently no-ops with exit code 0 and message `Transforms applied successfully.`, even though zero transforms were actually loaded or applied. Space form (`--mptf-dir PATH`) works correctly. Pre-existing in v0.1.3 — **not** a regression from any recent PR.

This is a high-impact CLI footgun: a user copies `--mptf-dir=./configs --tf-dir=./module` from any other CLI tool's docs, sees success output, and commits an unchanged tree. The failure is silent and masks itself in CI when downstream files happen to already be canonical.

## Reproduction (verified locally on `main`)

Adding this single-case test to `cmd/args_test.go` demonstrates the bug:

```go
func TestFilterArgsEqualsFormBug(t *testing.T) {
inputArgs := []string{"mapotf", "transform", "--tf-dir=/work/module", "--mptf-dir=/work"}
mptfArgs, nonMptfArgs := cmd.FilterArgs(inputArgs)
t.Logf("mptfArgs = %#v", mptfArgs)
t.Logf("nonMptfArgs = %#v", nonMptfArgs)
}
```

Output:

```
mptfArgs = []string{"mapotf", "transform"}
nonMptfArgs = []string{"--tf-dir=/work/module", "--mptf-dir=/work"}
```

`mptfArgs` should contain `--tf-dir`, its value, `--mptf-dir`, and its value. Instead both flags are dropped from the mptf-bound argv entirely.

## Governance-side empirical matrix

From `Azure/avm-terraform-governance` verification against the v0.1.3 binary, repro'd cleanly by argv (no shell expansion involved):

| Invocation | Result |
|---|---|
| `--mptf-dir /work --tf-dir /work/module` (both space) | ✅ transforms run, "would be apply" JSON visible |
| `--mptf-dir=/work --tf-dir /work/module` (equals mptf-dir) | ❌ silent no-op |
| `--mptf-dir=/work --tf-dir=/work/module` (both equals) | ❌ silent no-op |
| `--tf-dir=/work/module --mptf-dir /work` (equals tf-dir, space mptf-dir) | ✅ transforms run |
| `--mptf-dir=/work,/work --tf-dir=/work/module` (equals + comma list) | ❌ silent no-op |
| `--mptf-dir=/work --mptf-dir=/work --tf-dir=/work/module` (equals + repeated) | ❌ silent no-op |

Pattern: any `=VALUE` token is dropped.

## Root cause

`cmd/args.go:36-51` `FilterArgs` uses exact-string map lookup on the full argv token:

```go
for i := 0; i < len(inputArgs); i++ {
arg := inputArgs[i]
if _, isSubCommand := subCommands[arg]; isSubCommand {
mptfArgs = append(mptfArgs, arg)
} else if _, isMptfVarFlag := mptfVarFlags[arg]; isMptfVarFlag {
mptfArgs = append(mptfArgs, arg)
if i != len(inputArgs)-1 && !strings.HasPrefix(inputArgs[i+1], "-") {
mptfArgs = append(mptfArgs, inputArgs[i+1])
i++
}
} else if _, isMptfShorthand := mptfShortHands[arg]; isMptfShorthand {
mptfArgs = append(mptfArgs, arg)
} else {
nonMptfArgs = append(nonMptfArgs, arg)
}
}
```

The map `mptfVarFlags` contains keys like `"--mptf-dir"` but `arg` is the full literal `"--mptf-dir=/work"`. The lookup misses, the token falls to the `else` branch, and the flag never reaches cobra.

`main.go:13-14` then mutates `os.Args = mptfArgs` before `cmd.Execute(ctx)`, so cobra parses an argv that's missing the flag entirely. `cf.mptfDirs` stays nil, the `for _, mptfDir := range mptfDirs` loop in `cmd/transform.go:79` iterates zero times, and execution drops straight to `fmt.Println("Transforms applied successfully.")` on line 91.

Same logic affects `--tf-dir=`, `--mptf-var=`, `--mptf-var-file=` — though only `--mptf-dir=` is the silent-success footgun. `--tf-dir=` falling out causes the default (working directory) to be used, which is sometimes accidentally right.

## Suggested fix

### Primary: handle `=` form in `FilterArgs`

Roughly six lines in `cmd/args.go`:

```go
for i := 0; i < len(inputArgs); i++ {
arg := inputArgs[i]
flagName := arg
hasInlineValue := false
if idx := strings.Index(arg, "="); idx > 0 {
flagName = arg[:idx]
hasInlineValue = true
}
if _, isSubCommand := subCommands[arg]; isSubCommand {
mptfArgs = append(mptfArgs, arg)
} else if _, isMptfVarFlag := mptfVarFlags[flagName]; isMptfVarFlag {
mptfArgs = append(mptfArgs, arg) // includes "=VALUE" when present
if !hasInlineValue && i != len(inputArgs)-1 && !strings.HasPrefix(inputArgs[i+1], "-") {
mptfArgs = append(mptfArgs, inputArgs[i+1])
i++
}
} else if _, isMptfShorthand := mptfShortHands[arg]; isMptfShorthand {
mptfArgs = append(mptfArgs, arg)
} else {
nonMptfArgs = append(nonMptfArgs, arg)
}
}
```

Cobra/pflag already understand `--flag=VALUE`, so just letting the token pass through unchanged is sufficient.

### Defense in depth: warn-on-empty

Independent of the parsing fix, `cmd/transform.go:79` could detect `len(mptfDirs) == 0` and either:

- Print a warning (`mapotf: no --mptf-dir provided; nothing to transform`), or
- Return an error (`mapotf transform requires at least one --mptf-dir`)

Cobra can also enforce this declaratively via `MarkFlagRequired("mptf-dir")` on `transformCmd`, mirroring what `debug.go:31` already does for the debug subcommand. This catches the bug class (zero transforms loaded) regardless of whether parsing or anything else misroutes the flag.

Recommend implementing both — primary fix unblocks the documented `=` form, defense in depth prevents the next variant of this footgun from being silent.

## Acceptance criteria

- [ ] `cmd/args_test.go` adds cases covering:
- `--mptf-dir=/path` (equals form, single value)
- `--tf-dir=/path` (equals form, StringVar variant)
- `--mptf-var=key=value` (equals form with embedded `=` in value)
- `--mptf-var-file=/path/to/file` (equals form on second StringSlice)
- `--mptf-dir=/a --mptf-dir=/b` (repeated equals form)
- [ ] After the fix, all governance-side matrix rows above produce ✅ (transforms run, exit 0).
- [ ] Either `MarkFlagRequired("mptf-dir")` is added to `transformCmd` or `transform()` returns an error when `cf.mptfDirs` is empty.
- [ ] Documentation in `--help` and `README` confirms both `--mptf-dir PATH` and `--mptf-dir=PATH` are supported.

## Impact assessment

- **Production governance configs**: NONE. `Azure/avm-terraform-governance` `pre-commit.porch.yaml:66` and `pr-check.porch.yaml:216` both use space form. Existing CI is unaffected.
- **End-user CLI usage**: HIGH risk of silent failure. Most CLI users default to `=` form via muscle memory or doc copy-paste.
- **Severity**: pre-existing in v0.1.3, no recent regression. Recommend v0.1.5 (not v0.1.4-blocking — v0.1.4 train is already in flight with #101 + #102 + #103 + #106).

## Related

Reported by `Azure/avm-terraform-governance` verification session, 2026-06-09, during PR #106 byte-canonical bonus check. Originally noted as "v0.1.5+ usability polish" after the verifier's own messages were impacted (copy-paste muscle memory used `=` form without realising).

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.