cli-flag-review

name	cli-flag-review
description	Audit and optimize CLI flag definitions across all skeptic subcommands. Reviews help text quality, global-vs-local flag modeling, short flag coverage, shorthand config-precedence safety, help formatter consistency, and test isolation. Use when adding flags, reviewing CLI ergonomics, refactoring subcommand interfaces, or debugging flag override behavior.

CLI Flag Review

Systematic audit of every CLI flag in skeptic. Covers help text, short aliases, global passthrough decisions, config-precedence safety, help output formatting, test isolation, and implementation correctness.

Workflow

1. Inventory — collect every fs.*Var registration across cmd/skeptic/ and internal/ subcommand entry points.
2. Help text audit — apply the quality rules below to every help string.
3. Short flag audit — assign single-letter aliases per the priority table.
4. Shorthand precedence audit — verify every shorthand is registered in the config-layer maps (see below).
5. Help formatter audit — verify printScanFlags and scanFlagShorthands are in sync with registrations.
6. Global vs local audit — decide whether each flag should be global (pre-parsed in extractGlobalFlags) or local to its subcommand FlagSet.
7. Test isolation audit — verify every test calling run() or ResolveConfigLayers() sets XDG_DATA_HOME.
8. Implement — edit flag registrations, update maps, run go fmt && go vet && go test -race ./....

Help Text Quality Rules

Each help string must:

• Start lowercase (Go flag convention)
• Be a noun phrase or imperative verb phrase, not a sentence
• State the effect, not just the name (bad: "output format", good: "output format: json|text|sarif|markdown")
• List allowed values inline when the set is small (≤6 values)
• Include the unit for numeric flags ("bytes", "seconds", "goroutines")
• State the zero-value behavior when non-obvious ("0 = auto", "empty = auto-discover")
• Not exceed ~90 characters
• Not duplicate the flag name verbatim as the first word
• Use identical help text for both the short and long registration (no (shorthand) suffix — the custom help formatter merges them)

Examples

BAD:  "path"                          → just restates the flag name
GOOD: "file or directory to scan"

BAD:  "Output format"                 → capitalized, no values listed
GOOD: "output format: text|json|sarif|markdown"

BAD:  "workers"                       → no unit or zero-value note
GOOD: "parallel scan goroutines (0 = NumCPU)"

Short Flag Assignment

Priority tiers

Assign short flags to the most frequently typed options first. A flag qualifies for a short alias when it is used interactively (not just in config files or CI scripts).

Tier	Criteria	Examples
1 — always	Every subcommand that has it	-v verbose, -q quiet, -c config
2 — high	Primary I/O flags	-o out, -f format, -p path
3 — medium	Commonly toggled behavior	-r rules-dir, -s source, -n dry-run
4 — skip	Rarely typed or dangerous	--require-signed-rules, --xor-brute

Letter reservation

These letters are reserved project-wide to avoid collisions:

Letter	Reserved for
-v	--verbose (global)
-q	--quiet (global)
-c	--config (global)
-o	--out
-f	--format
-p	--path
-r	--rules-dir or --rules-file
-s	--source
-n	--dry-run or --name
-t	--timeout variants
-m	--max-* variants
-e	--expected-rules or --ecosystems
-H	--allow-host
-M	--metadata
-y	--confirm

A short flag must not collide with another short flag on the same FlagSet. Different subcommands may reuse the same letter for different flags if the semantics are analogous (e.g. -o always means output path).

Registration pattern

Always register the short form as a separate fs.*Var pointing to the same variable, immediately after the long form. Use identical help text for both:

fs.StringVar(&outPath, "out", "", "write report to file")
fs.StringVar(&outPath, "o", "", "write report to file")

Do not add a (shorthand) suffix — the custom help formatter (printScanFlags) merges both onto one line automatically.

Shorthand Config-Precedence Safety

This is the most common source of CLI flag bugs. Go's flag.Visit records the Flag.Name that was parsed, not aliases. When a user passes -f json, the visited set contains "f", not "format". If ApplyRunConfigValues only checks FlagWasSet(visited, "format"), it sees format as unset and allows config/preset/profile layers to silently overwrite the user's explicit choice.

Three synchronization points

Adding or removing a shorthand requires updates in three places:

Location	Map	Direction	Purpose
internal/config/config.go ApplyRunConfigValues	flagShorthands	canonical → short ("format": "f")	Config-layer precedence: prevents config/preset/profile from overwriting CLI shorthand values
cmd/skeptic/main.go	scanFlagShorthands	short → canonical ("f": "format")	Help formatter: merges -f and --format onto one line, suppresses standalone shorthand entry
cmd/skeptic/perform_skeptic_run.go registerRunFlags	(the fs.*Var calls)	Both names registered	Actual flag registration with Go's flag package

When adding a new shorthand, update all three. When reviewing existing flags, verify all three are in sync.

How ApplyRunConfigValues uses the map

flagShorthands := map[string]string{
    "format": "f",
    "path":   "p",
}

flagExplicit := func(flagName string) bool {
    if FlagWasSet(visited, flagName) {
        return true
    }
    if short, ok := flagShorthands[flagName]; ok {
        return FlagWasSet(visited, short)
    }
    return false
}

Every setString / setBool / setInt / setFloat64 call in that function uses flagExplicit(name) instead of raw FlagWasSet(visited, name).

Flags that also need shorthand checks in ResolveConfigLayers

--mode and --preset are checked directly in ResolveConfigLayers (before ApplyRunConfigValues runs) with bare FlagWasSet calls. If shorthands are ever added for these flags, those call sites must also check the short name. Both have inline comments flagging this.

Help Output Formatting

Custom formatter (printScanFlags)

The scan --help output uses printScanFlags (in main.go) instead of Go's fs.PrintDefaults(). This formatter:

• Merges shorthand and long-form flags on one line (-f, --format VALUE)
• Uses -- prefix for all multi-char flags (POSIX convention)
• Suppresses standalone shorthand entries (they appear merged)
• Omits global flags already shown in the hand-written header section
• Uses contextual type hints (PATH, VALUE, N, GLOB) via flagTypeName/inferStringMeta

When to update the formatter

• Adding a shorthand: add to scanFlagShorthands map
• Adding a path-like flag: add its name to inferStringMeta so it gets PATH label
• Adding a glob-like flag: add its name to inferStringMeta so it gets GLOB label
• Adding a new global flag: add to the globals map inside printScanFlags

Manual verification

After any flag change, build and check output visually:

go build -o /tmp/skeptic-test ./cmd/skeptic/
/tmp/skeptic-test scan --help 2>&1 | head -80
rm -f /tmp/skeptic-test

Verify: no duplicate entries, no single-dash long flags, shorthands merged, type hints make sense, globals not repeated in scan section.

Global vs Local Decision

When a flag should be global

A flag belongs in extractGlobalFlags (pre-parsed before subcommand dispatch) when all of these are true:

1. It applies to every subcommand (or nearly every one).
2. It controls infrastructure (logging, profiling, config loading), not scan behavior.
3. It does not conflict with a subcommand-specific flag of the same name.

Current globals: --verbose/-v, --quiet/-q, --cpu-profile, --mem-profile, --trace, --perf-debug, --log-file, --config/-c.

When a flag should be local

A flag is local when:

• It only makes sense for one subcommand (e.g. --bind for serve).
• It controls scan behavior that varies by context (e.g. --scan-style).
• It has a subcommand-specific default (e.g. --fail-on defaults differ).

Passthrough patterns

Pattern	When to use	Example
Direct field access (gf.Verbosity)	Subcommand handler is in cmd/skeptic/ and receives globalFlags	corpus subcommands
mergeGlobalFlagsIntoArgs	Subcommand parses its own FlagSet in internal/ and needs globals as argv	ingest, default scan
Prepend --config only	Subcommand only needs config path from globals	waive, serve, mcp
globalProfileAndStop only	Subcommand only needs profiling from globals	bundle, export-evidence

When adding a new subcommand, choose the simplest pattern that covers the globals it needs. Do not merge all globals if only config is needed.

Test Isolation for Config-Loading Code

Any test that calls run(), performSkepticRun(), or ResolveConfigLayers() exercises the config auto-discovery path: LoadConfigValues → DiscoverConfigPath → ActiveProfilePath. If XDG_DATA_HOME is not isolated, the test picks up the developer's real XDG profile (e.g. ~/.local/share/skeptic/profiles/dev.json), which can set preset, format, waivers, and other values that silently change test behavior.

Required isolation

func TestAnythingThatCallsRun(t *testing.T) {
    t.Setenv("XDG_DATA_HOME", t.TempDir())  // FIRST LINE
    // ... rest of test
}

When isolation is required

Calls	Needs XDG_DATA_HOME isolation?
run(...)	Yes — always
performSkepticRun(...)	Yes — always
ResolveConfigLayers(...)	Yes — always
LoadConfigValues(...)	Yes — always
DiscoverConfigPath()	Yes — always
ActiveProfilePath() / ActiveProfileName()	Yes — always
LoadSystemConfig("") (empty path)	Yes — auto-discovers from XDG
LoadSystemConfig("/explicit/path")	No — explicit path bypasses discovery
ApplyRunConfigValues(...)	No — pure value merge, no filesystem access
FlagWasSet(...) / VisitedFlagNames(...)	No — pure map lookups
Validation-only tests (exit code 2 before config loads)	Technically safe but still isolate for robustness

Audit checklist

When reviewing tests:

1. Search cmd/skeptic/*_test.go and internal/config/*_test.go for run(, performSkepticRun(, ResolveConfigLayers(
2. Verify each match has t.Setenv("XDG_DATA_HOME", t.TempDir()) before the call
3. If the test also uses os.Chdir, the XDG_DATA_HOME setenv must come before the chdir

Verification

After any flag change:

go fmt ./...
go vet ./...
go build ./...
go test -race -count=1 ./cmd/skeptic/ ./internal/config/ ./internal/corpus/

Check --help output for each modified subcommand to verify formatting.

Quick Checklist — Adding a New Shorthand

• Register both fs.*Var(&field, "long-name", ...) and fs.*Var(&field, "x", ...) in registerRunFlags
• Use identical help text for both (no (shorthand) suffix)
• Add "long-name": "x" to flagShorthands in ApplyRunConfigValues (internal/config/config.go)
• Add "x": "long-name" to scanFlagShorthands in main.go
• If the flag is also checked directly in ResolveConfigLayers, update that call site
• Add a test case to TestApplyRunConfigValuesShorthandFlagPrecedence verifying the shorthand blocks config override
• Run go test -race -count=1 ./cmd/skeptic/ ./internal/config/
• Build and visually verify --help output shows merged -x, --long-name line