diff --git a/CHANGELOG.md b/CHANGELOG.md index ecaf1a6c5..7c1cd2f28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,12 @@ ### Changed +- Internal, behavior-preserving refactor of CLI option intake and fact + disabling/gating metadata: runtime parsing now derives from the shared option + registry, core fact gating and built-in fact groups derive from one engine + descriptor table, and CLI fast-path/list-group external directory planning + uses the engine's discovery planning seam. No CLI flags, output bytes, + diagnostics, input precedence, or fact shapes change. - Internal, behavior-preserving refactor of the discovery engine: fact resolvers now reach the host only through the run-scoped Session seam, with an automated check freezing that boundary. Category assembly reads platform diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1ddd3b34e..439921585 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -138,6 +138,15 @@ deltas against the capabilities in `openspec/specs/`: Record user-visible behavior changes in `CHANGELOG.md` under Unreleased. +## CLI task semantics + +The `--list-block-groups` and `--list-cache-groups` tasks intentionally do +less than normal fact queries. They parse options through the shared CLI intake, +but only `--config` and `--external-dir` affect group listing; every other +accepted option, including `--no-external-facts`, `--no-cache`, `--disable`, +`--no-block`, and `--debug`, is inert for these tasks. Changing this frozen +semantics is a user-visible behavior change and needs its own OpenSpec change. + ## Parity questions Ruby Facter compatibility is promised at the CLI process boundary (output diff --git a/cmd/facts/main_test.go b/cmd/facts/main_test.go index 3c546b7e8..9df6a78cb 100644 --- a/cmd/facts/main_test.go +++ b/cmd/facts/main_test.go @@ -111,6 +111,51 @@ func TestRunMainReportsGenericErrors(t *testing.T) { } } +func TestRunMainReportsRuntimeOptionErrorsWithoutOptionsValidatorPrefix(t *testing.T) { + externalConflictConfig := filepath.Join(t.TempDir(), "external-conflict.conf") + if err := os.WriteFile(externalConflictConfig, []byte("global : {\n external-dir : [ \"/facts\" ],\n}\n"), 0o600); err != nil { + t.Fatal(err) + } + logConflictConfig := filepath.Join(t.TempDir(), "log-conflict.conf") + if err := os.WriteFile(logConflictConfig, []byte("cli : {\n debug : true,\n log-level : info,\n}\n"), 0o600); err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + args []string + wantStderr string + }{ + { + name: "external facts conflict after config parse", + args: []string{"--config", externalConflictConfig, "--no-external-facts", "facterversion"}, + wantStderr: "--no-external-facts and --external-dir options conflict: please specify only one\n", + }, + { + name: "log option conflict after config parse", + args: []string{"--config", logConflictConfig, "facterversion"}, + wantStderr: "debug, verbose, and log-level options conflict: please specify only one.\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var stdout, stderr bytes.Buffer + + if code := runMain(&stdout, &stderr, tt.args); code != 1 { + t.Fatalf("runMain() code = %d, want 1", code) + } + assertMainUsageOutput(t, stdout.String()) + if got := stderr.String(); got != tt.wantStderr { + t.Fatalf("stderr = %q, want %q", got, tt.wantStderr) + } + if strings.Contains(stderr.String(), "Facts::OptionsValidator") { + t.Fatalf("stderr = %q, want runtime error without OptionsValidator prefix", stderr.String()) + } + }) + } +} + func TestFactsCommand_noQueryPrintsStructuredFacts(t *testing.T) { bin := buildFactsCommand(t) @@ -248,6 +293,15 @@ func (w errorWriter) Write([]byte) (int, error) { return 0, w.err } +func assertMainUsageOutput(t *testing.T, got string) { + t.Helper() + for _, want := range []string{"Usage", "facts [options] [query]"} { + if !strings.Contains(got, want) { + t.Fatalf("stdout = %q, want %q", got, want) + } + } +} + func buildFactsCommand(t *testing.T) string { t.Helper() commandBuild.once.Do(func() { diff --git a/internal/app/app.go b/internal/app/app.go index 1d1fb2c89..d4ad4531d 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -3,7 +3,6 @@ package app import ( "context" "errors" - "flag" "fmt" "io" "log/slog" @@ -59,18 +58,20 @@ func Run(stdout, stderr io.Writer, args []string) error { } func factGroups(args []string) ([]engine.FactGroup, error) { + options := parseListOptions(args) groups := engine.BuiltinFactGroups() - configPath := configPathFromArgs(args) - config, err := engine.ParseConfig(configPath, slog.New(slog.DiscardHandler)) + config, err := engine.ParseConfig(options.ConfigPath, slog.New(slog.DiscardHandler)) if err != nil { return nil, err } groups = engine.MergeFactGroups(groups, config.FactGroups) - externalDirs := externalDirsFromArgs(args) - if len(externalDirs) == 0 { - externalDirs = config.ExternalDirs + config.NoExternalFacts = false + configuredExternalDirs := engine.DiscoveryExternalDirs(config, options.ExternalDirs, false, false, nil) + var defaultExternalDirs []string + if len(configuredExternalDirs) == 0 { + defaultExternalDirs = defaultExternalFactDirs() } - externalDirs = effectiveExternalDirs(externalDirs) + externalDirs := engine.DiscoveryExternalDirs(config, options.ExternalDirs, false, true, defaultExternalDirs) external, err := engine.ExternalFactGroups(externalDirs) if err != nil { return nil, err @@ -78,72 +79,6 @@ func factGroups(args []string) ([]engine.FactGroup, error) { return engine.MergeFactGroups(groups, external), nil } -func effectiveExternalDirs(explicit []string) []string { - if len(explicit) > 0 { - return explicit - } - return defaultExternalFactDirs() -} - -func configPathFromArgs(args []string) string { - for i := 0; i < len(args); i++ { - arg := args[i] - option, ok := cli.LookupOption(arg) - if !ok { - continue - } - if value, hasInlineValue := inlineOptionValue(arg); hasInlineValue { - if option.Canonical == "--config" { - return value - } - continue - } - if option.Canonical == "--config" { - if i+1 < len(args) { - return args[i+1] - } - return "" - } - if option.Arity == cli.RequiredValue && i+1 < len(args) { - i++ - } - } - return "" -} - -func externalDirsFromArgs(args []string) []string { - dirs := []string{} - for i := 0; i < len(args); i++ { - arg := args[i] - option, ok := cli.LookupOption(arg) - if !ok { - continue - } - if value, hasInlineValue := inlineOptionValue(arg); hasInlineValue { - if option.Canonical == "--external-dir" { - dirs = append(dirs, value) - } - continue - } - if option.Canonical == "--external-dir" { - if i+1 < len(args) { - dirs = append(dirs, args[i+1]) - i++ - } - continue - } - if option.Arity == cli.RequiredValue && i+1 < len(args) { - i++ - } - } - return dirs -} - -func inlineOptionValue(arg string) (string, bool) { - _, value, ok := strings.Cut(arg, "=") - return value, ok -} - func helpText() string { var b strings.Builder b.WriteString(`Usage @@ -215,101 +150,65 @@ Format facts as JSON: } func runQuery(stdout, stderr io.Writer, args []string) error { - flags := flag.NewFlagSet("query", flag.ContinueOnError) - flags.SetOutput(stderr) - jsonOutput := flags.Bool("json", false, "render facts as JSON") - jsonOutputShort := flags.Bool("j", false, "render facts as JSON") - flags.Bool("no-json", false, "do not render facts as JSON") - yamlOutput := flags.Bool("yaml", false, "render facts as YAML") - yamlOutputShort := flags.Bool("y", false, "render facts as YAML") - flags.Bool("no-yaml", false, "do not render facts as YAML") - hoconOutput := flags.Bool("hocon", false, "render facts as HOCON") - flags.Bool("no-hocon", false, "do not render facts as HOCON") - debug := flags.Bool("debug", false, "write debug logs") - debugShort := flags.Bool("d", false, "write debug logs") - color := flags.Bool("color", false, "colorize diagnostic output") - noColor := flags.Bool("no-color", false, "disable colorized diagnostic output") - flags.String("log-level", "", "accepted for Facter compatibility") - flags.String("l", "", "accepted for Facter compatibility") - timing := flags.Bool("timing", false, "write fact timing") - timingShort := flags.Bool("t", false, "write fact timing") - strict := flags.Bool("strict", false, "return an error when queried facts are missing") - forceDotResolution := flags.Bool("force-dot-resolution", false, "merge dotted facts into structured facts") - configPath := flags.String("config", "", "load configuration from file") - configPathShort := flags.String("c", "", "load configuration from file") - noBlock := flags.Bool("no-block", false, "disable fact blocking") - noCache := flags.Bool("no-cache", false, "disable loading and refreshing facts from the cache") - verbose := flags.Bool("verbose", false, "write info logs") - flags.Bool("sequential", false, "accepted for Facter compatibility") - flags.Bool("http-debug", false, "accepted for Facter compatibility") - noExternalFacts := flags.Bool("no-external-facts", false, "accepted for Facter compatibility") - var externalDirs []string - flags.Func("external-dir", "load external facts from directory", func(value string) error { - externalDirs = append(externalDirs, value) - return nil - }) - var disableEntries []string - flags.Func("disable", "disable facts or fact groups (comma-separated, repeatable)", func(value string) error { - disableEntries = append(disableEntries, engine.SplitDisableList(value)...) - return nil - }) - if err := flags.Parse(args); err != nil { + flags, options, err := parseOptions("query", stderr, args) + if err != nil { return err } - colorOutput := resolveColor(*color, *noColor, stdout) - colorDiagnostics := resolveColor(*color, *noColor, stderr) + colorOutput := resolveColor(options.Color, options.NoColor, stdout) + colorDiagnostics := resolveColor(options.Color, options.NoColor, stderr) // One diagnostics sink for the whole run: engine diagnostics (config, cache, // collection, …) and CLI diagnostics share this handler. debug/verbose are // set once resolved below; warn-class (the only level ParseConfig emits) is // always enabled, so config diagnostics render identically before that. logHandler := &stderrLogHandler{stderr: stderr, color: colorDiagnostics} logger := slog.New(logHandler) - configFile := firstNonEmpty(*configPath, *configPathShort) + configFile := options.ConfigPath configOptions, configErr := engine.ParseConfig(configFile, logger) if configErr != nil { return configErr } - cliExternalDirs := externalDirs - discoveryExternalDirs := externalDirs - if len(discoveryExternalDirs) == 0 { - discoveryExternalDirs = configOptions.ExternalDirs - } + cliExternalDirs := options.ExternalDirs + configForConflict := configOptions + configForConflict.NoExternalFacts = false + conflictExternalDirs := engine.DiscoveryExternalDirs(configForConflict, options.ExternalDirs, false, false, nil) if configOptions.NoExternalFacts { - *noExternalFacts = true + options.NoExternalFacts = true } if configOptions.Debug { - *debug = true + options.Debug = true } if configOptions.Verbose { - *verbose = true + options.Verbose = true } - if *noExternalFacts && hasNonEmpty(discoveryExternalDirs) { + if options.NoExternalFacts && hasNonEmpty(conflictExternalDirs) { return optionError(stdout, errors.New("--no-external-facts and --external-dir options conflict: please specify only one")) } - if !*noExternalFacts { - discoveryExternalDirs = effectiveExternalDirs(discoveryExternalDirs) + var defaultExternalDirs []string + if !options.NoExternalFacts && len(conflictExternalDirs) == 0 { + defaultExternalDirs = defaultExternalFactDirs() } + discoveryExternalDirs := engine.DiscoveryExternalDirs(configOptions, options.ExternalDirs, options.NoExternalFacts, true, defaultExternalDirs) disabledFactsForFastPath := map[string]bool{} var disabledFacts map[string]bool - if *noBlock { + if options.NoBlock { // --no-block is the master override: an empty non-nil map clears the // whole disabled set for this run, including --disable and FACTS_DISABLE. disabledFacts = map[string]bool{} } - if !*noBlock { + if !options.NoBlock { // The fast path honors every disable source so a disabled facterversion // query falls through to normal resolution (and disable-beats-query). It // derives its set from the same engine union discovery planning uses, so // the two can never disagree on whether facterversion is disabled. - disabledFactsForFastPath = engine.DisabledUnion(configOptions, disableEntries, os.Environ()) + disabledFactsForFastPath = engine.DisabledUnion(configOptions, options.DisableEntries, os.Environ()) } - mergeDottedFacts := configOptions.ForceDotResolution || *forceDotResolution - logLevel := firstNonEmpty(flags.Lookup("log-level").Value.String(), flags.Lookup("l").Value.String(), configOptions.LogLevel) + mergeDottedFacts := configOptions.ForceDotResolution || options.ForceDotResolution + logLevel := firstNonEmpty(options.LogLevel, configOptions.LogLevel) if logLevel != "" && !cli.SupportedLogLevel(logLevel) { return optionError(stdout, fmt.Errorf("unsupported log level %s", logLevel)) } - debugEnabled := *debug || *debugShort - verboseEnabled := *verbose + debugEnabled := options.Debug + verboseEnabled := options.Verbose if resolvedLogOptionsConflict(debugEnabled, verboseEnabled, logLevel) { return optionError(stdout, errors.New("debug, verbose, and log-level options conflict: please specify only one.")) } @@ -322,8 +221,8 @@ func runQuery(stdout, stderr io.Writer, args []string) error { writeInfo(stderr, "executed with command line: "+strings.Join(args, " "), colorDiagnostics) writeInfo(stderr, "resolving facts", colorDiagnostics) } - if canUseVersionQueryFastPath(flags.Args(), discoveryExternalDirs, disabledFactsForFastPath, *noExternalFacts, *timing || *timingShort) { - return writeVersionQuery(stdout, *jsonOutput || *jsonOutputShort, *yamlOutput || *yamlOutputShort, *hoconOutput) + if canUseVersionQueryFastPath(flags.Args(), discoveryExternalDirs, disabledFactsForFastPath, options.NoExternalFacts, options.Timing) { + return writeVersionQuery(stdout, options.JSON, options.YAML, options.HOCON) } resolutionStart := time.Now() @@ -334,12 +233,12 @@ func runQuery(stdout, stderr io.Writer, args []string) error { ConfigLoaded: true, Config: configOptions, ExternalDirs: cliExternalDirs, - UseCache: !*noCache, - NoExternalFacts: *noExternalFacts, + UseCache: !options.NoCache, + NoExternalFacts: options.NoExternalFacts, DisabledFacts: disabledFacts, - ExtraDisabled: disableEntries, + ExtraDisabled: options.DisableEntries, DefaultExternalDirsSet: true, - DefaultExternalDirs: defaultExternalFactDirs(), + DefaultExternalDirs: defaultExternalDirs, IncludeTypedDotted: mergeDottedFacts, Logger: logger, }) @@ -353,16 +252,16 @@ func runQuery(stdout, stderr io.Writer, args []string) error { facts := snapshot.Facts() resolutionDuration := time.Since(resolutionStart).Seconds() out, err := engine.BuildFormatter(engine.FormatOptions{ - JSON: *jsonOutput || *jsonOutputShort, - YAML: *yamlOutput || *yamlOutputShort, - HOCON: *hoconOutput, + JSON: options.JSON, + YAML: options.YAML, + HOCON: options.HOCON, IncludeTypedDotted: mergeDottedFacts, Colorize: colorOutput, }).Format(facts) if err != nil { return err } - if *timing || *timingShort { + if options.Timing { for _, fact := range facts { name := fact.UserQuery if name == "" { @@ -383,7 +282,7 @@ func runQuery(stdout, stderr io.Writer, args []string) error { return err } } - if *strict { + if options.Strict { missing := engine.NewProjection(facts, mergeDottedFacts).MissingQueries(facts) if len(missing) > 0 { for _, name := range missing { diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 5b4d3888e..def7ac268 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -90,6 +90,32 @@ func TestRun_facterversionHOCONFastPathBytes(t *testing.T) { } } +func TestRun_facterversionFastPathIgnoresColorAndForceDotResolution(t *testing.T) { + tests := []struct { + name string + args []string + }{ + {name: "color", args: []string{"--color", "facterversion"}}, + {name: "force dot resolution", args: []string{"--force-dot-resolution", "facterversion"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var stdout, stderr bytes.Buffer + + if err := Run(&stdout, &stderr, tt.args); err != nil { + t.Fatal(err) + } + if got, want := stdout.String(), engine.Version+"\n"; got != want { + t.Fatalf("stdout = %q, want %q", got, want) + } + if stderr.Len() != 0 { + t.Fatalf("stderr = %q, want empty", stderr.String()) + } + }) + } +} + func TestRun_facterversionQueryAllowsExternalOverride(t *testing.T) { dir := t.TempDir() if err := os.WriteFile(filepath.Join(dir, "version.txt"), []byte("facterversion=external\n"), 0o600); err != nil { @@ -637,6 +663,71 @@ func TestRun_strictLogsMissingFactErrorWhenQueriedFactIsMissing(t *testing.T) { } } +func TestRun_noCacheDisabledQueriedFactKeepsProjectionPlaceholder(t *testing.T) { + tests := []struct { + name string + args []string + wantStdout string + wantStderr string + wantStatus int + }{ + { + name: "legacy", + args: []string{"--no-cache", "--disable", "facterversion", "facterversion"}, + wantStdout: "", + }, + { + name: "json", + args: []string{"--no-cache", "--disable", "facterversion", "--json", "facterversion"}, + wantStdout: "{\n \"facterversion\": null\n}\n", + }, + { + name: "yaml", + args: []string{"--no-cache", "--disable", "facterversion", "--yaml", "facterversion"}, + wantStdout: "facterversion: \"\"\n", + }, + { + name: "hocon", + args: []string{"--no-cache", "--disable", "facterversion", "--hocon", "facterversion"}, + wantStdout: "", + }, + { + name: "strict", + args: []string{"--no-cache", "--disable", "facterversion", "--strict", "facterversion"}, + wantStdout: "", + wantStderr: "ERROR Facts - fact \"facterversion\" does not exist.\n", + wantStatus: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var stdout, stderr bytes.Buffer + + err := Run(&stdout, &stderr, tt.args) + if tt.wantStatus == 0 { + if err != nil { + t.Fatalf("Run(%v) err = %v, want nil", tt.args, err) + } + } else { + status, ok := err.(ExitStatus) + if !ok { + t.Fatalf("Run(%v) err = %T %[2]v, want ExitStatus", tt.args, err) + } + if status.Code() != tt.wantStatus { + t.Fatalf("Run(%v) status = %d, want %d", tt.args, status.Code(), tt.wantStatus) + } + } + if got := stdout.String(); got != tt.wantStdout { + t.Fatalf("stdout = %q, want %q", got, tt.wantStdout) + } + if got := stderr.String(); got != tt.wantStderr { + t.Fatalf("stderr = %q, want %q", got, tt.wantStderr) + } + }) + } +} + func TestRun_queryNoJSONUsesLegacyOutput(t *testing.T) { var stdout, stderr bytes.Buffer @@ -651,6 +742,24 @@ func TestRun_queryNoJSONUsesLegacyOutput(t *testing.T) { } } +func TestRun_noFormatCompatibilityFlagsAreAcceptedAndInert(t *testing.T) { + for _, flag := range []string{"--no-json", "--no-yaml", "--no-hocon"} { + t.Run(flag, func(t *testing.T) { + var stdout, stderr bytes.Buffer + + if err := Run(&stdout, &stderr, []string{flag, "facterversion"}); err != nil { + t.Fatal(err) + } + if got, want := stdout.String(), engine.Version+"\n"; got != want { + t.Fatalf("stdout = %q, want %q", got, want) + } + if stderr.Len() != 0 { + t.Fatalf("stderr = %q, want empty", stderr.String()) + } + }) + } +} + func TestRun_timingPrintsResolutionDuration(t *testing.T) { var stdout, stderr bytes.Buffer @@ -1612,6 +1721,81 @@ func assertUsageOutput(t *testing.T, got string) { } } +func runAppOutput(t *testing.T, args ...string) (string, string) { + t.Helper() + var stdout, stderr bytes.Buffer + if err := Run(&stdout, &stderr, args); err != nil { + t.Fatalf("Run(%v) err = %v", args, err) + } + return stdout.String(), stderr.String() +} + +func TestRun_listTaskNonConfigOptionsAreInert(t *testing.T) { + tasks := []string{"--list-cache-groups", "--list-block-groups"} + options := []struct { + name string + args []string + }{ + {name: "no external facts", args: []string{"--no-external-facts"}}, + {name: "no cache", args: []string{"--no-cache"}}, + {name: "disable", args: []string{"--disable", "networking"}}, + {name: "no block", args: []string{"--no-block"}}, + {name: "debug", args: []string{"--debug"}}, + } + + for _, task := range tasks { + t.Run(task, func(t *testing.T) { + wantStdout, wantStderr := runAppOutput(t, task) + for _, option := range options { + t.Run(option.name, func(t *testing.T) { + args := append([]string{task}, option.args...) + gotStdout, gotStderr := runAppOutput(t, args...) + if gotStdout != wantStdout { + t.Fatalf("stdout = %q, want bare task output %q", gotStdout, wantStdout) + } + if gotStderr != wantStderr { + t.Fatalf("stderr = %q, want bare task stderr %q", gotStderr, wantStderr) + } + }) + } + }) + } +} + +func TestRun_listTasksIgnoreTrailingTaskFlags(t *testing.T) { + tests := []struct { + name string + args []string + want string + }{ + { + name: "cache groups ignore version", + args: []string{"--list-cache-groups", "--version"}, + want: "memory\n- memory", + }, + { + name: "block groups ignore help", + args: []string{"--list-block-groups", "-h"}, + want: "networking\n- networking", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var stdout, stderr bytes.Buffer + if err := Run(&stdout, &stderr, tt.args); err != nil { + t.Fatalf("Run(%v) err = %v, want nil", tt.args, err) + } + if !strings.Contains(stdout.String(), tt.want) { + t.Fatalf("stdout = %q, want substring %q", stdout.String(), tt.want) + } + if stderr.Len() != 0 { + t.Fatalf("stderr = %q, want empty", stderr.String()) + } + }) + } +} + func TestRun_listBlockGroupsPrintsFactGroups(t *testing.T) { var stdout, stderr bytes.Buffer @@ -1644,6 +1828,106 @@ func TestRun_listCacheGroupsPrintsFactGroups(t *testing.T) { } } +func TestRun_listCacheGroupsSkipsInterleavedValuedOptions(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "facter.conf") + content := `fact-groups : { + pinned-config : [ "from_config" ], +}` + if err := os.WriteFile(configPath, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "from-dir.yaml"), []byte("site_role: web\n"), 0o600); err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + args []string + }{ + { + name: "separate values", + args: []string{"--list-cache-groups", "-l", "debug", "-c", configPath, "--external-dir", dir}, + }, + { + name: "attached long values", + args: []string{"--list-cache-groups", "--log-level=debug", "--config=" + configPath, "--external-dir=" + dir}, + }, + { + name: "attached short log level and config", + args: []string{"--list-cache-groups", "-ldebug", "-c=" + configPath, "--external-dir", dir}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + stdout, stderr := runAppOutput(t, tt.args...) + for _, want := range []string{"pinned-config\n- from_config", "from-dir.yaml\n"} { + if !strings.Contains(stdout, want) { + t.Fatalf("stdout = %q, want parsed group %q", stdout, want) + } + } + if strings.Contains(stdout, "debug\n") { + t.Fatalf("stdout = %q, want log-level value skipped, not treated as an external group", stdout) + } + if stderr != "" { + t.Fatalf("stderr = %q, want empty", stderr) + } + }) + } +} + +func TestRun_listTasksScanWholeTailForConfigAndExternalDirs(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "facter.conf") + content := `fact-groups : { + pinned-config : [ "from_config" ], +}` + if err := os.WriteFile(configPath, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "from-dir.yaml"), []byte("site_role: web\n"), 0o600); err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + args []string + want string + }{ + { + name: "config after positional", + args: []string{"--list-cache-groups", "bogus", "--config", configPath}, + want: "pinned-config\n- from_config", + }, + { + name: "config after delimiter", + args: []string{"--list-cache-groups", "--", "--config", configPath}, + want: "pinned-config\n- from_config", + }, + { + name: "external dir after positional", + args: []string{"--list-block-groups", "bogus", "--external-dir", dir}, + want: "from-dir.yaml\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var stdout, stderr bytes.Buffer + if err := Run(&stdout, &stderr, tt.args); err != nil { + t.Fatalf("Run(%v) err = %v, want nil", tt.args, err) + } + if !strings.Contains(stdout.String(), tt.want) { + t.Fatalf("stdout = %q, want substring %q", stdout.String(), tt.want) + } + if stderr.Len() != 0 { + t.Fatalf("stderr = %q, want empty", stderr.String()) + } + }) + } +} + func TestRun_listCacheGroupsIncludesConfiguredFactGroups(t *testing.T) { configPath := filepath.Join(t.TempDir(), "facter.conf") content := `fact-groups : { diff --git a/internal/app/options.go b/internal/app/options.go new file mode 100644 index 000000000..f2f9a0592 --- /dev/null +++ b/internal/app/options.go @@ -0,0 +1,164 @@ +package app + +import ( + "flag" + "fmt" + "io" + "strings" + + "github.com/ncode/facts/internal/cli" + "github.com/ncode/facts/internal/engine" +) + +type parsedOptions struct { + JSON bool + NoJSON bool + YAML bool + NoYAML bool + HOCON bool + NoHOCON bool + Debug bool + Color bool + NoColor bool + Timing bool + Strict bool + ForceDotResolution bool + NoBlock bool + NoCache bool + Verbose bool + Sequential bool + HTTPDebug bool + NoExternalFacts bool + + ConfigPath string + LogLevel string + ExternalDirs []string + DisableEntries []string +} + +type optionBinder func(*flag.FlagSet, string) + +func parseOptions(name string, output io.Writer, args []string) (*flag.FlagSet, *parsedOptions, error) { + flags, parsed, err := newParsedOptionFlagSet(name, output) + if err != nil { + return nil, nil, err + } + if err := flags.Parse(args); err != nil { + return nil, nil, err + } + return flags, parsed, nil +} + +func parseListOptions(args []string) *parsedOptions { + parsed := &parsedOptions{} + for i := 0; i < len(args); i++ { + arg := args[i] + option, ok := cli.LookupOption(arg) + if !ok || option.TaskFlag { + continue + } + value, hasInlineValue := inlineOptionValue(arg) + switch option.Canonical { + case "--config": + if hasInlineValue { + parsed.ConfigPath = value + } else if i+1 < len(args) { + parsed.ConfigPath = args[i+1] + i++ + } + case "--external-dir": + if hasInlineValue { + parsed.ExternalDirs = append(parsed.ExternalDirs, value) + } else if i+1 < len(args) { + parsed.ExternalDirs = append(parsed.ExternalDirs, args[i+1]) + i++ + } + default: + if option.Arity == cli.RequiredValue && !hasInlineValue && i+1 < len(args) { + i++ + } + } + } + return parsed +} + +func newParsedOptionFlagSet(name string, output io.Writer) (*flag.FlagSet, *parsedOptions, error) { + flags := flag.NewFlagSet(name, flag.ContinueOnError) + flags.SetOutput(output) + parsed := &parsedOptions{} + bindings := parsed.optionBindings() + for _, option := range cli.Options() { + if option.TaskFlag { + continue + } + bind, ok := bindings[option.Canonical] + if !ok { + return nil, nil, fmt.Errorf("missing parser binding for %s", option.Canonical) + } + bind(flags, flagName(option.Canonical)) + for _, alias := range option.Aliases { + bind(flags, flagName(alias)) + } + } + return flags, parsed, nil +} + +func (p *parsedOptions) optionBindings() map[string]optionBinder { + bindBool := func(dst *bool, usage string) optionBinder { + return func(flags *flag.FlagSet, name string) { + flags.BoolVar(dst, name, false, usage) + } + } + bindString := func(dst *string, usage string) optionBinder { + return func(flags *flag.FlagSet, name string) { + flags.StringVar(dst, name, "", usage) + } + } + return map[string]optionBinder{ + "--color": bindBool(&p.Color, "colorize diagnostic output"), + "--no-color": bindBool(&p.NoColor, "disable colorized diagnostic output"), + "--config": bindString(&p.ConfigPath, "load configuration from file"), + "--debug": bindBool(&p.Debug, "write debug logs"), + "--disable": p.bindDisable, + "--external-dir": p.bindExternalDir, + "--force-dot-resolution": bindBool(&p.ForceDotResolution, "merge dotted facts into structured facts"), + "--hocon": bindBool(&p.HOCON, "render facts as HOCON"), + "--json": bindBool(&p.JSON, "render facts as JSON"), + "--log-level": bindString(&p.LogLevel, "accepted for Facter compatibility"), + "--no-block": bindBool(&p.NoBlock, "disable fact blocking"), + "--no-cache": bindBool(&p.NoCache, "disable loading and refreshing facts from the cache"), + "--no-external-facts": bindBool(&p.NoExternalFacts, "accepted for Facter compatibility"), + "--no-hocon": bindBool(&p.NoHOCON, "do not render facts as HOCON"), + "--no-json": bindBool(&p.NoJSON, "do not render facts as JSON"), + "--no-yaml": bindBool(&p.NoYAML, "do not render facts as YAML"), + "--verbose": bindBool(&p.Verbose, "write info logs"), + "--yaml": bindBool(&p.YAML, "render facts as YAML"), + "--strict": bindBool(&p.Strict, "return an error when queried facts are missing"), + "--timing": bindBool(&p.Timing, "write fact timing"), + "--sequential": bindBool(&p.Sequential, "accepted for Facter compatibility"), + "--http-debug": bindBool(&p.HTTPDebug, "accepted for Facter compatibility"), + } +} + +func (p *parsedOptions) bindExternalDir(flags *flag.FlagSet, name string) { + flags.Func(name, "load external facts from directory", func(value string) error { + p.ExternalDirs = append(p.ExternalDirs, value) + return nil + }) +} + +func (p *parsedOptions) bindDisable(flags *flag.FlagSet, name string) { + flags.Func(name, "disable facts or fact groups (comma-separated, repeatable)", func(value string) error { + p.DisableEntries = append(p.DisableEntries, engine.SplitDisableList(value)...) + return nil + }) +} + +func flagName(option string) string { + return strings.TrimLeft(option, "-") +} + +func inlineOptionValue(arg string) (string, bool) { + _, value, ok := strings.Cut(arg, "=") + return value, ok +} diff --git a/internal/app/options_test.go b/internal/app/options_test.go new file mode 100644 index 000000000..cd18b91b3 --- /dev/null +++ b/internal/app/options_test.go @@ -0,0 +1,99 @@ +package app + +import ( + "flag" + "io" + "testing" + + "github.com/ncode/facts/internal/cli" +) + +func TestParsedOptionFlagSetMatchesRegistry(t *testing.T) { + flags, _, err := newParsedOptionFlagSet("test", io.Discard) + if err != nil { + t.Fatal(err) + } + + wantNames := map[string]bool{} + wantCanonicals := map[string]bool{} + for _, option := range cli.Options() { + if option.TaskFlag { + continue + } + wantCanonicals[option.Canonical] = true + wantNames[flagName(option.Canonical)] = true + for _, alias := range option.Aliases { + wantNames[flagName(alias)] = true + } + } + + gotNames := map[string]bool{} + flags.VisitAll(func(f *flag.Flag) { + gotNames[f.Name] = true + }) + for name := range wantNames { + if !gotNames[name] { + t.Fatalf("parser missing registry option %q", name) + } + } + for name := range gotNames { + if !wantNames[name] { + t.Fatalf("parser accepts %q outside registry", name) + } + } + + bindings := (&parsedOptions{}).optionBindings() + for canonical := range wantCanonicals { + if bindings[canonical] == nil { + t.Fatalf("registry option %q has no parser binding", canonical) + } + } +} + +func TestParsedOptionFlagSetKeepsHeadUsageStrings(t *testing.T) { + flags, _, err := newParsedOptionFlagSet("test", io.Discard) + if err != nil { + t.Fatal(err) + } + + want := map[string]string{ + "json": "render facts as JSON", + "j": "render facts as JSON", + "no-json": "do not render facts as JSON", + "yaml": "render facts as YAML", + "y": "render facts as YAML", + "no-yaml": "do not render facts as YAML", + "hocon": "render facts as HOCON", + "no-hocon": "do not render facts as HOCON", + "debug": "write debug logs", + "d": "write debug logs", + "color": "colorize diagnostic output", + "no-color": "disable colorized diagnostic output", + "log-level": "accepted for Facter compatibility", + "l": "accepted for Facter compatibility", + "timing": "write fact timing", + "t": "write fact timing", + "strict": "return an error when queried facts are missing", + "force-dot-resolution": "merge dotted facts into structured facts", + "config": "load configuration from file", + "c": "load configuration from file", + "no-block": "disable fact blocking", + "no-cache": "disable loading and refreshing facts from the cache", + "verbose": "write info logs", + "sequential": "accepted for Facter compatibility", + "http-debug": "accepted for Facter compatibility", + "no-external-facts": "accepted for Facter compatibility", + "external-dir": "load external facts from directory", + "disable": "disable facts or fact groups (comma-separated, repeatable)", + } + + for name, usage := range want { + got := flags.Lookup(name) + if got == nil { + t.Fatalf("flags.Lookup(%q) = nil", name) + } + if got.Usage != usage { + t.Fatalf("usage for %q = %q, want %q", name, got.Usage, usage) + } + } +} diff --git a/internal/engine/cache_test.go b/internal/engine/cache_test.go index 6622fce7c..34abe4aa8 100644 --- a/internal/engine/cache_test.go +++ b/internal/engine/cache_test.go @@ -1,7 +1,9 @@ package engine import ( + "context" "encoding/json" + "errors" "os" "path/filepath" "reflect" @@ -288,6 +290,66 @@ func TestFactCache_cacheFactsWritesConfiguredGroups(t *testing.T) { } } +func TestDiscover_disabledFactIsNotServedFromFreshCache(t *testing.T) { + cacheDir := t.TempDir() + writeJSONFile(t, filepath.Join(cacheDir, "networking"), map[string]any{ + "cache_format_version": float64(1), + "networking.hostname": "cached-host", + }) + oldDefaultCachePath := DefaultCachePath + DefaultCachePath = func() string { return cacheDir } + t.Cleanup(func() { DefaultCachePath = oldDefaultCachePath }) + + eng, err := NewEngine(EngineConfig{ + UseCache: true, + ConfigLoaded: true, + Config: Config{TTLs: []FactTTL{{Fact: "networking", TTL: "1 hour"}}}, + ExtraDisabled: []string{"networking.hostname"}, + }) + if err != nil { + t.Fatal(err) + } + snap, err := eng.Discover(context.Background(), "networking.hostname") + if err != nil { + t.Fatal(err) + } + + if got, err := snap.Value("networking.hostname"); got != nil || !errors.Is(err, ErrFactNotFound) { + t.Fatalf("Value(networking.hostname) = %#v, %v; want ErrFactNotFound, not cached value", got, err) + } +} + +func TestDiscover_prunedSubfactIsNotCached(t *testing.T) { + cacheDir := t.TempDir() + oldDefaultCachePath := DefaultCachePath + DefaultCachePath = func() string { return cacheDir } + t.Cleanup(func() { DefaultCachePath = oldDefaultCachePath }) + + eng, err := NewEngine(EngineConfig{ + UseCache: true, + ConfigLoaded: true, + Config: Config{ + Disabled: []string{"os.release"}, + TTLs: []FactTTL{{Fact: "operating system", TTL: "1 hour"}}, + }, + }) + if err != nil { + t.Fatal(err) + } + if _, err := eng.Discover(context.Background(), "os"); err != nil { + t.Fatal(err) + } + + data := readJSONFile(t, filepath.Join(cacheDir, "operating system")) + osFact, ok := data["os"].(map[string]any) + if !ok { + t.Fatalf("cached os = %#v, want object", data["os"]) + } + if _, ok := osFact["release"]; ok { + t.Fatalf("cached os = %#v, want os.release pruned before cache write", osFact) + } +} + func TestFactCache_cacheFactsReturnsNonPermissionMkdirError(t *testing.T) { dir := filepath.Join(t.TempDir(), "cache-file") if err := os.WriteFile(dir, []byte("not a directory"), 0o600); err != nil { diff --git a/internal/engine/config_test.go b/internal/engine/config_test.go index 38d83c24d..819718d54 100644 --- a/internal/engine/config_test.go +++ b/internal/engine/config_test.go @@ -1049,6 +1049,19 @@ func TestFilterDisabledFacts_blocksExactNameAndRoot(t *testing.T) { } } +func TestFilterDisabledFactsKeepsFlatDottedFactWhenMiddleSegmentDisabled(t *testing.T) { + facts := []ResolvedFact{{Name: "a.b.c", Value: "kept", Type: "file"}} + + got := FilterDisabledFacts(facts, map[string]bool{"a.b": true}) + if !reflect.DeepEqual(got, facts) { + t.Fatalf("FilterDisabledFacts(a.b) = %#v, want flat dotted fact kept %#v", got, facts) + } + + if got := FilterDisabledFacts(facts, map[string]bool{"a": true}); len(got) != 0 { + t.Fatalf("FilterDisabledFacts(a) = %#v, want fact dropped by first segment", got) + } +} + func TestFilterDisabledFacts_prunesDisabledDescendantsFromStructuredParents(t *testing.T) { facts := []ResolvedFact{ { @@ -1082,6 +1095,39 @@ func TestFilterDisabledFacts_prunesDisabledDescendantsFromStructuredParents(t *t } } +func TestFilterDisabledFactsPrunesDescendantsByDisabledKeyPresence(t *testing.T) { + facts := []ResolvedFact{ + { + Name: "os", + Value: map[string]any{ + "name": "Ubuntu", + "release": map[string]any{ + "full": "24.04", + "major": "24", + }, + }, + Type: "core", + }, + } + + got := FilterDisabledFacts(facts, map[string]bool{"os.release.major": false}) + want := []ResolvedFact{ + { + Name: "os", + Value: map[string]any{ + "name": "Ubuntu", + "release": map[string]any{ + "full": "24.04", + }, + }, + Type: "core", + }, + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("FilterDisabledFacts(false-valued disabled key) = %#v, want %#v", got, want) + } +} + func TestFilterDisabledFactsPrunesEmptyMapsAndKeepsOriginalValue(t *testing.T) { original := map[string]any{ "name": "Ubuntu", diff --git a/internal/engine/core.go b/internal/engine/core.go index fc0319c8a..1f93e862c 100644 --- a/internal/engine/core.go +++ b/internal/engine/core.go @@ -57,50 +57,14 @@ func disabledFingerprint(disabled map[string]bool) string { // the composition order does not affect the resolved output; it mirrors the // historical assembly order for reviewability. func buildCoreFacts(s *Session, disabled map[string]bool) []ResolvedFact { - goos := s.goos() - virtualization := detectVirtualization(s) - virtualFact, isVirtualFact := virtualizationFactValues(virtualization) - dmi := s.cachedDMI() - facts := []ResolvedFact{ - {Name: "facterversion", Value: Version}, - {Name: "is_virtual", Value: isVirtualFact}, - {Name: "path", Value: currentPathEntries(goos, s.getenv)}, - {Name: "virtual", Value: virtualFact}, - } - // gate skips a single-output category whose only top-level fact name is - // disabled, so the resolver never runs (resolution-gating, ADR-0015). Only - // categories whose entire output shares one root and whose probes are not - // needed by a kept fact are gated; multi-output (os/disks/uptime), - // shared-probe (identity, dmi), inline, and cloud/hypervisor facts below - // stay eager and rely on FilterDisabledFacts resolve-then-prune. selinux is - // likewise eager: it emits as os.selinux.* descendants, so a name-level - // disable of "selinux" is a no-op — it is disabled via os.selinux. - gate := func(fact string, resolve func(*Session) []ResolvedFact) { - if disabled[fact] { - return + build := newCoreFactBuild(s) + facts := make([]ResolvedFact, 0) + for _, descriptor := range coreFactDescriptors { + if descriptor.class == coreFactStandalone && disabled[descriptor.root] { + continue } - facts = append(facts, resolve(s)...) - } - gate("networking", networkingCoreFacts) - gate("processors", processorsCoreFacts) - gate("memory", memoryCoreFacts) - facts = append(facts, osCoreFacts(s)...) - facts = append(facts, dmiCoreFacts(s)...) - facts = append(facts, disksCoreFacts(s)...) - gate("ssh", sshCoreFacts) - facts = append(facts, identityCoreFacts(s)...) - facts = append(facts, uptimeCoreFacts(s)...) - facts = append(facts, selinuxCoreFacts(s)...) - gate("fips_enabled", fipsCoreFacts) - gate("timezone", timezoneCoreFacts) - gate("augeas", augeasCoreFacts) - gate("xen", xenCoreFacts) - gate("packages", packagesCoreFacts) - facts = append(facts, currentLinuxHypervisorFacts(s)...) - facts = append(facts, currentWindowsHypervisorFacts(s)...) - facts = append(facts, azureFacts(s.Context(), newAzureClient(azureMetadataBaseURL, nil), virtualization)...) - facts = append(facts, ec2Facts(s, newEC2Client(ec2MetadataBaseURL, nil), virtualization)...) - facts = append(facts, platformGCEFacts(s.Context(), goos, virtualization, dmiBIOSVendor(dmi), newGCEClient(gceMetadataBaseURL, nil))...) + facts = append(facts, descriptor.assemble(build)...) + } return facts } diff --git a/internal/engine/core_gating_test.go b/internal/engine/core_gating_test.go index 384b5b1a3..1b212a9aa 100644 --- a/internal/engine/core_gating_test.go +++ b/internal/engine/core_gating_test.go @@ -22,19 +22,12 @@ func hasRoot(facts []ResolvedFact, root string) bool { return rootNames(facts)[root] } -// gatedSingleOutputCategories pairs each resolution-gated core category with the -// top-level fact name that gates it (ADR-0015). -var gatedSingleOutputCategories = []string{ - "networking", "processors", "memory", "ssh", - "timezone", "fips_enabled", "augeas", "xen", "packages", -} - func TestBuildCoreFacts_resolutionGatesSingleOutputCategories(t *testing.T) { // buildCoreFacts performs no filtering, so a fact root that is absent here // can only be absent because its resolver was skipped — exactly the // resolution-gating contract. baseline := buildCoreFacts(NewSession(), nil) - for _, fact := range gatedSingleOutputCategories { + for _, fact := range standaloneCoreFactRoots() { if !hasRoot(baseline, fact) { // Not every category resolves on this host (e.g. augeas/xen). Only // assert gating for the ones that do produce output by default. diff --git a/internal/engine/descriptors.go b/internal/engine/descriptors.go new file mode 100644 index 000000000..d672874cc --- /dev/null +++ b/internal/engine/descriptors.go @@ -0,0 +1,263 @@ +package engine + +import "slices" + +type coreFactGatingClass string + +const ( + coreFactStandalone coreFactGatingClass = "standalone" + coreFactMultiOutput coreFactGatingClass = "multiOutput" + coreFactSharedProbe coreFactGatingClass = "sharedProbe" + coreFactInlineEager coreFactGatingClass = "inlineEager" +) + +type coreFactDescriptor struct { + root string + group string + groupOrder int + class coreFactGatingClass + assemble func(*coreFactBuild) []ResolvedFact + emittedRoots []string + probeConsumers []string + emitsUnder string +} + +type coreFactBuild struct { + s *Session + goos string + virtualization virtualization + virtualFact any + isVirtualFact any + dmi map[string]any +} + +var coreFactDescriptors = []coreFactDescriptor{ + { + root: "facterversion", + class: coreFactInlineEager, + assemble: func(*coreFactBuild) []ResolvedFact { return []ResolvedFact{{Name: "facterversion", Value: Version}} }, + emittedRoots: []string{"facterversion"}, + }, + { + root: "is_virtual", + class: coreFactInlineEager, + assemble: func(b *coreFactBuild) []ResolvedFact { + return []ResolvedFact{{Name: "is_virtual", Value: b.isVirtualFact}} + }, + emittedRoots: []string{"is_virtual"}, + }, + { + root: "path", + group: "path", + groupOrder: 5, + class: coreFactInlineEager, + assemble: func(b *coreFactBuild) []ResolvedFact { + return []ResolvedFact{{Name: "path", Value: currentPathEntries(b.goos, b.s.getenv)}} + }, + emittedRoots: []string{"path"}, + }, + { + root: "virtual", + class: coreFactInlineEager, + assemble: func(b *coreFactBuild) []ResolvedFact { + return []ResolvedFact{{Name: "virtual", Value: b.virtualFact}} + }, + emittedRoots: []string{"virtual"}, + probeConsumers: []string{"azure", "ec2", "gce", "hypervisors"}, + }, + { + root: "networking", + group: "networking", + groupOrder: 2, + class: coreFactStandalone, + assemble: func(b *coreFactBuild) []ResolvedFact { return networkingCoreFacts(b.s) }, + emittedRoots: []string{"networking"}, + }, + { + root: "processors", + group: "processor", + groupOrder: 6, + class: coreFactStandalone, + assemble: func(b *coreFactBuild) []ResolvedFact { return processorsCoreFacts(b.s) }, + emittedRoots: []string{"processors"}, + }, + { + root: "memory", + group: "memory", + groupOrder: 1, + class: coreFactStandalone, + assemble: func(b *coreFactBuild) []ResolvedFact { return memoryCoreFacts(b.s) }, + emittedRoots: []string{"memory"}, + }, + { + root: "os", + group: "operating system", + groupOrder: 3, + class: coreFactMultiOutput, + assemble: func(b *coreFactBuild) []ResolvedFact { return osCoreFacts(b.s) }, + emittedRoots: []string{"filesystems", "kernel", "os", "system_profiler"}, + }, + { + root: "dmi", + class: coreFactSharedProbe, + assemble: func(b *coreFactBuild) []ResolvedFact { return dmiCoreFacts(b.s) }, + emittedRoots: []string{"dmi"}, + probeConsumers: []string{"gce"}, + }, + { + root: "disks", + class: coreFactMultiOutput, + assemble: func(b *coreFactBuild) []ResolvedFact { return disksCoreFacts(b.s) }, + emittedRoots: []string{"disks", "mountpoints", "partitions", "zfs", "zpool"}, + }, + { + root: "ssh", + class: coreFactStandalone, + assemble: func(b *coreFactBuild) []ResolvedFact { return sshCoreFacts(b.s) }, + emittedRoots: []string{"ssh"}, + probeConsumers: []string{"identity"}, + }, + { + root: "identity", + class: coreFactSharedProbe, + assemble: func(b *coreFactBuild) []ResolvedFact { return identityCoreFacts(b.s) }, + emittedRoots: []string{"identity"}, + probeConsumers: []string{"ssh"}, + }, + { + root: "system_uptime", + class: coreFactMultiOutput, + assemble: func(b *coreFactBuild) []ResolvedFact { return uptimeCoreFacts(b.s) }, + emittedRoots: []string{"load_averages", "system_uptime"}, + }, + { + root: "selinux", + class: coreFactInlineEager, + assemble: func(b *coreFactBuild) []ResolvedFact { return selinuxCoreFacts(b.s) }, + emittedRoots: []string{"os"}, + emitsUnder: "os.selinux", + }, + { + root: "fips_enabled", + class: coreFactStandalone, + assemble: func(b *coreFactBuild) []ResolvedFact { return fipsCoreFacts(b.s) }, + emittedRoots: []string{"fips_enabled"}, + }, + { + root: "timezone", + class: coreFactStandalone, + assemble: func(b *coreFactBuild) []ResolvedFact { return timezoneCoreFacts(b.s) }, + emittedRoots: []string{"timezone"}, + }, + { + root: "augeas", + class: coreFactStandalone, + assemble: func(b *coreFactBuild) []ResolvedFact { return augeasCoreFacts(b.s) }, + emittedRoots: []string{"augeas"}, + }, + { + root: "xen", + class: coreFactStandalone, + assemble: func(b *coreFactBuild) []ResolvedFact { return xenCoreFacts(b.s) }, + emittedRoots: []string{"xen"}, + }, + { + root: "packages", + group: "packages", + groupOrder: 4, + class: coreFactStandalone, + assemble: func(b *coreFactBuild) []ResolvedFact { return packagesCoreFacts(b.s) }, + emittedRoots: []string{"packages"}, + }, + { + root: "hypervisors", + class: coreFactSharedProbe, + assemble: func(b *coreFactBuild) []ResolvedFact { return currentLinuxHypervisorFacts(b.s) }, + emittedRoots: []string{"hypervisors"}, + probeConsumers: []string{"virtual"}, + }, + { + root: "hypervisors", + class: coreFactSharedProbe, + assemble: func(b *coreFactBuild) []ResolvedFact { return currentWindowsHypervisorFacts(b.s) }, + emittedRoots: []string{"hypervisors"}, + probeConsumers: []string{"virtual"}, + }, + { + root: "az_metadata", + class: coreFactSharedProbe, + assemble: func(b *coreFactBuild) []ResolvedFact { + return azureFacts(b.s.Context(), newAzureClient(azureMetadataBaseURL, nil), b.virtualization) + }, + emittedRoots: []string{"az_metadata", "cloud"}, + probeConsumers: []string{"virtual"}, + }, + { + root: "ec2_metadata", + class: coreFactSharedProbe, + assemble: func(b *coreFactBuild) []ResolvedFact { + return ec2Facts(b.s, newEC2Client(ec2MetadataBaseURL, nil), b.virtualization) + }, + emittedRoots: []string{"cloud", "ec2_metadata", "ec2_userdata"}, + probeConsumers: []string{"virtual"}, + }, + { + root: "gce", + class: coreFactSharedProbe, + assemble: func(b *coreFactBuild) []ResolvedFact { + return platformGCEFacts(b.s.Context(), b.goos, b.virtualization, dmiBIOSVendor(b.dmi), newGCEClient(gceMetadataBaseURL, nil)) + }, + emittedRoots: []string{"cloud", "gce"}, + probeConsumers: []string{"dmi", "virtual"}, + }, +} + +func newCoreFactBuild(s *Session) *coreFactBuild { + goos := s.goos() + virtualization := detectVirtualization(s) + virtualFact, isVirtualFact := virtualizationFactValues(virtualization) + return &coreFactBuild{ + s: s, + goos: goos, + virtualization: virtualization, + virtualFact: virtualFact, + isVirtualFact: isVirtualFact, + dmi: s.cachedDMI(), + } +} + +func standaloneCoreFactRoots() []string { + roots := make([]string, 0) + for _, descriptor := range coreFactDescriptors { + if descriptor.class == coreFactStandalone { + roots = append(roots, descriptor.root) + } + } + return roots +} + +func builtinFactGroupsFromDescriptors() []FactGroup { + groups := make([]FactGroup, 0) + for _, descriptor := range coreFactDescriptors { + if descriptor.group == "" { + continue + } + groups = append(groups, FactGroup{ + Name: descriptor.group, + Facts: []string{descriptor.root}, + }) + } + slices.SortFunc(groups, func(a, b FactGroup) int { + return descriptorGroupOrder(a.Name) - descriptorGroupOrder(b.Name) + }) + return groups +} + +func descriptorGroupOrder(name string) int { + for _, descriptor := range coreFactDescriptors { + if descriptor.group == name { + return descriptor.groupOrder + } + } + return 0 +} diff --git a/internal/engine/descriptors_test.go b/internal/engine/descriptors_test.go new file mode 100644 index 000000000..1f1934410 --- /dev/null +++ b/internal/engine/descriptors_test.go @@ -0,0 +1,84 @@ +package engine + +import ( + "reflect" + "slices" + "strings" + "testing" +) + +func TestCoreFactDescriptors_projectBuiltinGroups(t *testing.T) { + want := []FactGroup{ + {Name: "memory", Facts: []string{"memory"}}, + {Name: "networking", Facts: []string{"networking"}}, + {Name: "operating system", Facts: []string{"os"}}, + {Name: "packages", Facts: []string{"packages"}}, + {Name: "path", Facts: []string{"path"}}, + {Name: "processor", Facts: []string{"processors"}}, + } + if got := BuiltinFactGroups(); !reflect.DeepEqual(got, want) { + t.Fatalf("BuiltinFactGroups() = %#v, want %#v", got, want) + } +} + +func TestBuildCoreFacts_inlineFactsKeepHeadOrder(t *testing.T) { + facts := buildCoreFacts(NewSession(), map[string]bool{"packages": true}) + if len(facts) < 4 { + t.Fatalf("buildCoreFacts() returned %d facts, want at least 4", len(facts)) + } + got := []string{facts[0].Name, facts[1].Name, facts[2].Name, facts[3].Name} + want := []string{"facterversion", "is_virtual", "path", "virtual"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("leading facts = %#v, want %#v", got, want) + } +} + +func TestCoreFactDescriptors_coverEmittedRoots(t *testing.T) { + allowed := map[string]bool{} + for _, descriptor := range coreFactDescriptors { + if descriptor.root == "" { + t.Fatalf("descriptor %#v has empty root", descriptor) + } + if len(descriptor.emittedRoots) == 0 { + t.Fatalf("descriptor %q has no emitted roots", descriptor.root) + } + if descriptor.emitsUnder == "" && !slices.Contains(descriptor.emittedRoots, descriptor.root) { + t.Fatalf("descriptor %q emitted roots = %#v, want root included", descriptor.root, descriptor.emittedRoots) + } + for _, root := range descriptor.emittedRoots { + allowed[root] = true + } + } + + for _, fact := range buildCoreFacts(NewSession(), map[string]bool{"packages": true}) { + root, _, _ := strings.Cut(fact.Name, ".") + if !allowed[root] { + t.Fatalf("buildCoreFacts emitted root %q from fact %q, missing from descriptor table", root, fact.Name) + } + } +} + +func TestCoreFactDescriptors_standaloneGatesAreTableDriven(t *testing.T) { + roots := standaloneCoreFactRoots() + if len(roots) == 0 { + t.Fatal("standaloneCoreFactRoots() = empty, want gated categories from descriptors") + } + for _, root := range roots { + descriptor, ok := coreFactDescriptorByRoot(root, coreFactStandalone) + if !ok { + t.Fatalf("standalone root %q missing descriptor", root) + } + if !slices.Contains(descriptor.emittedRoots, root) { + t.Fatalf("standalone descriptor %q emitted roots = %#v, want root included", root, descriptor.emittedRoots) + } + } +} + +func coreFactDescriptorByRoot(root string, class coreFactGatingClass) (coreFactDescriptor, bool) { + for _, descriptor := range coreFactDescriptors { + if descriptor.root == root && descriptor.class == class { + return descriptor, true + } + } + return coreFactDescriptor{}, false +} diff --git a/internal/engine/discovery_plan.go b/internal/engine/discovery_plan.go index 8e399b908..a83daa89b 100644 --- a/internal/engine/discovery_plan.go +++ b/internal/engine/discovery_plan.go @@ -2,7 +2,6 @@ package engine import ( "slices" - "strings" ) type discoveryPlan struct { @@ -40,9 +39,6 @@ func (e *Engine) planDiscovery(s *Session, queries []string) (discoveryPlan, []e if err != nil { failures = append(failures, err) } else if ok { - if len(plan.externalDirs) == 0 { - plan.externalDirs = slices.Clone(config.ExternalDirs) - } plan.noExternalFacts = plan.noExternalFacts || config.NoExternalFacts plan.cacheGroups = cloneFactGroups(config.FactGroups) if plan.useCache { @@ -62,12 +58,36 @@ func (e *Engine) planDiscovery(s *Session, queries []string) (discoveryPlan, []e if plan.disabledFacts == nil { plan.disabledFacts = map[string]bool{} } - if !plan.noExternalFacts && len(plan.externalDirs) == 0 && e.cfg.SystemDefaults { - plan.externalDirs = e.defaultExternalDirs() + var defaultExternalDirs []string + systemDefaults := e.cfg.SystemDefaults && !plan.noExternalFacts + if systemDefaults && len(plan.externalDirs) == 0 && len(config.ExternalDirs) == 0 { + defaultExternalDirs = e.defaultExternalDirs() } + configForDirs := config + configForDirs.NoExternalFacts = false + plan.externalDirs = DiscoveryExternalDirs(configForDirs, plan.externalDirs, false, systemDefaults, defaultExternalDirs) return plan, failures } +// DiscoveryExternalDirs returns the external fact directories discovery would +// load for the same inputs: explicit directories first, then config +// directories, then process defaults when system defaults are enabled. +func DiscoveryExternalDirs(config Config, explicit []string, noExternalFacts, systemDefaults bool, defaults []string) []string { + if noExternalFacts || config.NoExternalFacts { + return nil + } + if len(explicit) > 0 { + return slices.Clone(explicit) + } + if len(config.ExternalDirs) > 0 { + return slices.Clone(config.ExternalDirs) + } + if systemDefaults { + return slices.Clone(defaults) + } + return nil +} + // disabledSourceEnv and disabledSourceConfig label the ambient (non-CLI) // sources of a disabled fact for the explicit-query diagnostic. const ( @@ -111,7 +131,7 @@ func (e *Engine) unionDisabledFacts(s *Session, config Config, includeEnv bool) for name := range DisabledFactsWithGroups(e.cfg.ExtraDisabled, groups) { delete(ambient, name) for k := range ambient { - if strings.HasPrefix(k, name+".") { + if factHierarchyCovers(name, k) { delete(ambient, k) } } diff --git a/internal/engine/discovery_plan_test.go b/internal/engine/discovery_plan_test.go index fd393ba7a..f5347428c 100644 --- a/internal/engine/discovery_plan_test.go +++ b/internal/engine/discovery_plan_test.go @@ -114,3 +114,63 @@ func TestPlanDiscoveryDisabledFactsOverrideWinsOverUnion(t *testing.T) { t.Fatalf("disabledFacts = %#v, want override verbatim %#v (--no-block / explicit set wins)", plan.disabledFacts, want) } } + +func TestDiscoveryExternalDirsResolutionOrder(t *testing.T) { + tests := []struct { + name string + config Config + explicit []string + noExternal bool + systemDefaults bool + defaults []string + want []string + }{ + { + name: "explicit wins", + config: Config{ExternalDirs: []string{"/config"}}, + explicit: []string{"/cli"}, + systemDefaults: true, + defaults: []string{"/default"}, + want: []string{"/cli"}, + }, + { + name: "config fallback", + config: Config{ExternalDirs: []string{"/config"}}, + systemDefaults: true, + defaults: []string{"/default"}, + want: []string{"/config"}, + }, + { + name: "system defaults fallback", + systemDefaults: true, + defaults: []string{"/default"}, + want: []string{"/default"}, + }, + { + name: "explicit no external", + explicit: []string{"/cli"}, + noExternal: true, + }, + { + name: "config no external", + config: Config{ExternalDirs: []string{"/config"}, NoExternalFacts: true}, + explicit: []string{"/cli"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := DiscoveryExternalDirs(tt.config, tt.explicit, tt.noExternal, tt.systemDefaults, tt.defaults) + if !reflect.DeepEqual(got, tt.want) { + t.Fatalf("DiscoveryExternalDirs() = %#v, want %#v", got, tt.want) + } + if len(got) > 0 { + got[0] = "mutated" + again := DiscoveryExternalDirs(tt.config, tt.explicit, tt.noExternal, tt.systemDefaults, tt.defaults) + if reflect.DeepEqual(got, again) { + t.Fatalf("DiscoveryExternalDirs() returned aliased slice %#v", again) + } + } + }) + } +} diff --git a/internal/engine/engine.go b/internal/engine/engine.go index c51fc4ba6..9f775248f 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -134,20 +134,14 @@ func diagnoseAmbientDisabledQueries(logger *slog.Logger, queries []string, ambie // ambientDisableSource reports the ambient source that disables name, matching // the full dotted name and then every ancestor (parent, grandparent, … root) so -// the diagnostic covers descendants the way pruneDisabledDescendants / -// FilterDisabledFacts do: a config disable of `os.release` is reported for a -// query of `os.release.major`. +// a config disable of `os.release` is reported for a query of +// `os.release.major`. func ambientDisableSource(name string, ambient map[string]string) (string, bool) { - for { - if source, ok := ambient[name]; ok { - return source, true - } - cut := strings.LastIndex(name, ".") - if cut < 0 { - return "", false - } - name = name[:cut] - } + _, source, ok := factHierarchyMatch(name, func(candidate string) (string, bool) { + source, ok := ambient[candidate] + return source, ok + }) + return source, ok } // warnOnce emits message at warn level the first time it is seen on this @@ -256,7 +250,9 @@ func (e *Engine) Discover(ctx context.Context, queries ...string) (*Snapshot, er if plan.useCache && ctx.Err() == nil { cache := NewFactCache(DefaultCachePath(), plan.cacheTTLs, plan.cacheGroups, s.logger) - remaining, cached := cache.ResolveFacts(facts) + cacheFacts := FilterDisabledFacts(facts, plan.disabledFacts) + remaining, cached := cache.ResolveFacts(cacheFacts) + cached = FilterDisabledFacts(cached, plan.disabledFacts) if err := cache.CacheFacts(remaining); err != nil { failures = append(failures, err) } diff --git a/internal/engine/groups.go b/internal/engine/groups.go index 452b541dd..fe685d4bb 100644 --- a/internal/engine/groups.go +++ b/internal/engine/groups.go @@ -21,17 +21,7 @@ type FactGroup struct { // ADR-0007 and never resolve, so listing them was a no-op. Disabling a group // name still drops the whole subtree through the structured root. func BuiltinFactGroups() []FactGroup { - return []FactGroup{ - {Name: "memory", Facts: []string{"memory"}}, - {Name: "networking", Facts: []string{"networking"}}, - {Name: "operating system", Facts: []string{"os"}}, - // Facts-native group (ADR-0014): a deliberate divergence from Ruby - // Facter's group list, so a disable names package collection as one - // unit and --list-block-groups shows it. - {Name: "packages", Facts: []string{"packages"}}, - {Name: "path", Facts: []string{"path"}}, - {Name: "processor", Facts: []string{"processors"}}, - } + return cloneFactGroups(builtinFactGroupsFromDescriptors()) } // MergeFactGroups returns defaults with configured groups replacing same-name defaults. @@ -217,6 +207,24 @@ func DisabledUnion(config Config, extraDisabled []string, environ []string) map[ return DisabledFactsWithGroups(entries, config.FactGroups) } +func factHierarchyCovers(ancestor, name string) bool { + return ancestor == name || strings.HasPrefix(name, ancestor+".") +} + +func factHierarchyMatch[T any](name string, lookup func(string) (T, bool)) (string, T, bool) { + for { + if value, ok := lookup(name); ok { + return name, value, true + } + cut := strings.LastIndex(name, ".") + if cut < 0 { + var zero T + return "", zero, false + } + name = name[:cut] + } +} + // FilterDisabledFacts removes facts whose root name is disabled. func FilterDisabledFacts(facts []ResolvedFact, disabled map[string]bool) []ResolvedFact { if len(disabled) == 0 { @@ -237,7 +245,7 @@ func FilterDisabledFacts(facts []ResolvedFact, disabled map[string]bool) []Resol func pruneDisabledDescendants(name string, value any, disabled map[string]bool) any { var pruned any for disabledName := range disabled { - if !strings.HasPrefix(disabledName, name+".") { + if disabledName == name || !factHierarchyCovers(name, disabledName) { continue } if pruned == nil { diff --git a/openspec/changes/add-fact-disable-controls/design.md b/openspec/changes/archive/2026-07-06-add-fact-disable-controls/design.md similarity index 100% rename from openspec/changes/add-fact-disable-controls/design.md rename to openspec/changes/archive/2026-07-06-add-fact-disable-controls/design.md diff --git a/openspec/changes/add-fact-disable-controls/proposal.md b/openspec/changes/archive/2026-07-06-add-fact-disable-controls/proposal.md similarity index 100% rename from openspec/changes/add-fact-disable-controls/proposal.md rename to openspec/changes/archive/2026-07-06-add-fact-disable-controls/proposal.md diff --git a/openspec/changes/add-fact-disable-controls/specs/fact-disable-controls/spec.md b/openspec/changes/archive/2026-07-06-add-fact-disable-controls/specs/fact-disable-controls/spec.md similarity index 100% rename from openspec/changes/add-fact-disable-controls/specs/fact-disable-controls/spec.md rename to openspec/changes/archive/2026-07-06-add-fact-disable-controls/specs/fact-disable-controls/spec.md diff --git a/openspec/changes/add-fact-disable-controls/specs/facts-cli-option-contract/spec.md b/openspec/changes/archive/2026-07-06-add-fact-disable-controls/specs/facts-cli-option-contract/spec.md similarity index 100% rename from openspec/changes/add-fact-disable-controls/specs/facts-cli-option-contract/spec.md rename to openspec/changes/archive/2026-07-06-add-fact-disable-controls/specs/facts-cli-option-contract/spec.md diff --git a/openspec/changes/add-fact-disable-controls/specs/facts-native-input-surface/spec.md b/openspec/changes/archive/2026-07-06-add-fact-disable-controls/specs/facts-native-input-surface/spec.md similarity index 100% rename from openspec/changes/add-fact-disable-controls/specs/facts-native-input-surface/spec.md rename to openspec/changes/archive/2026-07-06-add-fact-disable-controls/specs/facts-native-input-surface/spec.md diff --git a/openspec/changes/add-fact-disable-controls/tasks.md b/openspec/changes/archive/2026-07-06-add-fact-disable-controls/tasks.md similarity index 85% rename from openspec/changes/add-fact-disable-controls/tasks.md rename to openspec/changes/archive/2026-07-06-add-fact-disable-controls/tasks.md index 82f1f2f66..6f624d71c 100644 --- a/openspec/changes/add-fact-disable-controls/tasks.md +++ b/openspec/changes/archive/2026-07-06-add-fact-disable-controls/tasks.md @@ -5,7 +5,7 @@ - [x] 1.3 Test resolution-gating: a standalone-resolver fact (`packages`) skips its resolver; a multi-output category runs and prunes when only some of its outputs are disabled; a disabled sub-fact is pruned. - [x] 1.4 Test `--no-block` clears the set; disable-beats-query returns empty; an env/config-sourced disable on an explicit query emits the stderr diagnostic with empty stdout. - [x] 1.5 Test `FACTS_DISABLE`, `FACTSDISABLE`, `FACTER_DISABLE`, and `FACTERDISABLE` are all reserved (no `disable` fact created) and feed the disabled set. -- [ ] 1.6 Test a disabled fact is never served from cache and a pruned sub-fact is not persisted into a cached group. (Behavior verified sound by review — disabled set is subtracted before cache resolution — but a dedicated regression test is still a follow-up.) +- [x] 1.6 Test a disabled fact is never served from cache and a pruned sub-fact is not persisted into a cached group. (Closed by deepen-cli-intake-and-gating-descriptors, commit cb8b35f1: `TestDiscover_disabledFactIsNotServedFromFreshCache` / `TestDiscover_prunedSubfactIsNotCached` — the pin exposed a real query-path cache resurrection, fixed by confining disabled subtraction to the cache interaction.) ## 2. Implementation diff --git a/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/.openspec.yaml b/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/.openspec.yaml new file mode 100644 index 000000000..dd9a1d92e --- /dev/null +++ b/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-07-06 diff --git a/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/design.md b/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/design.md new file mode 100644 index 000000000..7abcb64b6 --- /dev/null +++ b/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/design.md @@ -0,0 +1,79 @@ +# Design: deepen-cli-intake-and-gating-descriptors + +## Context + +Three verified locality findings from the 2026-07-06 architecture review, all behavior-preserving consolidations of shipped code: + +1. **Disable/gating knowledge is encoded six ways** with no source of truth: the `gate()` string literals in `buildCoreFacts` (core.go:78-104), the `BuiltinFactGroups` group→fact table (groups.go:23-35) whose spellings must agree with the gate literals ("processor"→"processors", "operating system"→"os") with nothing enforcing it, the `FilterDisabledFacts`/`pruneDisabledDescendants` hierarchy walks (groups.go:220-274), two more independent hierarchy walks in the ambient diagnostic (`unionDisabledFacts` descendant walk, discovery_plan.go:111-118; `ambientDisableSource` ancestor walk, engine.go:135-151), and the hard-coded gated-category list in core_gating_test.go:27-30. `add-fact-disable-controls` (ADR-0015, commit 129f7ad5) landed this machinery correctly but distributed. +2. **The option vocabulary is declared three ways**: the `optionDefinitions` registry (options.go:31-245, 27 canonical entries — validation/help/man), `runQuery`'s hand-built FlagSet (app.go:220-255, 28 declarations), and the partial hand walkers `configPathFromArgs`/`externalDirsFromArgs` (app.go:88-140) used by the `--list-*-groups` tasks. The query path has no drift today, but sync is implicit, and the list path silently ignores 20 of 22 options with no test locking either behavior. +3. **The app mirrors engine planning**: app.go:272-291 re-derives external-dir resolution (CLI → config → process defaults) line-for-line from `planDiscovery` (discovery_plan.go:43-67), solely to feed `canUseVersionQueryFastPath`; `factGroups` (app.go:61-86) re-derives config/external-dir planning a second time. + +Constraints: the CLI process edge is the only Ruby Facter compatibility boundary (ADR-0001/0009) — every stderr byte, conflict message (British "unrecognised"), exit status, and format shape is contract. The published `facts-cli-option-contract` spec requires the fast-path *decision* to stay CLI-owned. `man/man8/facts.8` is committed and hand-maintained; only option-name presence is tested. + +## Goals / Non-Goals + +**Goals:** + +- One gating descriptor table in `internal/engine` that gating, group expansion, filtering/pruning, and ambient diagnostics all read; one hierarchy-walk helper; agreement enforced by test. +- One option intake: FlagSet derived from the `cli` registry, one parsed-options value consumed by every task path, hand walkers deleted. +- The app consumes engine-resolved planning (external dirs) instead of mirroring the resolution order. +- Pre-refactor pins for every currently-untested behavior the refactor could silently change. +- Absorb the deferred `add-fact-disable-controls` task 1.6 cache regression test. + +**Non-Goals:** + +- Query-scoped core-fact resolution (`buildCoreFacts` stays query-agnostic). +- Folding the version fast path behind `Discover`, or deleting it. +- Changing list-task option semantics (only `--config`/`--external-dir` act; kept as-is, now pinned). +- Any output-contract or input-contract change; any new user-facing option. +- A man-page generator. + +## Decisions + +### D1: A gating descriptor table, with `BuiltinFactGroups` as a view over it + +One table (new `internal/engine` file, e.g. `descriptors.go`) with a row per core fact category: root name, optional Facter-compat group name, gating class (`standalone` / `multiOutput` / `sharedProbe` / `inlineEager`), assembly function, emitted roots, probe consumers, emits-under. `buildCoreFacts` iterates the table instead of inlining nine `gate()` calls plus eager appends; `BuiltinFactGroups()` becomes a projection of the table so its two other consumers (cache TTL bucketing at cache.go:49, `--list-*-groups` rendering) are untouched. `core_gating_test.go`'s hard-coded list migrates to assert against the table, and a new agreement test fails when gate names, group membership, or emitted roots drift. + +*Why not leave the literals and just add an agreement test?* The test would be a seventh copy of the same strings. The table is the only shape where agreement is by construction and ADR-0015's per-probe language is data, not comment prose. + +*Rejected:* deriving the table by reflection over resolver outputs at init — runtime cleverness for a compile-time fact; the table is small and explicit. + +### D2: One hierarchy helper + +The four walk implementations (root-cut filter, descendant prune, ambient descendant subsumption, ambient ancestor attribution) collapse onto one helper answering "is name X disabled by set S, and through which entry". Existing `disable_diagnostic_test.go` cases become the parity matrix; behavior byte-identical. + +### D3: FlagSet derived from the registry; binding lives in `internal/app` + +The `cli` registry stays pure metadata (its package doc promises "without replacing the existing parser"). `internal/app` gains a builder that iterates `cli` option metadata and binds each canonical option (and aliases) into one `parsedOptions` struct — bool/value/repeated binding chosen by the existing arity metadata. A parity test asserts every non-task registry entry has a binding and the FlagSet accepts nothing the registry doesn't name. `runQuery`'s 28 hand declarations are deleted. + +*Rejected:* moving parsing into `internal/cli` — it would swallow `flag` semantics the app owns (task dispatch, config interplay) and widen the `cli` interface for one consumer; the registry-drives-the-parser property is what the spec needs, not a package move. + +### D4: List tasks route through the same intake; semantics frozen + +`factGroups` receives the same `parsedOptions` (it reads only config path and external dirs from it); `configPathFromArgs`/`externalDirsFromArgs` are deleted. Current semantics — every other option inert on list tasks — are preserved and pinned by new tests *before* the walkers are removed. + +*Rejected:* honoring all options uniformly on list tasks — an observable behavior change (e.g. `--no-external-facts` would start dropping external groups from `--list-cache-groups`), out of scope for a behavior-preserving change; if wanted later it is its own proposal. + +### D5: The fast-path gate asks the engine for resolved planning + +The engine exports one pure planning seam (mirroring the existing `DisabledUnion` precedent) that answers "given CLI dirs, config, and no-external-facts, which external dirs would discovery load" — the same code path `planDiscovery` uses. app.go:272-291 is deleted; `canUseVersionQueryFastPath` consumes the seam's answer. The decision and formatter selection stay in `internal/app`, so the published requirement's ownership sentence stands; only its mechanics are amended (delta spec). + +*Rejected:* folding the fast path behind `Discover` — `buildCoreFacts` is not query-scoped; any query resolves every category including cloud-metadata HTTP (100ms–5s timeouts on cloud hosts), so the fold would need query-scoped resolution first, which is out of scope. *Rejected:* deleting the fast path — same latency reason; `facts facterversion` is the documented cheap path. + +### D6: Pins land first (tests-first ordering) + +Every behavior the refactor could silently change gets a pinning test before any production edit: list-path inertness and walker arity-skip, `--no-json`/`--no-yaml`/`--no-hocon` standalone acceptance, runtime-vs-validation error prefix asymmetry at the binary layer, `--color facterversion` bytes, and the task 1.6 cache regression (disabled fact never served from cache; pruned sub-fact never persisted into a cached group). + +## Risks / Trade-offs + +- [Ruby-parity bytes drift during intake consolidation] → all conflict messages, prefixes, and format shapes are already or newly byte-pinned; `app_test.go` (~78 cases), `disable_test.go`, `main_test.go` must pass unmodified. +- [Descriptor table over-skips a shared probe] → gating class `sharedProbe` keeps `dmi`/`identity` eager exactly as today; `TestBuildCoreFacts_resolutionGatingSkipsProbeWork` and the preserved-behavior spec scenario guard it. +- [`--no-block` regression] → the empty-non-nil `DisabledFacts` override (app.go:294-298 → discovery_plan.go:59) is exercised by existing tests; the table must not re-introduce defaults ahead of the override. +- [Cache ordering disturbed by table-driven build] → the build/gate → filter → select → cache order in `Discover` (engine.go:248-260) is untouched; the new task 1.6 regression test locks it. +- [List-path pin reveals a disagreement about intended semantics] → the pin records today's behavior; changing it is deliberately deferred to its own change. +- [Config parse errors must still precede the fast path] → sequencing in `runQuery` (parse config → gate → discover) is preserved and covered by existing config-error tests. +- [Archive-order coupling] → `add-fact-disable-controls` must archive before this change so the `fact-disable-controls` capability exists to receive the delta. + +## Open Questions + +- None blocking. The one judgment call — freeze vs. unify list-task option semantics — is decided (freeze, D4) and reversible by a future change. diff --git a/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/proposal.md b/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/proposal.md new file mode 100644 index 000000000..e50600cd5 --- /dev/null +++ b/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/proposal.md @@ -0,0 +1,33 @@ +# Deepen CLI intake and gating descriptors + +## Why + +Two vocabularies that must stay internally consistent are each declared several times with nothing enforcing agreement. Disable/gating semantics — which fact names gate which resolvers, how groups expand, how the name hierarchy is walked — are encoded six ways across `core.go`, `groups.go`, `discovery_plan.go`, `engine.go`, `app.go`, and a hard-coded test list; the CLI option vocabulary is declared three ways (the `cli` registry, `runQuery`'s hand-built FlagSet, and partial arg walkers for the list-groups tasks), kept in sync only implicitly. On top of both, `internal/app` re-derives the engine's discovery planning (external-dir resolution, defaults) line-for-line just to feed the version fast path and the list-groups tasks. Every future change to disabling, options, or planning currently pays an N-site synchronization tax with no failing test when a site is missed — the list path already silently ignores 20 of 22 options with zero coverage. + +## What Changes + +- **One gating descriptor table** (`internal/engine`): a single table describing each core-fact category — root fact name, Facter-compatible group name, gating class (standalone / multi-output / shared-probe / inline-eager), resolver, emitted roots, probe consumers, emits-under — becomes the source `buildCoreFacts` gating, `BuiltinFactGroups`, disabled-fact filtering/pruning, and the ambient-disable diagnostics all read. One hierarchy-walk helper replaces the four independent ancestor/descendant walk implementations. Behavior-preserving: resolved names, gating semantics (per-probe, not per-category), group spellings, and diagnostic bytes are unchanged. +- **Registry-driven option intake** (`internal/cli`, `internal/app`): the `runQuery` FlagSet is derived from the option registry instead of hand-redeclared; all task paths consume one parsed-options value; the hand-rolled `configPathFromArgs`/`externalDirsFromArgs` walkers are deleted. Current list-path semantics (only `--config`/`--external-dir` act on `--list-*-groups`) are kept and pinned by new tests before the refactor. +- **App consumes engine planning instead of mirroring it**: the external-dir/defaults resolution duplicated in `app.go` (feeding the version fast path) and in `factGroups` moves behind engine-owned seams the app queries. The fast-path *decision* stays CLI-owned, per the published `facts-cli-option-contract` requirement — its mechanics are amended, not its ownership. +- **Pre-refactor pins** (tests first): list-path option semantics, walker arity-skip behavior, `--no-json`/`--no-yaml`/`--no-hocon` standalone inertness, runtime-vs-validation error prefix asymmetry, `--color facterversion` bytes, and the deferred `add-fact-disable-controls` task 1.6 regression test (disabled facts never served from or persisted into cache). +- No output-contract or input-contract changes. No new user-facing surface. + +## Capabilities + +### New Capabilities + +_None — this change consolidates the implementation behind existing capabilities._ + +### Modified Capabilities + +- `facts-cli-option-contract`: "CLI option vocabulary is shared" is strengthened — the parser's flag set is derived from the shared registry (single declaration), and list-task option semantics are pinned. "Version fast path reuses engine-owned seams" is amended — the fast-path gate consumes engine-resolved planning inputs instead of re-deriving them; the decision stays in `internal/app`. +- `fact-disable-controls`: gating, group expansion, pruning, and disable diagnostics are required to derive from one descriptor table with agreement structurally enforced by test; the cache-ordering invariant gains its deferred regression test. (This capability's spec lands with `add-fact-disable-controls`, which is code-complete and unarchived — that change must archive before this one; this change builds on its landed code, commit `129f7ad5`.) + +## Impact + +- `internal/engine`: `core.go` (gate calls → table-driven), `groups.go` (`BuiltinFactGroups`, `FilterDisabledFacts`, `pruneDisabledDescendants`), `discovery_plan.go` (`unionDisabledFacts` ambient walk), `engine.go` (`ambientDisableSource`), `cache.go` (group-table consumer, unchanged semantics), new descriptor table file. +- `internal/cli`: `options.go` grows whatever metadata FlagSet derivation needs; `arguments.go`/`validation.go` unchanged in behavior. +- `internal/app`: `app.go` (`runQuery` FlagSet, `factGroups`, hand walkers deleted, fast-path gate rewired), `loghandler.go` untouched. +- `cmd/facts`: unchanged behavior; error-rendering seam byte-identical. +- Tests: new pins in `internal/app` and `internal/engine`; `core_gating_test.go`'s hard-coded category list migrates to assert against the table; existing ~78 `app_test.go` cases, `disable_test.go`, `disable_diagnostic_test.go`, `builtin_groups_test.go`, `main_test.go` must stay green unmodified. +- Dependency: `add-fact-disable-controls` archives first. Independent of `add-packages-fact` and the other open changes. diff --git a/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/specs/fact-disable-controls/spec.md b/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/specs/fact-disable-controls/spec.md new file mode 100644 index 000000000..00b2026a9 --- /dev/null +++ b/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/specs/fact-disable-controls/spec.md @@ -0,0 +1,22 @@ +## ADDED Requirements + +### Requirement: Disable semantics derive from one gating descriptor table + +The engine SHALL derive resolver gating, fact-group expansion, disabled-fact filtering and pruning, and ambient-disable diagnostics from a single gating descriptor table describing each core fact category — root fact name, Facter-compatible group name, gating class (standalone, multi-output, shared-probe, inline-eager), emitted roots, probe consumers, and emits-under root. The fact-name hierarchy SHALL be walked through one shared helper wherever ancestor or descendant relationships are evaluated. Agreement between gate names, group membership, and emitted roots MUST be structurally enforced by tests, not by convention. + +#### Scenario: Descriptor agreement is enforced + +- **WHEN** a category's gate name, group membership, or emitted roots disagree with the gating descriptor table +- **THEN** an engine test MUST fail rather than silently mis-gating or mis-expanding + +#### Scenario: Hierarchy walks agree + +- **WHEN** a disabled name is evaluated for post-resolution filtering, descendant pruning, ambient-source attribution, or CLI-disable subsumption +- **THEN** every path MUST reach the same ancestor and descendant conclusions through the shared hierarchy helper + +#### Scenario: Gating behavior is preserved + +- **WHEN** discovery runs with any disabled set +- **THEN** resolved fact names, resolution-gating decisions, group expansion, and diagnostic bytes MUST be identical to the pre-table behavior +- **AND** per-probe semantics hold: a disabled fact that shares a memoized probe with a kept fact skips only its own resolution while the shared probe still runs for the kept consumer +- **AND** disabling `selinux` by name remains a no-op because its facts emit under `os.selinux` diff --git a/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/specs/facts-cli-option-contract/spec.md b/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/specs/facts-cli-option-contract/spec.md new file mode 100644 index 000000000..5214e9fa2 --- /dev/null +++ b/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/specs/facts-cli-option-contract/spec.md @@ -0,0 +1,87 @@ +## MODIFIED Requirements + +### Requirement: CLI option vocabulary is shared + +The `facts` CLI SHALL use one supported option vocabulary for validation, option metadata, help output, man output, installed man page content, and runtime parsing. The runtime parser's flag set SHALL be derived from the shared option metadata rather than declared independently, so an option cannot be accepted by validation and unknown to the parser (or vice versa). + +#### Scenario: Accepted options are documented + +- **WHEN** an option is accepted by `facts` validation and runtime handling +- **THEN** the option MUST be listed in generated help/man documentation unless it is explicitly marked hidden +- **AND** `--force-dot-resolution` MUST be documented while it remains accepted + +#### Scenario: Unsupported options remain rejected + +- **WHEN** validation reads an option outside the shared supported option vocabulary +- **THEN** validation MUST reject it before runtime execution + +#### Scenario: Parser flag set derives from the shared vocabulary + +- **WHEN** a non-task option exists in the shared option metadata +- **THEN** the runtime parser MUST accept it (with its aliases, arity, and repeatability) without a second hand-written declaration +- **AND** a mismatch between the shared metadata and the parser's accepted flag set MUST fail tests + +### Requirement: Version fast path reuses engine-owned seams + +The CLI's version-query fast path SHALL derive its disabled-fact set from the engine's exported pure disabled-union function — the same union semantics the engine's discovery planning applies — and SHALL consume the engine's resolved external-fact-directory planning (CLI dirs, then config dirs, then process defaults) instead of re-deriving that resolution in `internal/app`. It SHALL render its output through the engine's formatter-selection seam (`BuildFormatter`) instead of a CLI-local re-derivation of format precedence. The fast-path decision itself and formatter selection remain owned by `internal/app` per the discovery-input-surface design. The engine SHALL NOT export helpers whose only purpose is to feed a CLI-side re-implementation of engine policy. + +#### Scenario: Fast-path disabled set matches discovery semantics + +- **WHEN** `facts facterversion` runs with any combination of `--disable`, the `FACTS_DISABLE` environment variable, and a config-file disable list +- **THEN** the fast path takes effect exactly when a full discovery would omit `facterversion` for the same inputs, because both derive the disabled set from the same engine union + +#### Scenario: Fast-path external-dir gate matches discovery planning + +- **WHEN** external-fact directories are supplied by `--external-dir`, by config, or by process defaults, with or without `--no-external-facts` +- **THEN** the fast path declines exactly when a full discovery would load external facts for the same inputs, because the gate consumes the engine's resolved planning rather than a CLI-side copy of the resolution order + +#### Scenario: Disabled facterversion falls through identically + +- **WHEN** `facterversion` is disabled by any disable source and queried in the default format +- **THEN** stdout, stderr diagnostics, and exit status are byte-identical to the behavior before the fast path consumed the engine union + +#### Scenario: Version output is format-stable + +- **WHEN** `facts facterversion` is rendered with `--json`, `--yaml`, `--hocon`, or the default format +- **THEN** the bytes written to stdout are identical to the previous hand-selected formatter output for each format + +#### Scenario: Color does not change fast-path bytes + +- **WHEN** `facts --color facterversion` runs +- **THEN** stdout bytes are identical to `facts facterversion` (the fast path renders the bare version scalar uncolored, as today) + +## ADDED Requirements + +### Requirement: Group-listing tasks parse options through the shared intake + +The `--list-block-groups` and `--list-cache-groups` tasks SHALL obtain option values through the shared registry-driven intake — the option vocabulary, aliases, and value arity come from the same option metadata the query task's parser derives from, with no independently-maintained option knowledge. The list-task intake preserves the historical permissive scan: it walks the entire argument tail, skipping positional tokens, `--`, and additional task flags, and honors `--config`/`--external-dir` wherever they appear. Option semantics are unchanged: only `--config` and `--external-dir` affect group listing; every other accepted option remains inert on these tasks. + +#### Scenario: Config and external dirs reach group listing via the shared intake + +- **WHEN** `facts --list-cache-groups -c PATH --external-dir DIR` runs (including `=`-attached and short-alias spellings, and with other valued options interleaved) +- **THEN** group listing MUST honor exactly the config file and external directories the shared intake parsed +- **AND** an interleaved valued option (such as `-l debug`) MUST NOT have its value misread as a different option or as a query + +#### Scenario: List tasks tolerate positionals, delimiters, and extra task flags + +- **WHEN** `--config` or `--external-dir` appears after a positional token, after `--`, or alongside an additional task flag (e.g. `facts --list-cache-groups bogus --config PATH`, `facts --list-cache-groups --version`) +- **THEN** group listing MUST still honor the option, ignore the stray tokens and extra task flags, and exit 0 — matching the historical permissive scan + +#### Scenario: Other options stay inert on list tasks + +- **WHEN** `facts --list-cache-groups --no-external-facts` (or any other accepted non-config, non-external-dir option) runs +- **THEN** the group listing output MUST be byte-identical to the same invocation without that option + +### Requirement: Option error rendering is stable at the process edge + +Validation-time option errors SHALL render on stderr with the `ERROR Facts::OptionsValidator - ` prefix and exit status 1. Runtime option-interplay errors (conflicts detectable only after config parsing, such as `--no-external-facts` combined with `--external-dir`, or a config-file log-level conflict) SHALL render as plain error lines without the OptionsValidator prefix, byte-identical to current behavior. + +#### Scenario: Validation errors carry the OptionsValidator prefix + +- **WHEN** the binary runs with an unknown or conflicting option rejected by validation (such as `-z`) +- **THEN** stderr MUST begin `ERROR Facts::OptionsValidator - ` and the process MUST exit 1 + +#### Scenario: Runtime option-interplay errors render plainly + +- **WHEN** the binary runs with an option combination rejected after config parsing (such as `--no-external-facts --external-dir DIR`) +- **THEN** the error line MUST render without the OptionsValidator prefix, byte-identical to today's output, and the process MUST exit 1 diff --git a/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/tasks.md b/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/tasks.md new file mode 100644 index 000000000..d7dea792a --- /dev/null +++ b/openspec/changes/archive/2026-07-06-deepen-cli-intake-and-gating-descriptors/tasks.md @@ -0,0 +1,41 @@ +## 1. Pre-refactor pins (tests first — all must pass against today's code) + +- [x] 1.1 Pin list-task option inertness: `--list-cache-groups`/`--list-block-groups` with `--no-external-facts`, `--no-cache`, `--disable`, `--no-block`, `--debug` produce byte-identical output to the bare invocation (`internal/app/app_test.go`). +- [x] 1.2 Pin walker arity-skip behavior: list tasks with interleaved valued options (e.g. `--list-cache-groups -l debug -c PATH --external-dir DIR`, `=`-attached and short-alias spellings) honor exactly the parsed config path and external dirs (`internal/app/app_test.go`). +- [x] 1.3 Pin `--no-json`/`--no-yaml`/`--no-hocon` standalone acceptance and inertness on a query (`internal/app/app_test.go`). +- [x] 1.4 Pin the two-tier option-error rendering at the binary layer: validation error → `ERROR Facts::OptionsValidator - ` + exit 1; runtime interplay error (`--no-external-facts --external-dir DIR`, config log-level conflict) → plain error line without the prefix + exit 1 (`cmd/facts/main_test.go`). +- [x] 1.5 Pin `facts --color facterversion` and `facts --force-dot-resolution facterversion` stdout bytes (fast path renders uncolored, un-merged) (`internal/app/app_test.go`). +- [x] 1.6 Add the deferred `add-fact-disable-controls` task 1.6 regression test: a disabled fact with a live cached value is not served from cache, and a pruned sub-fact is not persisted into a cached group (`internal/engine/cache_gating_test.go` or alongside `core_gating_test.go`). + +## 2. Gating descriptor table (internal/engine) + +- [x] 2.1 Add the descriptor table: one row per core fact category — root name, Facter-compat group name, gating class (`standalone`/`multiOutput`/`sharedProbe`/`inlineEager`), assembly func, emitted roots, probe consumers, emits-under (`internal/engine/descriptors.go`). +- [x] 2.2 Add the descriptor-agreement test: gate names, group membership, and emitted roots asserted against the table; a drifted literal fails (`internal/engine/descriptors_test.go`). +- [x] 2.3 Rewrite `buildCoreFacts` to iterate the table (gated standalone categories; eager multi-output, shared-probe, inline, emits-under classes exactly as today), removing the nine inline `gate()` calls (`core.go`). +- [x] 2.4 Re-derive `BuiltinFactGroups()` as a projection of the table; `cache.go` TTL bucketing and `--list-*-groups` output unchanged (`groups.go`). +- [x] 2.5 Add the shared hierarchy helper and collapse the four walk implementations onto it: `FilterDisabledFacts` root check, `pruneDisabledDescendants`, the ambient descendant subsumption (`discovery_plan.go:111-118`), and `ambientDisableSource` ancestor walk (`engine.go:135-151`). +- [x] 2.6 Migrate `core_gating_test.go`'s hard-coded gated-category list to read from the table; all existing gating, group, pruning, and diagnostic tests pass unmodified. + +## 3. Registry-driven option intake (internal/app + internal/cli) + +- [x] 3.1 Add the FlagSet builder: iterate `cli` option metadata, bind every non-task option (canonical + aliases, arity/repeatability from metadata) into one `parsedOptions` struct (`internal/app`). +- [x] 3.2 Add the registry↔parser parity test: every non-task registry entry has a binding; the FlagSet accepts nothing outside the registry (`internal/app`). +- [x] 3.3 Replace `runQuery`'s 28 hand declarations with the builder; all existing `app_test.go`, `disable_test.go`, `contract_test.go`, and `main_test.go` cases pass unmodified (`app.go`). +- [x] 3.4 Route `factGroups` through the same parsed intake (reading only config path and external dirs) and delete `configPathFromArgs`/`externalDirsFromArgs`; tasks 1.1–1.2 pins stay green (`app.go`). + +## 4. Engine-owned planning for the fast-path gate (internal/app + internal/engine) + +- [x] 4.1 Export the pure external-dir planning seam from the engine (the same resolution `planDiscovery` applies: CLI dirs → config dirs → process defaults, honoring no-external-facts), following the `DisabledUnion` precedent (`discovery_plan.go`). +- [x] 4.2 Rewire `canUseVersionQueryFastPath` inputs to the seam's answer and delete the app-side mirror (`app.go:272-291`); config-parse-error ordering, disabled-facterversion fall-through, external-dir shadowing, and `--timing` bypass all covered by existing tests plus task 1.5 pins. +- [x] 4.3 Use the same seam for `factGroups`' external-dir resolution, deleting its private `effectiveExternalDirs` merge duplication (`app.go:81-86`). + +## 5. Docs + +- [x] 5.1 Update CHANGELOG (internal consolidation; no user-facing behavior change). +- [x] 5.2 Record the frozen list-task option semantics decision where contributor docs describe the CLI tasks (CONTRIBUTING or package docs), so a future change to unify semantics starts from a recorded decision. + +## 6. Verification + +- [x] 6.1 Run `go test ./...` and `go vet ./...`; confirm the pre-existing test files pass unmodified (only additions permitted). +- [x] 6.2 Run the CLI option-contract tests (help/man drift) and the acceptance suite. +- [x] 6.3 Confirm `add-fact-disable-controls` is archived before this change archives; run `openspec validate deepen-cli-intake-and-gating-descriptors --strict`. (add-fact-disable-controls archived 2026-07-06; validate passed.) diff --git a/openspec/specs/fact-disable-controls/spec.md b/openspec/specs/fact-disable-controls/spec.md new file mode 100644 index 000000000..39d27ae2c --- /dev/null +++ b/openspec/specs/fact-disable-controls/spec.md @@ -0,0 +1,104 @@ +# fact-disable-controls Specification + +## Purpose +Define the all-on fact model with subtractive disabling: the disabled-set inputs (`--disable`, `FACTS_DISABLE`, config `disable`/`blocklist`), resolution-gating semantics, the `--no-block` override, and the cache interaction. +## Requirements +### Requirement: Facts are on by default and removed only by disabling + +Facts SHALL resolve every applicable fact by default and SHALL remove a fact only when it is named in the disabled set, with no opt-in, allowlist, or default-off tier. + +#### Scenario: Default discovery resolves everything + +- **WHEN** discovery runs with an empty disabled set +- **THEN** every applicable fact MUST resolve, including voluminous facts such as `packages` + +#### Scenario: A disabled fact is absent + +- **WHEN** a fact name or group name is in the disabled set +- **THEN** that fact, or the group's member facts, MUST be absent from the result + +### Requirement: The disabled set unions the CLI, environment, and config inputs + +Facts SHALL build the disabled set as the union of `--disable`, `FACTS_DISABLE`, and the `facts.conf` `disable` key (with the Facter `blocklist` key as its compatibility alias), accepting fact names and group names. + +#### Scenario: Three sources union + +- **WHEN** `--disable a`, `FACTS_DISABLE=b`, and config `disable: [c]` are all present +- **THEN** the disabled set MUST contain `a`, `b`, and `c` +- **AND** a group name MUST expand to its member facts + +#### Scenario: Native disable wins over the compat alias + +- **WHEN** both the `disable` key and the Facter `blocklist` key are present in config +- **THEN** the `disable` key MUST take precedence +- **AND** the `blocklist` key MUST still be honored when it is the only one present + +### Requirement: Disabling skips resolution for a dedicated resolver + +Facts SHALL skip a fact's resolution when every fact its resolver produces is disabled, and otherwise fall back to resolve-then-prune. + +#### Scenario: A standalone-resolver fact is resolution-gated + +- **WHEN** a fact produced by its own resolver (such as `packages`) is disabled +- **THEN** its resolver MUST NOT run and its collection work MUST be skipped + +#### Scenario: A shared resolver still runs for kept siblings + +- **WHEN** a disabled fact shares a resolver that also produces a fact not in the disabled set +- **THEN** the resolver MUST run and the disabled output MUST be pruned from the result + +#### Scenario: A disabled sub-fact is pruned + +- **WHEN** a disabled target names a sub-fact such as `os.release` +- **THEN** its parent resolver MUST run and the sub-fact MUST be pruned from the value + +### Requirement: --no-block clears the disabled set and disable beats query + +Facts SHALL treat `--no-block` as a master override that resolves everything, and SHALL let a disable override an explicit query while surfacing the cause. + +#### Scenario: --no-block resolves everything + +- **WHEN** `--no-block` is given alongside any disable inputs +- **THEN** the disabled set MUST be empty for that run and all facts MUST resolve + +#### Scenario: A disabled fact named in a query returns nothing + +- **WHEN** a fact is in the disabled set and is also named as a query +- **THEN** the result MUST be empty for that fact + +#### Scenario: An ambient disable is diagnosed + +- **WHEN** an explicitly queried fact is suppressed by a disable sourced from `FACTS_DISABLE` or config rather than the same command line +- **THEN** a one-line stderr diagnostic MUST name the fact and the disabling source +- **AND** stdout MUST stay empty + +### Requirement: Disabled facts are never served from cache + +Facts SHALL subtract the disabled set before the cache is consulted and before any cache write. + +#### Scenario: Cache does not serve a disabled fact + +- **WHEN** a fact is disabled and a cached value for it exists +- **THEN** discovery MUST NOT serve the cached value +- **AND** a pruned sub-fact MUST NOT be written into a cached group + +### Requirement: Disable semantics derive from one gating descriptor table + +The engine SHALL derive resolver gating, fact-group expansion, disabled-fact filtering and pruning, and ambient-disable diagnostics from a single gating descriptor table describing each core fact category — root fact name, Facter-compatible group name, gating class (standalone, multi-output, shared-probe, inline-eager), emitted roots, probe consumers, and emits-under root. The fact-name hierarchy SHALL be walked through one shared helper wherever ancestor or descendant relationships are evaluated. Agreement between gate names, group membership, and emitted roots MUST be structurally enforced by tests, not by convention. + +#### Scenario: Descriptor agreement is enforced + +- **WHEN** a category's gate name, group membership, or emitted roots disagree with the gating descriptor table +- **THEN** an engine test MUST fail rather than silently mis-gating or mis-expanding + +#### Scenario: Hierarchy walks agree + +- **WHEN** a disabled name is evaluated for post-resolution filtering, descendant pruning, ambient-source attribution, or CLI-disable subsumption +- **THEN** every path MUST reach the same ancestor and descendant conclusions through the shared hierarchy helper + +#### Scenario: Gating behavior is preserved + +- **WHEN** discovery runs with any disabled set +- **THEN** resolved fact names, resolution-gating decisions, group expansion, and diagnostic bytes MUST be identical to the pre-table behavior +- **AND** per-probe semantics hold: a disabled fact that shares a memoized probe with a kept fact skips only its own resolution while the shared probe still runs for the kept consumer +- **AND** disabling `selinux` by name remains a no-op because its facts emit under `os.selinux` diff --git a/openspec/specs/facts-cli-option-contract/spec.md b/openspec/specs/facts-cli-option-contract/spec.md index 4e81da17b..caf656056 100644 --- a/openspec/specs/facts-cli-option-contract/spec.md +++ b/openspec/specs/facts-cli-option-contract/spec.md @@ -5,7 +5,7 @@ Define the internal contract that keeps accepted `facts` CLI options, parser met ## Requirements ### Requirement: CLI option vocabulary is shared -The `facts` CLI SHALL use one supported option vocabulary for validation, option metadata, help output, man output, and installed man page content. +The `facts` CLI SHALL use one supported option vocabulary for validation, option metadata, help output, man output, installed man page content, and runtime parsing. The runtime parser's flag set SHALL be derived from the shared option metadata rather than declared independently, so an option cannot be accepted by validation and unknown to the parser (or vice versa). #### Scenario: Accepted options are documented @@ -18,6 +18,12 @@ The `facts` CLI SHALL use one supported option vocabulary for validation, option - **WHEN** validation reads an option outside the shared supported option vocabulary - **THEN** validation MUST reject it before runtime execution +#### Scenario: Parser flag set derives from the shared vocabulary + +- **WHEN** a non-task option exists in the shared option metadata +- **THEN** the runtime parser MUST accept it (with its aliases, arity, and repeatability) without a second hand-written declaration +- **AND** a mismatch between the shared metadata and the parser's accepted flag set MUST fail tests + ### Requirement: CLI option metadata preserves parser behavior The shared CLI option metadata SHALL describe canonical names, aliases, value arity, repeatability, task flags, and conflicts without replacing the existing parser. @@ -39,13 +45,18 @@ The shared CLI option metadata SHALL describe canonical names, aliases, value ar ### Requirement: Version fast path reuses engine-owned seams -The CLI's version-query fast path SHALL derive its disabled-fact set from the engine's exported pure disabled-union function — the same union semantics the engine's discovery planning applies — instead of re-implementing the union in `internal/app`, and SHALL render its output through the engine's formatter-selection seam (`BuildFormatter`) instead of a CLI-local re-derivation of format precedence. The fast-path decision itself and formatter selection remain owned by `internal/app` per the discovery-input-surface design. The engine SHALL NOT export helpers whose only purpose is to feed a CLI-side re-implementation of engine policy. +The CLI's version-query fast path SHALL derive its disabled-fact set from the engine's exported pure disabled-union function — the same union semantics the engine's discovery planning applies — and SHALL consume the engine's resolved external-fact-directory planning (CLI dirs, then config dirs, then process defaults) instead of re-deriving that resolution in `internal/app`. It SHALL render its output through the engine's formatter-selection seam (`BuildFormatter`) instead of a CLI-local re-derivation of format precedence. The fast-path decision itself and formatter selection remain owned by `internal/app` per the discovery-input-surface design. The engine SHALL NOT export helpers whose only purpose is to feed a CLI-side re-implementation of engine policy. #### Scenario: Fast-path disabled set matches discovery semantics - **WHEN** `facts facterversion` runs with any combination of `--disable`, the `FACTS_DISABLE` environment variable, and a config-file disable list - **THEN** the fast path takes effect exactly when a full discovery would omit `facterversion` for the same inputs, because both derive the disabled set from the same engine union +#### Scenario: Fast-path external-dir gate matches discovery planning + +- **WHEN** external-fact directories are supplied by `--external-dir`, by config, or by process defaults, with or without `--no-external-facts` +- **THEN** the fast path declines exactly when a full discovery would load external facts for the same inputs, because the gate consumes the engine's resolved planning rather than a CLI-side copy of the resolution order + #### Scenario: Disabled facterversion falls through identically - **WHEN** `facterversion` is disabled by any disable source and queried in the default format @@ -56,3 +67,56 @@ The CLI's version-query fast path SHALL derive its disabled-fact set from the en - **WHEN** `facts facterversion` is rendered with `--json`, `--yaml`, `--hocon`, or the default format - **THEN** the bytes written to stdout are identical to the previous hand-selected formatter output for each format +#### Scenario: Color does not change fast-path bytes + +- **WHEN** `facts --color facterversion` runs +- **THEN** stdout bytes are identical to `facts facterversion` (the fast path renders the bare version scalar uncolored, as today) + +### Requirement: The --disable option is part of the shared option vocabulary + +The `facts` CLI SHALL accept `--disable` as a valued, comma-separated, repeatable option in the shared option vocabulary, documented like any other non-hidden option, contributing fact and group names to the disabled set. + +#### Scenario: --disable is accepted and documented + +- **WHEN** `--disable packages,os` is parsed +- **THEN** validation MUST accept it as a valued option contributing `packages` and `os` to the disabled set +- **AND** `--disable` MUST appear in generated help and man output + +#### Scenario: --disable composes with --no-block + +- **WHEN** both `--disable packages` and `--no-block` are given +- **THEN** `--no-block` MUST clear the disabled set so nothing is disabled + +### Requirement: Group-listing tasks parse options through the shared intake + +The `--list-block-groups` and `--list-cache-groups` tasks SHALL obtain option values through the shared registry-driven intake — the option vocabulary, aliases, and value arity come from the same option metadata the query task's parser derives from, with no independently-maintained option knowledge. The list-task intake preserves the historical permissive scan: it walks the entire argument tail, skipping positional tokens, `--`, and additional task flags, and honors `--config`/`--external-dir` wherever they appear. Option semantics are unchanged: only `--config` and `--external-dir` affect group listing; every other accepted option remains inert on these tasks. + +#### Scenario: Config and external dirs reach group listing via the shared intake + +- **WHEN** `facts --list-cache-groups -c PATH --external-dir DIR` runs (including `=`-attached and short-alias spellings, and with other valued options interleaved) +- **THEN** group listing MUST honor exactly the config file and external directories the shared intake parsed +- **AND** an interleaved valued option (such as `-l debug`) MUST NOT have its value misread as a different option or as a query + +#### Scenario: List tasks tolerate positionals, delimiters, and extra task flags + +- **WHEN** `--config` or `--external-dir` appears after a positional token, after `--`, or alongside an additional task flag (e.g. `facts --list-cache-groups bogus --config PATH`, `facts --list-cache-groups --version`) +- **THEN** group listing MUST still honor the option, ignore the stray tokens and extra task flags, and exit 0 — matching the historical permissive scan + +#### Scenario: Other options stay inert on list tasks + +- **WHEN** `facts --list-cache-groups --no-external-facts` (or any other accepted non-config, non-external-dir option) runs +- **THEN** the group listing output MUST be byte-identical to the same invocation without that option + +### Requirement: Option error rendering is stable at the process edge + +Validation-time option errors SHALL render on stderr with the `ERROR Facts::OptionsValidator - ` prefix and exit status 1. Runtime option-interplay errors (conflicts detectable only after config parsing, such as `--no-external-facts` combined with `--external-dir`, or a config-file log-level conflict) SHALL render as plain error lines without the OptionsValidator prefix, byte-identical to current behavior. + +#### Scenario: Validation errors carry the OptionsValidator prefix + +- **WHEN** the binary runs with an unknown or conflicting option rejected by validation (such as `-z`) +- **THEN** stderr MUST begin `ERROR Facts::OptionsValidator - ` and the process MUST exit 1 + +#### Scenario: Runtime option-interplay errors render plainly + +- **WHEN** the binary runs with an option combination rejected after config parsing (such as `--no-external-facts --external-dir DIR`) +- **THEN** the error line MUST render without the OptionsValidator prefix, byte-identical to today's output, and the process MUST exit 1 diff --git a/openspec/specs/facts-native-input-surface/spec.md b/openspec/specs/facts-native-input-surface/spec.md index ec24edcd9..27c783e26 100644 --- a/openspec/specs/facts-native-input-surface/spec.md +++ b/openspec/specs/facts-native-input-surface/spec.md @@ -21,3 +21,29 @@ Facts SHALL accept its operator input surface under facts-native names — `fact #### Scenario: Compat surface is documented - **WHEN** an operator reads the configuration compatibility document - **THEN** it MUST state the native names, the facter-named compat reads, and the precedence between them + +### Requirement: disable is the native disabled-set key with blocklist compatibility + +Facts SHALL accept `disable` as the facts-native `facts.conf` key for the disabled set while continuing to read the Facter `blocklist` key as its compatibility alias, with the native key taking precedence. + +#### Scenario: Native disable key honored with blocklist compat + +- **WHEN** `facts.conf` sets `disable` and the facter-compatible config sets `blocklist` +- **THEN** discovery MUST honor `disable` +- **AND** discovery MUST still honor `blocklist` when it is the only key present + +### Requirement: FACTS_DISABLE is a reserved control key, not an environment fact + +Facts SHALL treat an environment variable whose resolved fact name is `disable` as the disabled-set control input rather than an external environment fact, across every accepted prefix. + +#### Scenario: FACTS_DISABLE controls disabling, not a fact + +- **WHEN** `FACTS_DISABLE=packages` is set +- **THEN** `packages` MUST be added to the disabled set +- **AND** no external fact named `disable` MUST be created + +#### Scenario: All prefix spellings are reserved + +- **WHEN** any of `FACTS_DISABLE`, `FACTSDISABLE`, `FACTER_DISABLE`, or `FACTERDISABLE` is set +- **THEN** it MUST be treated as the disable control +- **AND** it MUST NOT create a `disable` fact