From 4f97d12c1f10022d3a430503613ffdd2bba81566 Mon Sep 17 00:00:00 2001 From: Juliano Martinez Date: Tue, 23 Jun 2026 21:47:05 +0200 Subject: [PATCH] deepen discovery input surface --- cache_test.go | 29 +++++ engine_contract_test.go | 28 ++++- engine_test.go | 62 ++++++++++ internal/app/app.go | 50 ++++---- internal/app/app_test.go | 41 +++++++ internal/engine/discovery_plan.go | 109 ++++++++++++++++++ internal/engine/engine.go | 80 ++++++------- .../.openspec.yaml | 2 + .../deepen-discovery-input-surface/design.md | 94 +++++++++++++++ .../proposal.md | 30 +++++ .../specs/facts-cli-option-contract/spec.md | 23 ++++ .../specs/facts-library-api/spec.md | 23 ++++ .../specs/facts-native-input-surface/spec.md | 17 +++ .../deepen-discovery-input-surface/tasks.md | 42 +++++++ 14 files changed, 559 insertions(+), 71 deletions(-) create mode 100644 internal/engine/discovery_plan.go create mode 100644 openspec/changes/deepen-discovery-input-surface/.openspec.yaml create mode 100644 openspec/changes/deepen-discovery-input-surface/design.md create mode 100644 openspec/changes/deepen-discovery-input-surface/proposal.md create mode 100644 openspec/changes/deepen-discovery-input-surface/specs/facts-cli-option-contract/spec.md create mode 100644 openspec/changes/deepen-discovery-input-surface/specs/facts-library-api/spec.md create mode 100644 openspec/changes/deepen-discovery-input-surface/specs/facts-native-input-surface/spec.md create mode 100644 openspec/changes/deepen-discovery-input-surface/tasks.md diff --git a/cache_test.go b/cache_test.go index 81847d4e..8ffe0609 100644 --- a/cache_test.go +++ b/cache_test.go @@ -3,6 +3,7 @@ package facts import ( "context" "encoding/json" + "errors" "os" "path/filepath" "testing" @@ -111,6 +112,34 @@ func TestWithCache_servesFreshCachedValueOverResolver(t *testing.T) { } } +func TestWithCache_selectsQueriedFactsThroughEngineCachePath(t *testing.T) { + dir := redirectCacheDir(t) + conf := writeTTLConfig(t, "demo") + seedCacheFile(t, filepath.Join(dir, "demo"), map[string]any{"demo": map[string]any{"child": "from-cache"}}) + + eng, err := New( + WithCache(), + WithConfigFile(conf), + WithFact("demo", func(context.Context) (any, error) { + return map[string]any{"child": "fresh"}, nil + }), + WithFact("other", cachingResolver("fresh")), + ) + if err != nil { + t.Fatal(err) + } + snap, err := eng.Discover(context.Background(), "demo.child") + if err != nil { + t.Fatalf("Discover() err = %v", err) + } + if got, err := snap.Value("demo.child"); err != nil || got != "from-cache" { + t.Fatalf("Value(demo.child) = %#v, %v, want queried cached value", got, err) + } + if _, err := snap.Value("other"); !errors.Is(err, ErrFactNotFound) { + t.Fatalf("Value(other) err = %v, want unqueried fact omitted from cached Snapshot", err) + } +} + // TestWithoutCache_ignoresExistingCache is the toggle control: the same seeded // cache that WithCache would serve is ignored when WithCache is absent, proving // the option — not some always-on path — is what enables caching. diff --git a/engine_contract_test.go b/engine_contract_test.go index 1fa83747..a426f55c 100644 --- a/engine_contract_test.go +++ b/engine_contract_test.go @@ -130,6 +130,26 @@ func TestSnapshotTree_excludesLegacyAliasFacts(t *testing.T) { } } +func TestDiscoverQueriesSelectReturnedSnapshotFacts(t *testing.T) { + dir := t.TempDir() + writeTestFile(t, dir, "site.txt", "site_one=one\nsite_two=two\n") + + eng, err := New(WithExternalDirs(dir)) + if err != nil { + t.Fatal(err) + } + snap, err := eng.Discover(context.Background(), "site_one") + if err != nil { + t.Fatalf("Discover() err = %v", err) + } + if got, err := snap.Value("site_one"); err != nil || got != "one" { + t.Fatalf("Value(site_one) = %#v, %v, want selected fact", got, err) + } + if _, err := snap.Value("site_two"); !errors.Is(err, ErrFactNotFound) { + t.Fatalf("Value(site_two) err = %v, want unqueried fact omitted from selected Snapshot", err) + } +} + // Pins the surviving halves of TestAdd_resolvesProgrammaticCustomFactLazily, // TestValue_reusesResolvedProgrammaticCustomFact, and // TestValue_missingFactDoesNotResolveUnrelatedProgrammaticCustomFacts: each @@ -155,8 +175,8 @@ func TestWithFact_resolverRunsExactlyOncePerDiscover(t *testing.T) { t.Fatalf("resolver ran %d times after one Discover with an unrelated query, want eager resolution exactly once", calls) } for range 2 { - if got, err := first.Value("counted_fact"); err != nil || got != 1 { - t.Fatalf("Value(counted_fact) = %#v, %v, want first-run value 1", got, err) + if _, err := first.Value("counted_fact"); !errors.Is(err, ErrFactNotFound) { + t.Fatalf("Value(counted_fact) err = %v, want unrelated fact omitted from queried Snapshot", err) } } if calls != 1 { @@ -173,8 +193,8 @@ func TestWithFact_resolverRunsExactlyOncePerDiscover(t *testing.T) { if got, _ := second.Value("counted_fact"); got != 2 { t.Fatalf("second Value(counted_fact) = %#v, want fresh value 2", got) } - if got, _ := first.Value("counted_fact"); got != 1 { - t.Fatalf("first Value(counted_fact) = %#v after rediscovery, want immutable 1", got) + if _, err := first.Value("counted_fact"); !errors.Is(err, ErrFactNotFound) { + t.Fatalf("first Value(counted_fact) err = %v after rediscovery, want queried Snapshot unchanged", err) } } diff --git a/engine_test.go b/engine_test.go index d27351b2..f3e8394e 100644 --- a/engine_test.go +++ b/engine_test.go @@ -119,6 +119,68 @@ func TestWithConfigFile_loadsConfiguredDirs(t *testing.T) { } } +func TestWithConfigFile_recomputesDiscoveryPolicyEachDiscover(t *testing.T) { + cacheDir := redirectCacheDir(t) + firstDir := t.TempDir() + writeTestFile(t, firstDir, "site.txt", "site_location=first\n") + writeTestFile(t, firstDir, "blocked.txt", "blocked_probe=blocked\n") + secondDir := t.TempDir() + writeTestFile(t, secondDir, "site.txt", "site_location=second\nblocked_probe=visible\n") + configDir := t.TempDir() + configPath := filepath.Join(configDir, "facter.conf") + writeTestFile(t, configDir, "facter.conf", `global : { + external-dir : [ "`+firstDir+`" ], +} +facts : { + blocklist : [ "blocked.txt" ], +} +`) + + eng, err := New( + WithCache(), + WithConfigFile(configPath), + WithFact("cache_probe", func(context.Context) (any, error) { return "cached", nil }), + ) + if err != nil { + t.Fatal(err) + } + first, err := eng.Discover(context.Background()) + if err != nil { + t.Fatalf("first Discover() err = %v", err) + } + if got, err := first.Value("site_location"); err != nil || got != "first" { + t.Fatalf("first Value(site_location) = %#v, %v, want first", got, err) + } + if _, err := first.Value("blocked_probe"); !errors.Is(err, ErrFactNotFound) { + t.Fatalf("first Value(blocked_probe) err = %v, want ErrFactNotFound", err) + } + if _, err := os.Stat(filepath.Join(cacheDir, "cache_probe")); !os.IsNotExist(err) { + t.Fatalf("cache file after first Discover stat err = %v, want not exist", err) + } + + writeTestFile(t, configDir, "facter.conf", `global : { + external-dir : [ "`+secondDir+`" ], +} +facts : { + ttls : [ { "cache_probe" : "30 days" } ], +} +`) + second, err := eng.Discover(context.Background()) + if err != nil { + t.Fatalf("second Discover() err = %v", err) + } + if got, err := second.Value("site_location"); err != nil || got != "second" { + t.Fatalf("second Value(site_location) = %#v, %v, want second", got, err) + } + if got, err := second.Value("blocked_probe"); err != nil || got != "visible" { + t.Fatalf("second Value(blocked_probe) = %#v, %v, want visible", got, err) + } + data := readCacheFile(t, filepath.Join(cacheDir, "cache_probe")) + if data["cache_probe"] != "cached" { + t.Fatalf("cached cache_probe = %#v, want cached", data["cache_probe"]) + } +} + func TestWithConfigFile_blocklistSuppressesFacts(t *testing.T) { configPath := writeTestFile(t, t.TempDir(), "facter.conf", "facts : {\n blocklist : [ \"ssh\" ],\n}\n") diff --git a/internal/app/app.go b/internal/app/app.go index 540610a8..a3e453da 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -264,8 +264,10 @@ func runQuery(stdout, stderr io.Writer, args []string) error { if configErr != nil { return configErr } - if len(externalDirs) == 0 { - externalDirs = configOptions.ExternalDirs + cliExternalDirs := externalDirs + discoveryExternalDirs := externalDirs + if len(discoveryExternalDirs) == 0 { + discoveryExternalDirs = configOptions.ExternalDirs } if configOptions.NoExternalFacts { *noExternalFacts = true @@ -276,16 +278,21 @@ func runQuery(stdout, stderr io.Writer, args []string) error { if configOptions.Verbose { *verbose = true } - if *noExternalFacts && hasNonEmpty(externalDirs) { + if *noExternalFacts && hasNonEmpty(discoveryExternalDirs) { return optionError(stdout, errors.New("--no-external-facts and --external-dir options conflict: please specify only one")) } if !*noExternalFacts { - externalDirs = effectiveExternalDirs(externalDirs) + discoveryExternalDirs = effectiveExternalDirs(discoveryExternalDirs) + } + blockedFactsForFastPath := map[string]bool{} + var blockedFacts map[string]bool + if *noBlock { + blockedFacts = map[string]bool{} } - blockedFacts := map[string]bool{} if !*noBlock { - blockedFacts = engine.BlocklistedFactsForFiltering(configOptions.Blocklist, configOptions.FactGroups) + blockedFactsForFastPath = engine.BlocklistedFactsForFiltering(configOptions.Blocklist, configOptions.FactGroups) } + mergeDottedFacts := configOptions.ForceDotResolution || *forceDotResolution logLevel := firstNonEmpty(flags.Lookup("log-level").Value.String(), flags.Lookup("l").Value.String(), configOptions.LogLevel) if logLevel != "" && !cli.SupportedLogLevel(logLevel) { return optionError(stdout, fmt.Errorf("unsupported log level %s", logLevel)) @@ -304,17 +311,25 @@ 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(), externalDirs, blockedFacts, *noExternalFacts, *timing || *timingShort) { + if canUseVersionQueryFastPath(flags.Args(), discoveryExternalDirs, blockedFactsForFastPath, *noExternalFacts, *timing || *timingShort) { return writeVersionQuery(stdout, *jsonOutput || *jsonOutputShort, *yamlOutput || *yamlOutputShort, *hoconOutput) } resolutionStart := time.Now() eng, err := engine.NewEngine(engine.EngineConfig{ - CLICompat: true, - ExternalDirs: externalDirs, - NoExternalFacts: *noExternalFacts, - BlockedFacts: blockedFacts, - Logger: logger, + CLICompat: true, + SystemDefaults: true, + ConfigFile: configFile, + ConfigLoaded: true, + Config: configOptions, + ExternalDirs: cliExternalDirs, + UseCache: !*noCache, + NoExternalFacts: *noExternalFacts, + BlockedFacts: blockedFacts, + DefaultExternalDirsSet: true, + DefaultExternalDirs: defaultExternalFactDirs(), + IncludeTypedDotted: mergeDottedFacts, + Logger: logger, }) if err != nil { return err @@ -324,17 +339,6 @@ func runQuery(stdout, stderr io.Writer, args []string) error { return err } facts := snapshot.Facts() - mergeDottedFacts := configOptions.ForceDotResolution || *forceDotResolution - projection := engine.NewProjection(facts, mergeDottedFacts) - facts = projection.Select(flags.Args()) - if !*noCache { - cache := engine.NewFactCache(engine.DefaultCachePath(), configOptions.TTLs, configOptions.FactGroups, logger) - remaining, cached := cache.ResolveFacts(facts) - if err := cache.CacheFacts(remaining); err != nil { - return err - } - facts = append(remaining, cached...) - } resolutionDuration := time.Since(resolutionStart).Seconds() out, err := engine.BuildFormatter(engine.FormatOptions{ JSON: *jsonOutput || *jsonOutputShort, diff --git a/internal/app/app_test.go b/internal/app/app_test.go index f21b5d4a..f49d2b71 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -18,6 +18,18 @@ func TestMain(m *testing.M) { os.Exit(m.Run()) } +func seedAppCacheFile(t *testing.T, path string, data map[string]any) { + t.Helper() + data["cache_format_version"] = 1 + raw, err := json.Marshal(data) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, raw, 0o600); err != nil { + t.Fatal(err) + } +} + func TestRun_version(t *testing.T) { var stdout, stderr bytes.Buffer @@ -535,6 +547,35 @@ func TestRun_configTTLsUseFreshCachedFact(t *testing.T) { } } +func TestRun_noCacheIgnoresFreshCachedFact(t *testing.T) { + dir := t.TempDir() + factPath := filepath.Join(dir, "site.txt") + if err := os.WriteFile(factPath, []byte("site_role=fresh\n"), 0o600); err != nil { + t.Fatal(err) + } + configPath := filepath.Join(t.TempDir(), "facter.conf") + conf := "facts : {\n ttls : [ { \"site_role\" : \"1 hour\" } ],\n}\n" + if err := os.WriteFile(configPath, []byte(conf), 0o600); err != nil { + t.Fatal(err) + } + cacheDir := t.TempDir() + oldDefaultCachePath := engine.DefaultCachePath + engine.DefaultCachePath = func() string { return cacheDir } + t.Cleanup(func() { engine.DefaultCachePath = oldDefaultCachePath }) + seedAppCacheFile(t, filepath.Join(cacheDir, "site_role"), map[string]any{"site_role": "from-cache"}) + var stdout, stderr bytes.Buffer + + if err := Run(&stdout, &stderr, []string{"--no-cache", "--config", configPath, "--external-dir", dir, "site_role"}); err != nil { + t.Fatal(err) + } + if got, want := stdout.String(), "fresh\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_strictLogsMissingFactErrorWhenQueriedFactIsMissing(t *testing.T) { var stdout, stderr bytes.Buffer diff --git a/internal/engine/discovery_plan.go b/internal/engine/discovery_plan.go new file mode 100644 index 00000000..7ec72caa --- /dev/null +++ b/internal/engine/discovery_plan.go @@ -0,0 +1,109 @@ +package engine + +import "slices" + +type discoveryPlan struct { + externalDirs []string + noExternalFacts bool + blockedFacts map[string]bool + useCache bool + cacheTTLs []FactTTL + cacheGroups []FactGroup + loaderMode externalFactLoaderMode + includeEnv bool + queries []string + includeTypedDotted bool +} + +func (e *Engine) planDiscovery(s *Session, queries []string) (discoveryPlan, []error) { + plan := discoveryPlan{ + externalDirs: slices.Clone(e.cfg.ExternalDirs), + noExternalFacts: e.cfg.NoExternalFacts, + blockedFacts: cloneBoolMap(e.cfg.BlockedFacts), + useCache: e.cfg.UseCache, + loaderMode: externalFactLoaderLibrary, + includeEnv: e.cfg.SystemDefaults, + queries: slices.Clone(queries), + includeTypedDotted: e.cfg.IncludeTypedDotted, + } + if e.cfg.CLICompat { + plan.loaderMode = externalFactLoaderCLI + plan.includeEnv = true + } + + var failures []error + config, ok, err := e.configForDiscovery(s) + 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 e.cfg.BlockedFacts == nil { + plan.blockedFacts = BlocklistedFactsForFiltering(config.Blocklist, config.FactGroups) + } + if plan.useCache { + plan.cacheTTLs = slices.Clone(config.TTLs) + } + if e.cfg.CLICompat && config.ForceDotResolution { + plan.includeTypedDotted = true + } + } + if plan.blockedFacts == nil { + plan.blockedFacts = map[string]bool{} + } + if !plan.noExternalFacts && len(plan.externalDirs) == 0 && e.cfg.SystemDefaults { + plan.externalDirs = e.defaultExternalDirs() + } + return plan, failures +} + +func (e *Engine) configForDiscovery(s *Session) (Config, bool, error) { + if e.cfg.ConfigLoaded { + return cloneConfig(e.cfg.Config), true, nil + } + if e.cfg.ConfigFile == "" && !e.cfg.SystemDefaults { + return Config{}, false, nil + } + config, err := ParseConfig(e.cfg.ConfigFile, s.logger) + return config, true, err +} + +func (e *Engine) defaultExternalDirs() []string { + if e.cfg.DefaultExternalDirsSet { + return slices.Clone(e.cfg.DefaultExternalDirs) + } + return CurrentDefaultExternalFactDirs() +} + +func cloneBoolMap(in map[string]bool) map[string]bool { + if in == nil { + return nil + } + out := make(map[string]bool, len(in)) + for key, value := range in { + out[key] = value + } + return out +} + +func cloneConfig(in Config) Config { + in.Blocklist = slices.Clone(in.Blocklist) + in.ExternalDirs = slices.Clone(in.ExternalDirs) + in.TTLs = slices.Clone(in.TTLs) + in.FactGroups = cloneFactGroups(in.FactGroups) + return in +} + +func cloneFactGroups(in []FactGroup) []FactGroup { + if in == nil { + return nil + } + out := make([]FactGroup, len(in)) + for i, group := range in { + out[i] = FactGroup{Name: group.Name, Facts: slices.Clone(group.Facts)} + } + return out +} diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 35626808..547f32b8 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -57,6 +57,18 @@ type EngineConfig struct { NoExternalFacts bool // BlockedFacts overrides the config-derived blocklist when non-nil. BlockedFacts map[string]bool + // ConfigLoaded carries an already parsed config from internal/app. Library + // engines leave it false so ConfigFile/SystemDefaults are parsed fresh on + // every Discover. + ConfigLoaded bool + Config Config + // DefaultExternalDirs overrides process default external dirs for + // internal/app tests and CLI adapter wiring. Nil is a valid override when + // DefaultExternalDirsSet is true. + DefaultExternalDirsSet bool + DefaultExternalDirs []string + // IncludeTypedDotted enables CLI/config force-dot query projection. + IncludeTypedDotted bool } // Engine is an immutable unit of fact-discovery configuration. All @@ -75,6 +87,9 @@ type Engine struct { // NewEngine validates and freezes cfg into an Engine. func NewEngine(cfg EngineConfig) (*Engine, error) { cfg.ExternalDirs = slices.Clone(cfg.ExternalDirs) + cfg.BlockedFacts = cloneBoolMap(cfg.BlockedFacts) + cfg.DefaultExternalDirs = slices.Clone(cfg.DefaultExternalDirs) + cfg.Config = cloneConfig(cfg.Config) cfg.Facts = slices.Clone(cfg.Facts) for i, fact := range cfg.Facts { if fact.Name == "" { @@ -122,34 +137,8 @@ func (e *Engine) Discover(ctx context.Context, queries ...string) (*Snapshot, er var failures []error var facts []ResolvedFact - externalDirs := slices.Clone(e.cfg.ExternalDirs) - blocked := e.cfg.BlockedFacts - if blocked == nil { - blocked = map[string]bool{} - } - var ttls []FactTTL - var cacheGroups []FactGroup - noExternalFacts := e.cfg.NoExternalFacts - - if e.cfg.ConfigFile != "" || e.cfg.SystemDefaults { - configFile := e.cfg.ConfigFile - config, err := ParseConfig(configFile, s.logger) - if err != nil { - failures = append(failures, err) - } else { - if len(externalDirs) == 0 { - externalDirs = config.ExternalDirs - } - noExternalFacts = noExternalFacts || config.NoExternalFacts - cacheGroups = config.FactGroups - if e.cfg.BlockedFacts == nil { - blocked = BlocklistedFactsForFiltering(config.Blocklist, config.FactGroups) - } - if e.cfg.UseCache { - ttls = config.TTLs - } - } - } + plan, planFailures := e.planDiscovery(s, queries) + failures = append(failures, planFailures...) finish := func() (*Snapshot, error) { if err := ctx.Err(); err != nil { @@ -159,30 +148,26 @@ func (e *Engine) Discover(ctx context.Context, queries ...string) (*Snapshot, er } var externalFacts []ResolvedFact - if !noExternalFacts { - dirs := externalDirs - if e.cfg.SystemDefaults && !e.cfg.CLICompat && len(dirs) == 0 { - dirs = CurrentDefaultExternalFactDirs() - } - if len(dirs) > 0 && ExternalFactResolutionRunning() { + if !plan.noExternalFacts { + if len(plan.externalDirs) > 0 && ExternalFactResolutionRunning() { e.warnOnce("Recursion detected while resolving external facts; executable external facts will be skipped") } loader := externalFactLoader{ s: s, - dirs: dirs, - blocked: blocked, + dirs: plan.externalDirs, + blocked: plan.blockedFacts, } - if e.cfg.CLICompat { - loader.mode = externalFactLoaderCLI - loader.includeEnv = true + if plan.loaderMode == externalFactLoaderCLI { + loader.mode = plan.loaderMode + loader.includeEnv = plan.includeEnv loaded, err := loader.load() if err != nil { return newSnapshot(nil, s.logger), err } externalFacts = loaded } else { - loader.mode = externalFactLoaderLibrary - loader.includeEnv = e.cfg.SystemDefaults + loader.mode = plan.loaderMode + loader.includeEnv = plan.includeEnv loaded, err := loader.load() if err != nil { failures = append(failures, err) @@ -223,15 +208,22 @@ func (e *Engine) Discover(ctx context.Context, queries ...string) (*Snapshot, er facts = CoreFacts(s) facts = append(facts, registeredFacts...) facts = append(facts, externalFacts...) - facts = FilterBlockedFacts(facts, blocked) + facts = FilterBlockedFacts(facts, plan.blockedFacts) - if e.cfg.UseCache && ctx.Err() == nil { - cache := NewFactCache(DefaultCachePath(), ttls, cacheGroups, s.logger) + if len(plan.queries) > 0 { + facts = NewProjection(facts, plan.includeTypedDotted).Select(plan.queries) + } + + if plan.useCache && ctx.Err() == nil { + cache := NewFactCache(DefaultCachePath(), plan.cacheTTLs, plan.cacheGroups, s.logger) remaining, cached := cache.ResolveFacts(facts) if err := cache.CacheFacts(remaining); err != nil { failures = append(failures, err) } facts = append(remaining, cached...) + if len(plan.queries) > 0 { + facts = NewProjection(facts, plan.includeTypedDotted).Select(plan.queries) + } } return finish() diff --git a/openspec/changes/deepen-discovery-input-surface/.openspec.yaml b/openspec/changes/deepen-discovery-input-surface/.openspec.yaml new file mode 100644 index 00000000..a4ac4d78 --- /dev/null +++ b/openspec/changes/deepen-discovery-input-surface/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-06-23 diff --git a/openspec/changes/deepen-discovery-input-surface/design.md b/openspec/changes/deepen-discovery-input-surface/design.md new file mode 100644 index 00000000..2c08b8ce --- /dev/null +++ b/openspec/changes/deepen-discovery-input-surface/design.md @@ -0,0 +1,94 @@ +## Context + +Facts already has deep modules for projection and cache storage, but discovery input planning is still shallow. `internal/engine.Engine.Discover` resolves config, external source loading, registered facts, core facts, blocklists, and cache; `internal/app.runQuery` separately merges CLI flags with config, applies default external dirs, applies cache, then projects queries for formatting and strict mode. + +This change deepens the discovery input surface without changing public behavior. The public library remains `facts.Engine` plus immutable `Snapshot`. The CLI remains the compatibility boundary. `Projection` continues to own query semantics, and `FactCache` continues to own persistent cache storage. + +## Goals / Non-Goals + +**Goals:** + +- Put discovery-time source policy, blocklist policy, cache policy, and query selection point behind one package-internal module in `internal/engine`. +- Parse config on each `Discover`, preserving freshness for long-running engines. +- Keep `force-dot-resolution` as a projection/query-selection bit only. +- Make CLI cache/query/source behavior feed the same discovery plan as library system-following behavior. +- Preserve public `facts` interfaces, CLI flags, output, input source precedence, diagnostics, and cache behavior. + +**Non-Goals:** + +- No public `facts` option for `force-dot-resolution`. +- No formatter rewrite beyond removing discovery/query/cache planning from `internal/app`. +- No change to `Projection` internals, canonical tree semantics, or `FactCache` storage behavior. +- No fact schema, fact name, or supported release target change. + +## Decisions + +**1. Add an internal discovery planner in `internal/engine`.** + +The planner should be package-internal and concrete. It should derive a per-discovery plan from `EngineConfig`, the current context/session logger, and parsed config. + +The plan should own: + +- effective external fact dirs +- `no-external-facts` +- blocklisted facts +- cache enabled/disabled policy +- cache TTLs and fact groups +- external loader mode and env-fact inclusion +- query list and include-typed-dotted projection mode + +Rejected alternative: keep helper functions in `internal/app` and only shorten `Engine.Discover`. That would preserve the duplicated source/cache/query rules and fail the locality goal. + +**2. Parse config at discovery time.** + +`Engine` remains immutable configuration. Config files, default external dirs, environment facts, executable facts, and cache files remain discovery-time inputs. The planner must parse config on each `Discover` when `ConfigFile`, `SystemDefaults`, or CLI config handling requires it. + +Rejected alternative: parse config in `NewEngine`. That would make long-running engines stale and contradict freshness-by-rediscovery. + +**3. Move cache policy, not cache storage.** + +The planner decides whether cache applies and which TTLs/groups apply. `FactCache` remains the implementation that resolves and writes cache entries. + +For CLI behavior, the `--no-cache` flag and config-derived TTL/groups must reach the planner. Timing output still belongs in `internal/app`; the CLI can format timing from the selected facts returned by discovery. + +Rejected alternative: absorb `FactCache` into the planner. `FactCache` already has useful depth around file storage, freshness, groups, and diagnostics; merging it would make the new module too broad. + +**4. Move query selection point into discovery, keep projection semantics in `Projection`.** + +`Discover(ctx, queries...)` already accepts queries. The planner should make that parameter meaningful by applying `Projection.Select` at the end of discovery, before the Snapshot/CLI consumes selected facts. `Projection` still owns reverse precedence, dotted fallback, wildcard matching, missing-query detection, and selected-query value extraction. + +Rejected alternative: leave query projection in `internal/app`. That keeps CLI and library discovery paths divergent and makes cache placement depend on CLI code. + +**5. Treat `force-dot-resolution` as projection policy only.** + +The flag/config value exists so operator-supplied dotted external or registered facts can be interpreted as structured paths for output/query projection. It must not affect source loading, fact precedence, or the canonical Snapshot tree. The planner may carry it as an internal include-typed-dotted bit for query selection and selected CLI output. + +Rejected alternative: make `force-dot-resolution` a public library option. That exposes a CLI compatibility escape hatch as a library interface without a demonstrated consumer. + +**6. Keep the CLI as an adapter.** + +`internal/app` should continue to own process-edge behavior: flag parsing, help/man/version tasks, stdout/stderr, diagnostic rendering, timing lines, formatter selection, strict exit behavior, and fast-path version query handling. It should stop owning discovery source/cache/query planning. + +Rejected alternative: move all CLI state into `internal/engine`. That would make the engine own process-edge output behavior and blur the compatibility boundary. + +## Risks / Trade-offs + +- **Cache ordering drift** -> Add tests that compare cached queried output, full output, TTL group behavior, and `--no-cache` behavior before and after the planner move. +- **Canonical Snapshot tree accidentally changes under force-dot resolution** -> Keep force-dot resolution out of `newSnapshot` canonical collection and pin format/query-only behavior in CLI and projection tests. +- **Config diagnostics change timing or destination** -> Keep config parsing under the engine logger for discovery, and retain CLI log handler behavior for process-edge diagnostics. +- **Strict query behavior changes** -> Keep strict missing-query checks routed through `Projection.MissingQueries` and cover nil external/registered facts separately. +- **Planner grows into a grab bag** -> Keep renderer, diagnostics formatting, and cache storage outside the planner. + +## Migration Plan + +1. Add planner tests for effective external dirs, config-derived blocklists, cache TTL/groups, no-external-facts, loader mode/env inclusion, and include-typed-dotted projection policy. +2. Extract existing `Engine.Discover` planning code into the internal planner without changing behavior. +3. Move cache policy into the planner while keeping `FactCache` calls in the discovery path. +4. Move query selection into the discovery path using `Projection.Select`. +5. Change `internal/app` to pass CLI/config cache, external source, blocklist, and force-dot values into engine configuration instead of applying discovery planning itself. +6. Keep formatter selection, timing output, and strict error handling in `internal/app`, fed by the selected facts from discovery/projection. +7. Run targeted library, cache, query/projection, config/external, and CLI contract tests before the full Go test/vet pass. + +## Open Questions + +(none) diff --git a/openspec/changes/deepen-discovery-input-surface/proposal.md b/openspec/changes/deepen-discovery-input-surface/proposal.md new file mode 100644 index 00000000..3bbaddd6 --- /dev/null +++ b/openspec/changes/deepen-discovery-input-surface/proposal.md @@ -0,0 +1,30 @@ +## Why + +Discovery input behavior is split between `internal/engine.Engine.Discover` and `internal/app.runQuery`: config selection, external fact source planning, blocklists, cache policy, query selection, and `--force-dot-resolution` are assembled in more than one place. This keeps the output and input contracts correct by duplication instead of locality. + +## What Changes + +- Add one package-internal discovery planning module in `internal/engine` that resolves discovery-time input policy from `EngineConfig` and parsed config on each `Discover`. +- Move cache policy ownership into the planner: it decides when persistent cache applies and which TTLs/groups are used, while `FactCache` remains the cache storage implementation. +- Move the query-selection point into the engine discovery path so `Discover(ctx, queries...)` is handled where facts are discovered, while `Projection` keeps owning query semantics. +- Keep `--force-dot-resolution` as an internal projection/query-selection bit only; it must not affect source loading, fact precedence, or the canonical Snapshot tree. +- Make `internal/app` translate CLI flags/config into engine configuration and keep stdout/stderr, timing output, strict error handling, and formatter selection at the CLI adapter. +- Preserve public `facts` APIs, CLI flags, input contract, output contract, and cache behavior. No breaking change is intended. + +## Capabilities + +### New Capabilities + +(none) + +### Modified Capabilities + +- `facts-library-api`: Discovery must use one internal discovery plan for source policy, cache policy, and query selection while preserving `facts.Engine`, `Snapshot`, and `Discover(ctx, queries...)` behavior. +- `facts-native-input-surface`: Facts-native and facter-compatible input discovery must be planned once per discovery and shared by CLI-equivalent and library system-following engines. +- `facts-cli-option-contract`: CLI flags and config values for external facts, cache, and force-dot query projection must feed the shared discovery plan instead of duplicating discovery rules in `internal/app`. + +## Impact + +- **Code**: `internal/engine/engine.go`, new package-internal discovery planning code, `internal/app/app.go`, and focused tests in root library contracts, `internal/app`, cache, query/projection, and config/external fact paths. +- **Behavior**: Behavior-preserving refactor. Any observed public API, CLI output/status, input source precedence, cache TTL/group behavior, or `--force-dot-resolution` difference is a bug unless explicitly captured in a follow-up change. +- **Docs/schema**: No fact schema or changelog update expected because no user-visible behavior change is intended. diff --git a/openspec/changes/deepen-discovery-input-surface/specs/facts-cli-option-contract/spec.md b/openspec/changes/deepen-discovery-input-surface/specs/facts-cli-option-contract/spec.md new file mode 100644 index 00000000..a68648b6 --- /dev/null +++ b/openspec/changes/deepen-discovery-input-surface/specs/facts-cli-option-contract/spec.md @@ -0,0 +1,23 @@ +## ADDED Requirements + +### Requirement: CLI discovery options feed the shared plan +The `facts` CLI SHALL translate runtime flags and config values that affect discovery inputs, cache policy, and dotted query projection into engine discovery configuration instead of independently applying those discovery rules after `Discover`. + +#### Scenario: External source options are planned once +- **WHEN** the CLI runs with `--external-dir`, `--no-external-facts`, `--config`, or config-derived external dirs/blocklists +- **THEN** `internal/app` passes the resolved discovery policy into the engine path +- **AND** it does not duplicate source precedence, default-dir, or blocklist application outside the shared plan + +#### Scenario: Cache options are planned once +- **WHEN** the CLI runs with cache enabled, `--no-cache`, or config-derived cache TTL/groups +- **THEN** cache enablement and TTL/group policy are applied through the shared discovery plan +- **AND** persistent cache storage remains handled by the engine cache implementation + +#### Scenario: Force-dot resolution is projection-only +- **WHEN** the CLI runs with `--force-dot-resolution` or config `force-dot-resolution: true` +- **THEN** the value affects selected-query and formatter projection for dotted external or registered facts +- **AND** it does not affect source loading, source precedence, or the canonical Snapshot tree + +#### Scenario: CLI process-edge behavior stays in app +- **WHEN** discovery planning moves into the engine path +- **THEN** `internal/app` still owns stdout/stderr, help/man/version tasks, formatter selection, timing output, diagnostic rendering, strict exit behavior, and supported option validation diff --git a/openspec/changes/deepen-discovery-input-surface/specs/facts-library-api/spec.md b/openspec/changes/deepen-discovery-input-surface/specs/facts-library-api/spec.md new file mode 100644 index 00000000..ce522a68 --- /dev/null +++ b/openspec/changes/deepen-discovery-input-surface/specs/facts-library-api/spec.md @@ -0,0 +1,23 @@ +## ADDED Requirements + +### Requirement: Discovery uses one input plan per run +The library SHALL derive source loading, blocklist, cache, and query-selection policy from one internal discovery plan for each `Discover` call. The plan MUST be recomputed per discovery so config files, external fact directories, environment facts, executable facts, and cache contents remain fresh across repeated discovery on the same immutable Engine. + +#### Scenario: Config is read at discovery time +- **WHEN** an Engine configured with `WithConfigFile` discovers facts, the config file changes, and the same Engine discovers facts again +- **THEN** the second Snapshot reflects the updated config-derived external dirs, blocklists, and cache TTL/group policy + +#### Scenario: Query selection happens in discovery +- **WHEN** a consumer calls `Discover(ctx, "os.family")` +- **THEN** the returned Snapshot is backed by facts selected with the same projection semantics used by CLI query projection where the contracts overlap +- **AND** the public `Discover(ctx, queries...)` method shape remains unchanged + +#### Scenario: Cache policy stays discovery-scoped +- **WHEN** an Engine is configured with cache enabled and config-derived TTL/group policy +- **THEN** discovery applies cache resolution and cache refresh according to the per-discovery plan +- **AND** the Engine does not memoize resolved fact values between discoveries + +#### Scenario: Force-dot resolution is not public library configuration +- **WHEN** a library consumer constructs an Engine through public `facts` options +- **THEN** no public option exists for force-dot resolution +- **AND** the canonical Snapshot tree preserves existing dotted external and registered fact behavior diff --git a/openspec/changes/deepen-discovery-input-surface/specs/facts-native-input-surface/spec.md b/openspec/changes/deepen-discovery-input-surface/specs/facts-native-input-surface/spec.md new file mode 100644 index 00000000..e392631e --- /dev/null +++ b/openspec/changes/deepen-discovery-input-surface/specs/facts-native-input-surface/spec.md @@ -0,0 +1,17 @@ +## ADDED Requirements + +### Requirement: Input source planning is shared +Facts SHALL plan facts-native and facter-compatible input sources through one discovery-time module for CLI-equivalent and library system-following discovery. The plan MUST preserve native-name precedence, configured external dirs, default external dirs, environment fact precedence, `no-external-facts`, and blocklist semantics. + +#### Scenario: Library system defaults use the shared input plan +- **WHEN** an Engine constructed with `WithSystemDefaults` discovers facts on a host with native and facter-compatible config/input sources +- **THEN** discovery applies the same native-before-compatible input precedence as the `facts` CLI + +#### Scenario: Explicit library dirs remain explicit +- **WHEN** an Engine constructed with `WithExternalDirs` discovers facts +- **THEN** the shared input plan loads exactly the supplied external dirs and does not add default external dirs or environment facts + +#### Scenario: No external facts suppresses all external inputs +- **WHEN** discovery policy has `no-external-facts` enabled from CLI or config +- **THEN** the input plan omits external fact directories and external environment facts +- **AND** core and registered facts can still resolve diff --git a/openspec/changes/deepen-discovery-input-surface/tasks.md b/openspec/changes/deepen-discovery-input-surface/tasks.md new file mode 100644 index 00000000..4ebe7b53 --- /dev/null +++ b/openspec/changes/deepen-discovery-input-surface/tasks.md @@ -0,0 +1,42 @@ +## 1. Contract Tests + +- [x] 1.1 Add library tests proving config-derived external dirs, blocklists, and cache TTL/group policy are recomputed on each `Discover`. +- [x] 1.2 Add library tests proving `Discover(ctx, queries...)` returns selected facts through the same projection semantics as current CLI query projection. +- [x] 1.3 Add cache tests covering full output, queried output, TTL/group policy, and disabled cache behavior through the engine discovery path. +- [x] 1.4 Add CLI contract tests proving `--external-dir`, `--no-external-facts`, `--config`, blocklists, `--no-cache`, cache TTL/groups, and `--force-dot-resolution` still produce identical output/status behavior. + +## 2. Discovery Planner + +- [x] 2.1 Add a package-internal discovery planner in `internal/engine` that derives effective external dirs, no-external-facts, blocklisted facts, cache policy, cache TTLs/groups, loader mode/env inclusion, queries, and include-typed-dotted projection policy. +- [x] 2.2 Move config-file parsing and default input-source resolution from `Engine.Discover` into the planner while preserving per-discovery freshness. +- [x] 2.3 Move external fact loader mode/env inclusion and recursion warning decisions behind the planner. +- [x] 2.4 Keep `EngineConfig` immutable by cloning slices/maps needed by the planner and avoiding resolved fact state on `Engine`. + +## 3. Cache Policy + +- [x] 3.1 Move persistent cache enablement, TTLs, and fact groups into the discovery plan. +- [x] 3.2 Apply `FactCache.ResolveFacts` and `FactCache.CacheFacts` from the engine discovery path according to the plan. +- [x] 3.3 Preserve cache diagnostics through the engine logger and CLI diagnostic handler. +- [x] 3.4 Remove cache policy application from `internal/app` once engine discovery owns it. + +## 4. Query Projection + +- [x] 4.1 Apply `Projection.Select` in the engine discovery path when queries are supplied. +- [x] 4.2 Carry `force-dot-resolution` only as an internal include-typed-dotted projection bit. +- [x] 4.3 Preserve the canonical Snapshot tree by keeping force-dot behavior out of discovery-time canonical collection. +- [x] 4.4 Keep strict missing-query detection routed through `Projection.MissingQueries`. + +## 5. CLI Wiring + +- [x] 5.1 Change `internal/app` to translate CLI/config discovery inputs into `engine.EngineConfig` instead of duplicating source, blocklist, cache, and query planning. +- [x] 5.2 Keep CLI-owned behavior in `internal/app`: help/man/version tasks, stdout/stderr, formatter selection, timing output, diagnostic rendering, strict exit behavior, and option validation. +- [x] 5.3 Preserve the version-query fast path behavior before full discovery. +- [x] 5.4 Delete obsolete `internal/app` helpers for config path, external dirs, cache, or projection only after no callers remain. + +## 6. Verification + +- [x] 6.1 Run focused tests for library discovery, cache, config/external facts, projection/query, and CLI contracts. +- [x] 6.2 Run `gofmt -w` on edited Go files. +- [x] 6.3 Run `go test ./...`. +- [x] 6.4 Run `go vet ./...`. +- [x] 6.5 Run `openspec validate deepen-discovery-input-surface --strict`.