#129: importer top-level subcommands — a misread flag can no longer launch a full import #141
No reviewers
Labels
No labels
blocked
claude-wip
map-report
map:is
map:se
needs-review
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
art/ismap!141
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "issue-129-subcommand-cli"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
#129: importer top-level subcommands — a misread flag can no longer launch a full import
Closes #129
What changed
The
importer/importerCLI is now a subcommand CLI. Commands say what to do;flags on a command are modifiers. The old flat-flag surface (17 flags, no modes)
is gone — including the two shapes the incident depended on:
importer <config>(bare config = unlabelled full import) → error, with apointer at
importer import <config>.importer -validate <config>→ rejected with a pointer atimporter config-check <config>.-validatewas a full-import modifier, nota config check; the 3h12m Finland import of #113 is exactly the failure this
dispatch exists to make unreachable.
Subcommands
importerimporter help [command]importer import <config> [modifiers]importer config-check <config>validate-configis accepted as an alias (the name the incident was discussed with); the maintainer's decision #1 (config-check) is the documented nameimporter acceptance <config>-acceptance-onlymode, as a verb (read-only review path)importer fetch <config> [geotorget|lastkajen|trafiklab|all]*-fetchmodes; no provider arg = all the config declaresimporter list <config> [geotorget|lastkajen]*-listmodes; no provider arg = bothimportmodifiers:--layer,--regeneralize,--validate,--ensure,--refetch,-jobs,-batch-rows,--acceptance fast|all,--secrets.Both
-flagand--flagspellings work (Go's flag package treats them as thesame flag), so the Makefile's
-jobs 4 -batch-rows 10000survives unchanged.The blast-radius startup line (before any DB work):
Decisions from the issue, and how this resolves them
config-checkper the maintainer's comment; theparent task's
validate-configkept as an alias so both spellings work.No flat flags are kept as deprecated aliases;
-validate(and every otherflat flag) is rejected with a message. The Makefile is the whole consumer
list and is updated;
run/notes are historical records and were left alone.log.Printfbefore the pool opens; thewording is unit-tested (
blastRadiusLine). The issue's word "blast radius"is used, not "destructive" (the maintainer's point that a full import is
safe, just slow, is respected — the line is about surprise, not danger).
docs/importer-cli.mdgains the mode table and the footgun note;the acceptance-tests section is unchanged in substance. README, AGENTS.md,
geo-primer.md and country-sweden.md updated to the new syntax.
check-queriesis deliberately not implemented — that is #98's scope; theissue body says it slots into this shape when it lands.
Verification
make build,make test(importer + mapsrv),go vetboth packages,gofmt -lclean.importer/cli_test.go(no import, no DB): dispatchrouting, the
-validatefootgun (rejected, points at config-check),bare-config rejection, flat-flag rejection, import flag parsing (incl. the
flag.ErrHelppath and bad-acceptancevalues), provider-arg validationfor fetch/list, the blast-radius wording, and
config-checkend to endagainst all five real configs.
importer(help, exit 0);importer help import;importer help fetch.importer config-checkon conf/{is,se,pl,fi,world}map.yaml — all OK,"no database touched".
importer validate-config conf/fimap.yaml— alias works.importer acceptance conf/ismap.yaml— 26/26 passed, 0 failed.make acceptance(all five configs, fast bucket) — is 26, se 12/13(1 deferred to
-acceptance all), pl 13, fi 12, world 9; 0 failed.importer import --ensure conf/ismap.yamlandconf/world.yaml— printedthe blast-radius line and exited early on the already-populated DB (no
work; the only safe way to show the startup line live without importing).
importer fetch conf/ismap.yamlandimporter list conf/ismap.yaml—no-op "nothing to do" exits (ismap declares no providers), exit 0.
-validatewas never used.Left undone / notes
-acceptance-only,-geotorget-fetch, etc. are hard errors now; anyonewith a script other than the Makefile would need to update it (none exist in
the repo outside
run/history).importer fetchdoes not fetch staticdownloads:URLs (matching the old*-fetchmodes); those are covered byimportand the makefetch-*targets, and the help text says so.
Review
Verified: builds clean,
go vetclean,go test ./...green. The safety goals of #129 hold empirically — bareimporterprints help and exits 0 without starting work,importer conf/fimap.yamlis rejected with a pointer atimporter import, andimporter -validate conf/fimap.yamlis rejected with the two-way pointer (config-check vsimport --validate). No stale flat-flag invocations remain anywhere in the Makefile, docs or configs.The built-in usage text documents an argument order that always fails
Every usage constant puts the config before the modifiers, but Go's
flagpackage stops parsing at the first non-flag argument, so the documented form is rejected:It fails loudly rather than silently, and the Makefile uses the working order throughout (
import $(BIGJOBS) conf/semap.yaml,acceptance --acceptance all $$c), so nothing is broken today. But in a CLI whose whole thesis is that an operator must not be misled about what a command is going to do, the help text teaches the one form that cannot work, and the error it produces ("got 3") does not say why.Affects
usageText,importUsage,acceptanceUsage,fetchUsageandlistUsage— all five write<config>first. Either reword toimporter import [modifiers] <config>, or permute args beforefs.Parseso both orders work.run()returns an exit code, but the DB-touching commandslog.Fatalpast itcmdImport,cmdAcceptance,cmdFetchandcmdListall switch tolog.Fatal/log.Fatalfonce past argument parsing (importer/cli.go:376, 379, 383; importer/main.go:93, 96, 107; and the fetch/list secrets paths).log.Fatalcallsos.Exit(1)directly, which bypasses theintthatrunpromises to return.Two consequences: the
run(args, stdout, stderr) intsignature that makescli_test.gopossible only actually holds for the parse-level paths the tests cover, and the error loses theimport:/acceptance:prefix that the same functions carefully add to their parse errors. Returning1and writing to the injectedstderrwould make the contract real all the way down.Minor
The
importhelp block mixes conventions inside one list:--layer,--regeneralize,--validate,--ensure,--refetch,--acceptance,--secretsare written with two dashes,-jobs Nand-batch-rows Nwith one. Go treats them identically, so this is cosmetic, but they read as two different classes of thing.runRegeneralizestill says-regeneralize requires -layer(importer/main.go:455) in single-dash form now that the docs standardise on--layer.Review fixes (
4e684ee)All three points addressed on
issue-129-subcommand-cli(no new branch).1. Argument order — both orders now work
parseImportArgsand the acceptance/fetch/list commands permute args beforeflag.Parse(permuteArgsin importer/cli.go): flags may come before or afterthe config, and both orders parse identically. The documented form from the
review now runs:
(the Makefile's flags-first form is unchanged). The top-level help now says
"Modifier flags may come before or after the config", and the arg-count errors
say why instead of a bare "got N":
The permutation is value-aware (
--layer placestravels as a pair; bool flagsdon't consume the next token;
--still terminates), so an unknown flag stillreports itself:
importer import conf/fimap.yaml --bogus→import: flag provided but not defined: -bogus, not an arg-count error.2. Exit-code contract — no log.Fatal left in the command paths
cmdImport,cmdAcceptance,cmdFetchandcmdListno longer switch tolog.Fatalpast argument parsing. Every error past the parse returns the codeand writes to the injected stderr with the
import:/acceptance:/fetch:/list:prefix (loadConfig, prepareConfig, openDB, secrets, and therest of cmdImport's body). The helpers they reach were converted too:
selectedLayers/blastRadius(the unknown---layererror is nowimport: --layer "bogus" not found in config, exit 2, before any output),runRegeneralize/importOneLayer/generalizeAndSwap(error returns), andfinishAcceptance(returns the verdict code instead oflog.Fatal). There isno
os.Exitleft insiderun()'s call tree —mainis the only exit point.importer/cli_test.gocovers the paths (no DB, no import): missing config,invalid config, unparseable destination connstr, and missing secrets for all
four subcommands, each asserting the exit code and the stderr prefix; plus
TestImportArgsPermutation(both orders, every modifier) andTestRunAcceptanceConfigFirstPermutation.finishAcceptance's verdict codesare covered in
acceptance_test.go.3. Minor
Import help is all double-dash (
--jobs N,--batch-rows N), andrunRegeneralizenow says--regeneralize requires --layer.Verification
make build,go vet(importer + mapsrv),make test— all green;gofmt -lclean.importer acceptance conf/fimap.yaml --acceptance allparses and runs 12/12 green;
importer help/importer help importshowthe corrected text; the Makefile's flags-first order still parses.
-validatewas never used.Re-review
Both findings are fixed, and I verified each end to end rather than by reading the diff.
Argument order. The previously-failing documented forms now work, and the flags-first order the Makefile uses still does:
permuteArgshandles the cases that make this kind of function subtly wrong — inline--flag=value, boolean flags that must not eat the next token, unknown flags left in place soflag.Parsereports them itself.TestImportArgsPermutationcovers both orders, a fully shuffled line, and both dash styles.Exit-code contract.
os.Exitnow appears exactly once in non-test code, inmain(). All four DB-touching commands return codes with their prefix intact:finishAcceptancereturning a verdict is propagated at all three call sites, and I confirmed the whole path end to end with a deliberately-failing probe test:exit=1, tally line and failure summary both still printed.The
log.Fatal→ error conversion is the part with real regression potential, so I walked every converted site. Each former fatal is an immediatereturnat the caller — nothing continues past a failure that previously aborted, including inside the per-layer loop and the generalization cascade. Control flow is preserved.Verified on top of current
main(this branch is 7 commits behind, merges clean):go build,go vet,go test ./...green; all five acceptance suites pass;config-checkOK on all five configs. (conf/mapsrv.yamlandconf/secrets.example.yamlfailconfig-check, correctly — they aren't importer configs.)One thing left, minor
permuteArgsdrops the--token, so the terminator never reachesflag.Parse— which contradicts the function's own comment ("--still terminates flag parsing"):A config path starting with
-is exotic, so the impact is small; it's the comment claiming otherwise that makes it worth a line. The fix is one character —args[i+1:]→args[i:]in the--branch, keeping the terminator at the head of the positionals. Confirmed against the realflagpackage:The normal (no
--) path is byte-identical either way.The cosmetic items landed too — the import help block is all double-dash, and
runRegeneralizesays--regeneralize requires --layer.