Fix -pm <value> parsing and add comma-separated multi-fact -F (#14)

Two related query-ergonomics fixes surfaced while using the tool interactively.

## 1. `-pm <value>` failed with `unknown command "k8s"`

pflag does not attach a space-separated value to a string flag (`-m`) grouped with a bool flag (`-p`), so `k8s` was left as a stray positional. Only `-pm=k8s` or the un-grouped `-p -m k8s` worked.

- Allow one positional argument (`cobra.MaximumNArgs(1)`) and fall back to it for the match value when `-m` is empty (`matchValue()`). `-pm/-im/-ipm <value>` and a bare `-p <value>` now all work; `-m` still wins when both are given.

## 2. `-F a,b` (multiple facts) returned `{}`

`-jF ipaddress,enc_role` queried a single fact literally named `ipaddress,enc_role`.

- Split `-F` on commas (`splitFactNames`) and match any of them via an `or` over `["=","name",<n>]` clauses (`nameFilter`); a single name keeps the plain `=` form.
- Key JSON output by each result's real fact name so all requested facts appear under the host.

Both paths have unit tests; verified live: `node-lookup -R -pm externaldns | node-lookup -jF ipaddress,enc_role` returns both facts per host. AGENTS.md updated.

Reviewed-on: #14
Co-authored-by: Ben Vincent <ben@unkin.net>
Co-committed-by: Ben Vincent <ben@unkin.net>

AGENTS.md

@@ -59,14 +59,16 @@ installed. To load ad-hoc in the current shell, e.g. zsh:
 ./node-lookup -R                          # show all nodes with role fact
 ./node-lookup -n <hostname>               # lookup a specific node
 ./node-lookup -F <fact_name>              # filter by fact name
-./node-lookup -m <value>                  # exact value match (-m)
-./node-lookup -pm <value>                 # partial/regex match (-p -m combined)
-./node-lookup -im <value>                 # inverse exact match (-i -m combined)
-./node-lookup -ipm <value>                # inverse partial match (-i -p -m combined)
+./node-lookup -jF ipaddress,enc_role      # several facts at once (comma-separated)
+./node-lookup -R -m <value>               # exact value match (-m)
+./node-lookup -R -pm <value>              # partial/regex match (-p -m combined)
+./node-lookup -R -im <value>              # inverse exact match (-i -m combined)
+./node-lookup -R -ipm <value>             # inverse partial match (-i -p -m combined)
+./node-lookup -R -p <value>               # value may also be given positionally
 ./node-lookup -R -1                       # node names only
 ./node-lookup -R -2                       # values only
 ./node-lookup -R -C                       # count occurrences
-./node-lookup -R -A                       # output as Ansible YAML inventory
+./node-lookup -R -A                       # output as Ansible YAML inventory (queried facts become host vars)
 ./node-lookup -j                          # output as JSON { host → { fact → value } }
 ./node-lookup --url http://host:8080/...  # override PuppetDB URL for this invocation
 echo -e "node1\nnode2" | ./node-lookup -R # pipe node names via stdin
@@ -110,9 +112,11 @@ Show the active configuration (after all overrides applied):
 
 - **`loadConfig()`**: reads config file → applies env vars → returns `config` struct. Called once at startup in `main()`.
 - **`buildQuery()`**: returns a PuppetDB PQL-compatible JSON array string. Uses `roleFact` from config (not hardcoded). Match modifiers: `-p` (partial/regex, uses `~` op), `-i` (inverse, wraps with `not`), composable.
+- **Multiple facts**: `-F` accepts a comma-separated list (`ipaddress,enc_role`). `splitFactNames()`/`nameFilter()` turn several names into an `or` over `["=","name",<n>]` clauses; JSON output keys each value by the fact's real name so all requested facts appear per host.
+- **Match value / `matchValue()`**: the value to match comes from `-m/--match` or, if that is empty, an optional positional argument. The positional fallback exists because pflag does not attach a space-separated value to a string flag grouped with a bool flag, so in `-pm k8s` the `k8s` arrives as a positional. `-m` still wins when both are given.
 - **`queryPuppetDB(url, query)`**: takes the URL as a parameter — never reads globals.
 - **`processResults()`**: iterates facts, returns sorted `"certname value"` strings. JSON string values are unquoted; other JSON types rendered as compact JSON.
-- **Output modes**: JSON (`-j`), count (`-C`), Ansible YAML (`-A`), node-only (`-1`), value-only (`-2`), default (node + value).
+- **Output modes**: JSON (`-j`), count (`-C`), Ansible YAML (`-A`), node-only (`-1`), value-only (`-2`), default (node + value). `-j` and `-A` share `factsByHost()`, so both attach the queried fact(s) per host — as an object under the host (`-j`) or as inventory host vars (`-A`).
 - **Stdin support**: `stdinReader()` reads node names from stdin only when it is a real pipe/redirect carrying data (and no `-n` given). Terminals, `/dev/null`, and empty/closed pipes fall through to a normal query — so running without a TTY (e.g. invoked by an agent or CI) behaves like an interactive run instead of consuming empty input.
 - **SIGPIPE handling**: `signal.Ignore(syscall.SIGPIPE)` so pipes to `head` etc. work cleanly.
 
@@ -136,5 +140,5 @@ mode (default, `-1`, `-2`, `-C`, `-j`, `-A`, `-a`). PuppetDB is stubbed with
 
 - `-1`, `-2`, `-C`, and `-A` all require `-R` or `-F`; the tool exits with an error otherwise.
 - `-C` (count) with stdin reads all lines as pre-fetched `"node value"` output for counting — it does **not** query PuppetDB per line.
-- JSON output (`-j`) builds `{ hostname: { factname: value } }` where the fact key is the `-F` value, the `role_fact` config value (if `-R`), or `"value"` as fallback.
+- JSON output (`-j`) builds `{ hostname: { factname: value } }` keyed by each result's actual fact name (so `-F ipaddress,enc_role` yields both per host); it falls back to the `-F` value, the `role_fact` config value (if `-R`), or `"value"` only when a result carries no name.
 - `config init` fails if the config file already exists (will not overwrite).

main.go

@@ -110,6 +110,31 @@ type fact struct {
 	Value    json.RawMessage `json:"value"`
 }
 
+// splitFactNames splits a comma-separated -F value into trimmed, non-empty
+// names, so `-F ipaddress,enc_role` queries both facts.
+func splitFactNames(factName string) []string {
+	var names []string
+	for _, n := range strings.Split(factName, ",") {
+		if n = strings.TrimSpace(n); n != "" {
+			names = append(names, n)
+		}
+	}
+	return names
+}
+
+// nameFilter returns a PQL filter matching any of the given fact names: a plain
+// equality for one name, an "or" over per-name equalities for several.
+func nameFilter(names []string) []interface{} {
+	if len(names) == 1 {
+		return []interface{}{"=", "name", names[0]}
+	}
+	or := []interface{}{"or"}
+	for _, n := range names {
+		or = append(or, []interface{}{"=", "name", n})
+	}
+	return or
+}
+
 func buildQuery(node, factName, match, roleFact string, showRole, partial, inverse bool) string {
 	type filter = []interface{}
 	var filters []filter
@@ -117,8 +142,8 @@ func buildQuery(node, factName, match, roleFact string, showRole, partial, inver
 	if node != "" {
 		filters = append(filters, filter{"=", "certname", node})
 	}
-	if factName != "" {
-		filters = append(filters, filter{"=", "name", factName})
+	if names := splitFactNames(factName); len(names) > 0 {
+		filters = append(filters, nameFilter(names))
 	} else if showRole {
 		filters = append(filters, filter{"=", "name", roleFact})
 	}
@@ -236,6 +261,46 @@ func stdinReader(f *os.File) (*bufio.Reader, bool) {
 	return r, true
 }
 
+// matchValue resolves the value to match against. The -m/--match flag wins; if
+// it is empty, the (optional) positional argument is used instead. The
+// positional fallback exists so combined shorthands like `-pm k8s` work — pflag
+// leaves the space-separated `k8s` as a positional rather than attaching it to
+// the grouped -m flag.
+func matchValue(flagMatch string, args []string) string {
+	if flagMatch != "" {
+		return flagMatch
+	}
+	if len(args) > 0 {
+		return args[0]
+	}
+	return ""
+}
+
+// factsByHost groups collected facts into {certname: {factname: value}}. Each
+// value is keyed by the fact's real name, so multiple -F facts each appear
+// under the host; it falls back to the -F string, the role fact (with -R), or
+// "value" only when a result carries no name. Shared by the -j and -A outputs.
+func factsByHost(collected []fact, factName, roleFact string, showRole bool) map[string]map[string]interface{} {
+	out := map[string]map[string]interface{}{}
+	for _, f := range collected {
+		if _, ok := out[f.Certname]; !ok {
+			out[f.Certname] = map[string]interface{}{}
+		}
+		key := f.Name
+		if key == "" {
+			if key = factName; key == "" {
+				if showRole {
+					key = roleFact
+				} else {
+					key = "value"
+				}
+			}
+		}
+		out[f.Certname][key] = valueAny(f.Value)
+	}
+	return out
+}
+
 func allFactsForNode(puppetDBURL, node string) ([]fact, error) {
 	query, _ := json.Marshal([]interface{}{"=", "certname", node})
 	return queryPuppetDB(puppetDBURL, string(query))
@@ -306,25 +371,10 @@ func run(cfg config, nodeName, factName, match string, showRole, partial, invers
 
 	switch {
 	case jsonMode:
-		hostFactMap := map[string]map[string]interface{}{}
-		for _, f := range collected {
-			if _, ok := hostFactMap[f.Certname]; !ok {
-				hostFactMap[f.Certname] = map[string]interface{}{}
-			}
-			key := factName
-			if key == "" {
-				if showRole {
-					key = cfg.RoleFact
-				} else {
-					key = "value"
-				}
-			}
-			hostFactMap[f.Certname][key] = valueAny(f.Value)
-		}
 		enc := json.NewEncoder(os.Stdout)
 		enc.SetIndent("", "  ")
 		enc.SetEscapeHTML(false)
-		_ = enc.Encode(hostFactMap)
+		_ = enc.Encode(factsByHost(collected, factName, cfg.RoleFact, showRole))
 
 	case count:
 		values := stdinLines
@@ -334,10 +384,11 @@ func run(cfg config, nodeName, factName, match string, showRole, partial, invers
 		fmt.Println(strings.Join(countResults(values), "\n"))
 
 	case ansible:
+		// Attach each host's queried fact(s) as inventory host vars, e.g.
+		// `-F ipaddress,enc_role -A` yields hosts with ipaddress + enc_role set.
 		hosts := map[string]interface{}{}
-		for _, line := range returnData {
-			host := strings.Fields(line)[0]
-			hosts[host] = map[string]interface{}{}
+		for host, vars := range factsByHost(collected, factName, cfg.RoleFact, showRole) {
+			hosts[host] = vars
 		}
 		inventory := map[string]interface{}{
 			"all": map[string]interface{}{"hosts": hosts},
@@ -389,8 +440,14 @@ func main() {
 	)
 
 	rootCmd := &cobra.Command{
-		Use:   appName,
+		Use:   appName + " [value]",
 		Short: "Query PuppetDB for nodes.",
+		// Accept an optional positional match value in addition to -m. This makes
+		// combined shorthands like `-pm k8s` work: pflag does not attach a
+		// space-separated value to a string flag grouped with a bool flag (only
+		// `-pm=k8s` or `-p -m k8s` do), so `k8s` arrives here as a positional
+		// argument instead. Falling back to it keeps the ergonomic form working.
+		Args: cobra.MaximumNArgs(1),
 		PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
 			if cmd.Flags().Changed("url") {
 				cfg.PuppetDBURL = puppetDBURL
@@ -398,14 +455,14 @@ func main() {
 			return nil
 		},
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return run(cfg, nodeName, factName, match, showRole, partial, inverse, nodeOnly, valueOnly, count, ansible, jsonMode, allFacts)
+			return run(cfg, nodeName, factName, matchValue(match, args), showRole, partial, inverse, nodeOnly, valueOnly, count, ansible, jsonMode, allFacts)
 		},
 		SilenceUsage: true,
 	}
 
 	f := rootCmd.Flags()
 	f.StringVarP(&nodeName, "node", "n", "", "Node name")
-	f.StringVarP(&factName, "fact", "F", "", "Fact name")
+	f.StringVarP(&factName, "fact", "F", "", "Fact name (comma-separated for several, e.g. -F ipaddress,enc_role)")
 	f.BoolVarP(&showRole, "role", "R", false, "Show role fact ("+defaultRoleFact+" by default)")
 	f.StringVarP(&match, "match", "m", "", "Value to match (use with -p and/or -i)")
 	f.BoolVarP(&partial, "partial", "p", false, "Partial/regex match modifier (combine with -m)")

main_test.go

@@ -9,6 +9,8 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+
+	"gopkg.in/yaml.v3"
 )
 
 // ---- helpers ----------------------------------------------------------------
@@ -142,6 +144,101 @@ func TestBuildQuery_ValidJSON(t *testing.T) {
 	}
 }
 
+// ---- multi-fact -F (comma-separated) ----------------------------------------
+
+func TestSplitFactNames(t *testing.T) {
+	cases := map[string][]string{
+		"ipaddress":            {"ipaddress"},
+		"ipaddress,enc_role":   {"ipaddress", "enc_role"},
+		"ipaddress, enc_role ": {"ipaddress", "enc_role"}, // trims spaces
+		"a,,b,":                {"a", "b"},                // drops empties
+		"":                     nil,
+	}
+	for in, want := range cases {
+		got := splitFactNames(in)
+		if len(got) != len(want) {
+			t.Fatalf("splitFactNames(%q) = %v, want %v", in, got, want)
+		}
+		for i := range want {
+			if got[i] != want[i] {
+				t.Fatalf("splitFactNames(%q) = %v, want %v", in, got, want)
+			}
+		}
+	}
+}
+
+func TestBuildQuery_SingleFact_NoOr(t *testing.T) {
+	q := buildQuery("", "ipaddress", "", "enc_role", false, false, false)
+	if strings.Contains(q, `"or"`) {
+		t.Fatalf("single fact should not use 'or': %s", q)
+	}
+	if !strings.Contains(q, "ipaddress") {
+		t.Fatalf("expected fact name in query: %s", q)
+	}
+}
+
+func TestBuildQuery_MultiFact_UsesOr(t *testing.T) {
+	q := buildQuery("host1", "ipaddress,enc_role", "", "enc_role", false, false, false)
+	if !strings.Contains(q, `"or"`) {
+		t.Fatalf("expected 'or' over fact names: %s", q)
+	}
+	if !strings.Contains(q, "ipaddress") || !strings.Contains(q, "enc_role") {
+		t.Fatalf("expected both fact names: %s", q)
+	}
+	// Must remain valid PQL JSON, combined under "and" with the certname filter.
+	var v []interface{}
+	if err := json.Unmarshal([]byte(q), &v); err != nil {
+		t.Fatalf("query is not valid JSON: %v (%s)", err, q)
+	}
+	if v[0] != "and" {
+		t.Fatalf("expected top-level 'and', got %v", v[0])
+	}
+}
+
+func TestRun_JSON_MultipleFacts(t *testing.T) {
+	// Two facts returned for one host must both appear, keyed by their real name.
+	facts := []fact{
+		{Certname: "hosta", Name: "ipaddress", Value: rawJSON("198.18.0.1")},
+		{Certname: "hosta", Name: "enc_role", Value: rawJSON("roles::dns")},
+	}
+	out := runToString(t, facts, func(a *runArgs) {
+		a.showRole = false
+		a.factName = "ipaddress,enc_role"
+		a.jsonMode = true
+	})
+	var parsed map[string]map[string]interface{}
+	if err := json.Unmarshal([]byte(out), &parsed); err != nil {
+		t.Fatalf("output is not valid JSON: %v (%s)", err, out)
+	}
+	host := parsed["hosta"]
+	if host["ipaddress"] != "198.18.0.1" || host["enc_role"] != "roles::dns" {
+		t.Fatalf("expected both facts under host, got: %v", host)
+	}
+}
+
+// ---- matchValue (positional fallback for `-pm value`) -----------------------
+
+func TestMatchValue_FlagWins(t *testing.T) {
+	// An explicit -m value takes precedence over any positional arg.
+	if got := matchValue("flagval", []string{"posval"}); got != "flagval" {
+		t.Fatalf("expected flag value to win, got %q", got)
+	}
+}
+
+func TestMatchValue_PositionalFallback(t *testing.T) {
+	// This is the `-pm k8s` case: pflag leaves k8s as a positional because the
+	// grouped -m flag does not attach the space-separated value.
+	if got := matchValue("", []string{"k8s"}); got != "k8s" {
+		t.Fatalf("expected positional fallback, got %q", got)
+	}
+}
+
+func TestMatchValue_NoneGiven(t *testing.T) {
+	if got := matchValue("", nil); got != "" {
+		t.Fatalf("expected empty, got %q", got)
+	}
+}
+
 // ---- valueString / valueAny -------------------------------------------------
 
 func TestValueString_String(t *testing.T) {
@@ -677,6 +774,37 @@ func TestRun_Ansible(t *testing.T) {
 	if !strings.Contains(out, "hosta:") || !strings.Contains(out, "hostb:") {
 		t.Fatalf("expected both hosts in inventory, got: %q", out)
 	}
+	// The queried fact is attached as a host var.
+	if !strings.Contains(out, "enc_role: roles::db") || !strings.Contains(out, "enc_role: roles::web") {
+		t.Fatalf("expected fact host vars in inventory, got: %q", out)
+	}
+}
+
+func TestRun_Ansible_MultipleFacts(t *testing.T) {
+	// -F ipaddress,enc_role -A must include both facts as host vars.
+	facts := []fact{
+		{Certname: "hosta", Name: "ipaddress", Value: rawJSON("198.18.0.1")},
+		{Certname: "hosta", Name: "enc_role", Value: rawJSON("roles::dns")},
+	}
+	out := runToString(t, facts, func(a *runArgs) {
+		a.showRole = false
+		a.factName = "ipaddress,enc_role"
+		a.ansible = true
+	})
+
+	// Parse it back as YAML and assert the structure precisely.
+	var inv struct {
+		All struct {
+			Hosts map[string]map[string]interface{} `yaml:"hosts"`
+		} `yaml:"all"`
+	}
+	if err := yaml.Unmarshal([]byte(out), &inv); err != nil {
+		t.Fatalf("inventory is not valid YAML: %v (%s)", err, out)
+	}
+	host := inv.All.Hosts["hosta"]
+	if host["ipaddress"] != "198.18.0.1" || host["enc_role"] != "roles::dns" {
+		t.Fatalf("expected both facts as host vars, got: %v", host)
+	}
 }
 
 func TestRun_AllFacts_PrintsSortedByName(t *testing.T) {