fix(cli): honor json from config file and MMTREADER_JSON (#96) - #98
Merged
Merged
Conversation
cli_options_t carried two fields meaning one thing: output_format, which every output decision actually reads, and json, whose only reader was an argument to a verbose debug printf. The config path (parse_copy_config_fields) and the environment path (MMTREADER_JSON) both wrote json alone, so `json = 1` in a config file and MMTREADER_JSON=1 were silently ignored — only -j/--json, which wrote both, had any effect. Delete json and make output_format the single carrier. The config copy and the environment application now write it, and MMTREADER_JSON is mapped explicitly onto OUTPUT_FORMAT_TEXT/JSON so an out-of-range value cannot leave a bogus format behind. env_apply_int keeps its contract: an unset or empty variable still leaves the config value untouched. Second, deliberate behavior change: a -c/--config file used to be re-read *after* the getopt loop and re-copied unconditionally, so it beat explicit CLI flags (`-b 100 --config <buffer=777>` used 777) and skipped the environment entirely — the opposite precedence from ~/.mmtreader.conf, which loads before the loop. The comment above the old reload helper recorded that inversion as intentional and preserved it verbatim; it is intentional no longer. A new pre-scan finds the --config path before getopt runs (all four spellings, clustered short options, "--" terminator, last occurrence wins, unambiguous long-option abbreviations), both config files are loaded through the same helper before the loop, and the environment is applied once on top. One rule for both files: built-in defaults < ~/.mmtreader.conf < --config file < env < CLI flags case 'c' still records the path so getopt keeps accepting --config; the value it records is the same one the pre-scan already resolved. The replaced comment now carries the audit of all five config-backed fields: quiet and buffer_mb are live only on the live-capture path, verbose and no_color are live and general, json was the dead one.
The two checks that covered MMTREADER_JSON=1 and --json asserted only exit code 0, which the broken build satisfied while emitting the text table. Both now assert the produced OUTPUT: stdout parses as JSON with an input_stats key, and the text table is absent. A new "Issue #96" section covers the config file, which the script never touched before: json = 1 produces JSON; --text overrides it; --json overrides MMTREADER_JSON=0; MMTREADER_JSON=5 normalizes to JSON rather than a bogus format; the environment beats a --config file; and -v, -C and -b each beat a conflicting --config file. Temp configs go in an mktemp -d, matching the existing convention. Against the pre-fix binary 8 of these go red; they pass with the fix. Unprivileged run: 35/35 -> 52/52 (17 new asserts, all outside the root-gated live-capture block). tests/test_parse.c gains ten unit tests for the same behavior at the parser seam, plus make_bare_home() to isolate a real ~/.mmtreader.conf and make_test_conf() for an arbitrary -c path (make_test_home hardcodes the .mmtreader.conf basename). They pin the environment path, the out-of-range clamp, all four --config spellings, last-occurrence-wins, --config over the default config, and every field a --config file writes losing to its CLI flag. 73 -> 118 asserts.
docs/CONFIG.md described a priority order that no longer matched either config path: it ranked sections above the global block and never said which of the two config files won. It now states the single rule — compiled defaults < ~/.mmtreader.conf < --config file < environment < CLI flags — records that the two files used to have opposite precedence, and explains that a --config file replaces rather than merges the values the default file supplied. Both key tables gain an "Effective?" column. proto_path, sessions, output_format, ip_classify, hostname_classify, port_classify and the per-section buffer are parsed and then never read; quiet and buffer only reach the live-capture path. The worked example promised proto paths, session counts and a 100 MB capture buffer that the binary never applied, and implied section keys are scoped to their command when json, quiet, verbose and no_color share one global slot — both corrected. Test counts follow the new asserts: 252 -> 297 unit asserts (parse 73 -> 118) and 35/35 -> 52/52 CLI integration checks, across AGENT_ENVIRONMENT.md, docs/TESTING.md, docs/DEVELOPMENT.md and README.md. The counts recorded in the historical #72 changelog entry are left frozen. docs/USER_GUIDE.md is deliberately untouched — open issue #97 targets it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #96
Summary
json = 1in a config file andMMTREADER_JSON=1were silently ignored:cli_options_tcarried two fields meaning one thing —output_format, which every output decision actually reads, andjson, whose only reader was an argument to a verbose debug printf. The config path and the environment path both wrotejsonalone, so only-j/--json(which wrote both) had any effect. This deletesjson, makesoutput_formatthe single carrier, and — because acceptance criterion 3 cannot hold otherwise — also fixes the precedence inversion where a--configfile beat explicit CLI flags. Both config files now load before the getopt loop under one rule: built-in defaults <~/.mmtreader.conf<--configfile < environment < CLI flags.Approach
Option 2 — Balanced: unify both config paths and make
output_formatthe single carrier. A newparse_prescan_config_path()resolves the--configvalue beforegetopt_longruns (all four spellings, clustered short options,--terminator, unambiguous long-option abbreviations, last occurrence wins), so both config files are applied through one helper ahead of the option loop, the environment is applied once on top, and the CLI writes last and wins naturally. Because config now precedes the loop, no per-key presence tracking is needed — an unconditional copy overwrites nothing but built-in defaults.Decision Record
cli_options_thad two fields for one concept.output_format(cli/parse.h:64) is the only field read behaviorally (mmtReader.c:84,:114engine_set_output_format,:257);json(cli/parse.h:69) had exactly one reader — an argument to a debugfprintfatmmtReader.c:94.parse_copy_config_fields()andenv_apply_int("MMTREADER_JSON", &opts->json)both wrote the dead field, so neither documented path could ever select JSON. Compounding it,parse_reload_custom_config()ran after the getopt loop and re-copied all five shared fields unconditionally with no environment re-application, so a--configfile beat explicit CLI flags — the opposite precedence from~/.mmtreader.conf, which loads before the loop. Neither defect is a regression from [3.5] Split parse_options into layered helpers #67/PR refactor(cli): split parse_options into layered helpers (#67) #91: that refactor extracted the helpers and preserved the ordering verbatim, making the defect legible rather than introducing it.output_formatfrom the resolvedjsonflag plus a CLI tri-state); Option 2 — Balanced (unify both config paths,output_formatas single carrier); Option 3 — Comprehensive (per-key presence tracking inconfig_tplus revival of the seven dead config keys).jsonsymptom while leaving the confirmed config-beats-CLI inversion onno_color,verboseandbuffer_mb, so the same class of bug is refilable the day it merges. Option 3 — reviving seven dead config keys and changing the publicconfig_tis required by no acceptance criterion, and the ordering fix in Option 2 already makes presence tracking unnecessary.output_formatthe single carrier.--configpre-scan is a hand-rolled mirror ofgetopt_long;SHORT_OPTS_WITH_ARGis a second literal that must be kept in sync with the getopt optstring by hand. Verified against a 20-case argv matrix (see Test Results). Two pre-existing conditions are deliberately left alone and confirmed unchanged, not regressed: a config-filebufferstill bypasses the CLI's 1–10000 bound before the(uint16_t)cast atmmtReader.c:186(parse_validate_final()never checkedbuffer_mbon either side, and is still the last call inparse_options()), and a nonexistent--configpath is still silently ignored.printf 'json = 1\n' > "$T/mmt.conf"; ./mmtReader analyze -t smallFlows.pcap --config "$T/mmt.conf" | jq .andMMTREADER_JSON=1 ./mmtReader analyze -t smallFlows.pcap | jq .confirmed red for the stated reason — both emitted the text banner andjqexited 5 (parse error) while the--jsoncontrol exited 0 → regression teststests/test_cli.sh(Issue json option from config file and MMTREADER_JSON env var are silently ignored #96 section) andtests/test_parse.c::test_custom_config_json_sets_output_format/test_env_json_sets_output_format/test_custom_config_loses_to_cli_flags. Durability independently proven: the newtest_cli.shrun against a binary built fromHEAD~1fails 8 of the new asserts (44/52); it passes 52/52 with the fix.Analyzed at:
fix/96-config-env-json-ignored @ 38c3e88(2026-08-27)Changes
cli/parse.cparse_prescan_config_path()+long_opt_lookup()under a Config-file pre-scan banner; replacedparse_load_default_config()/parse_reload_custom_config()withparse_apply_env()+parse_load_config_files();parse_copy_config_fields()now writesoutput_format;MMTREADER_JSONmapped explicitly ontoOUTPUT_FORMAT_TEXT/JSONso an out-of-range value cannot leave a bogus format;parse_options()reordered to the single precedence rule; the stale "preserved verbatim" comment replaced by the criterion-5 field auditcli/parse.hint json;;output_formatdocumented as the single carrier.OUTPUT_FORMAT_*#defines deliberately untouched (they name-collide withcore/engine.h's enum; the include order inmmtReader.cis load-bearing)mmtReader.copts.json→opts.output_format == OUTPUT_FORMAT_JSON; includes not reorderedtests/test_parse.copts.jsonasserts retargeted to the liveopts.output_format; 2 new helpers and 11 new tests (73 → 120 asserts)tests/test_cli.shdocs/CONFIG.mddocs/TESTING.md,docs/DEVELOPMENT.md,AGENT_ENVIRONMENT.md,README.mdCHANGELOG.mdTest Results
tests/test_cli.sh) — up from 35/35; plus 6 / 6 SDK version-gate checksmake clean && make testexits 0, ends withAll tests passed!, 0 compiler warnings under the-Wall -Wextragate, exactly one tolerated skip (SKIP: live capture on 'lo' unavailable — Operation not permitted, root-gated by design)--configargv matrix against the built binary:-c path,-cpath,--config path,--config=path,--conf/--cabbreviations and the-qc filecluster all load the config;--terminates the scan;-x -c CONF -t f.pcapcorrectly does not treat an option value as the flag; missing values, a directory path, a missing file, an ambiguous--tand a bare-all exit cleanly with no crash or out-of-bounds readAcceptance Criteria Verification
printf 'json = 1\n' > /tmp/j.conf && ./mmtReader analyze -t smallFlows.pcap --config /tmp/j.conf | jq .succeeds (valid JSON)analyze -t smallFlows.pcap --config <json=1>emitted the text banner andjqexited 5 → fixed →tests/test_cli.shIssue #96 section +tests/test_parse.c::test_custom_config_json_sets_output_formatMMTREADER_JSON=1 ./mmtReader analyze -t smallFlows.pcap | jq .succeeds (valid JSON)jqexit 5 → fixed →tests/test_cli.shIssue #96 section +tests/test_parse.c::test_env_json_sets_output_formatjson = 1overridden by--text;MMTREADER_JSON=0overridden by--json-b 100 --config <buffer=777>used 777;-C --config <no_color=0>kept 37 ANSI sequences;-v --config <verbose=0>gave 0 DEBUG lines) → fixed →tests/test_parse.c::test_custom_config_loses_to_cli_flagsand the--text/MMTREADER_JSON=0 --jsoncases intests/test_cli.sh. Post-fix spot check:-b 100 --config <buffer=777>now reportsbuffer size: 100tests/test_cli.shasserts the produced OUTPUT is JSON for both the env-var and config-file paths, not merely exit code 0tests/test_cli.sh— the two exit-code-only checks (env var, and the--jsonflag itself) now assert output shape, plus a 13-assert Issue #96 section; total 35/35 → 52/52quiet,verbose,no_color,buffer_mb— confirm each is read, or report which are deadcli/parse.cand indocs/CONFIG.md's "Effective?" columns — see Field audit belowmake clean >/dev/null && make testexits 0, 0 failures (1 tolerated root-gated skip)All tests passed!, 299 unit asserts + 52/52 CLI + 6/6 SDK, 0 failures, 0 warnings, exactly oneSKIP: live capture on 'lo'Field audit (acceptance criterion 5)
Of the five fields
parse_copy_config_fields()writes:jsonmmtReader.c:94quietmmtReader.c:173,:244, both inside theMODE_LIVE_INTERFACEbranch — so-qhas no observable effect onanalyze(which emits noINFO:lines at all), making the existingtests/test_cli.sh:139assertion vacuous. Reported, not fixed — out of scopeverbosemmtReader.c:91,:149,:160,:181,:269; noMMTREADER_VERBOSEenv var exists (config + CLI only)no_colormmtReader.c:67buffer_mbmmtReader.c:174–:186(capture_initis the real consumer);analyzeusespcap_open_offline()and ignores itAdditionally found and reported, not revived (no acceptance criterion asks for them; suitable for a follow-up issue): seven config keys are parsed by
config.cbut never copied into the options —proto_path,sessions,output_format(fully dead, no reader anywhere),ip_classify,hostname_classify,port_classify, and the per-section[analyze] buffer/[capture] buffer.docs/CONFIG.mdnow marks these honestly instead of promising behavior that does not exist.Notes for reviewers
--config-vs-CLI precedence is deliberate. The comment above the oldparse_reload_custom_config()recorded that inversion as "pre-existing, intentional behavior … preserved here verbatim" (carried over untouched by the [3.5] Split parse_options into layered helpers #67 refactor, PR refactor(cli): split parse_options into layered helpers (#67) #91). It is intentional no longer — acceptance criterion 3 cannot hold while a config file outranks an explicit flag. The commit body forac8b98dstates this explicitly.docs/USER_GUIDE.mdandcompletions/are intentionally untouched: open issue docs/USER_GUIDE.md contains invented sample output that no code path emits #97 ownsUSER_GUIDE.md, and no new flags were added.USER_GUIDE.md's claim that "CLI flags always override config file values" is now true for the first time.