diff --git a/.github/workflows/unit_tests.yaml b/.github/workflows/unit_tests.yaml index a407073e..9cea72e1 100644 --- a/.github/workflows/unit_tests.yaml +++ b/.github/workflows/unit_tests.yaml @@ -109,7 +109,7 @@ jobs: export PATH=/usr/local/go/bin:$PATH export GOFLAGS=-buildvcs=false go version - go test . ./internal/engine ./internal/app ./tests/acceptance + go test -timeout 25m . ./internal/engine ./internal/app ./tests/acceptance go build -o /tmp/facts ./cmd/facts sh tools/freebsd-release-gate.sh /tmp/facts @@ -140,7 +140,7 @@ jobs: export PATH=/usr/local/go/bin:$PATH export GOFLAGS=-buildvcs=false go version - go test . ./internal/engine ./internal/app ./tests/acceptance + go test -timeout 25m . ./internal/engine ./internal/app ./tests/acceptance go build -o /tmp/facts ./cmd/facts sh tools/openbsd-release-gate.sh /tmp/facts @@ -172,7 +172,7 @@ jobs: export PATH=/usr/local/go/bin:/usr/bin:/bin:/usr/sbin:/sbin export GOFLAGS=-buildvcs=false go version - go test . ./internal/engine ./internal/app ./tests/acceptance + go test -timeout 25m . ./internal/engine ./internal/app ./tests/acceptance go build -o /tmp/facts ./cmd/facts sh tools/netbsd-release-gate.sh /tmp/facts @@ -204,7 +204,7 @@ jobs: export PATH=/usr/local/go/bin:$PATH export GOFLAGS=-buildvcs=false go version - go test . ./internal/engine ./internal/app ./tests/acceptance + go test -timeout 25m . ./internal/engine ./internal/app ./tests/acceptance go build -o /tmp/facts ./cmd/facts sh tools/dragonfly-release-gate.sh /tmp/facts @@ -241,6 +241,6 @@ jobs: export PATH=/usr/local/go/bin:$PATH export GOFLAGS=-buildvcs=false go version - go test . ./internal/engine ./internal/app ./tests/acceptance + go test -timeout 25m . ./internal/engine ./internal/app ./tests/acceptance go build -o /tmp/facts ./cmd/facts sh tools/illumos-release-gate.sh /tmp/facts diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bef8fb7..2e22a168 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,15 @@ ### Added +- Fact disabling is now a first-class, facts-native control. Disable any fact + or fact group with `--disable a,b`, the `FACTS_DISABLE=a,b` environment + variable, or the `facts.conf` `disable` key; the Facter `blocklist` key keeps + working as its compatibility alias, and `--no-block` clears every disable + source. Disabling is resolution-gated — a disabled standalone-resolver fact + (`networking`, `processors`, `memory`, `ssh`, `timezone`, `fips`, `augeas`, + `xen`) is not resolved at all, not merely filtered from output; multi-output + categories resolve then prune. An explicitly queried fact disabled by the + environment or config emits a one-line stderr diagnostic. - Release artifacts and cross-compile CI now include `arm` and `arm64` for FreeBSD, OpenBSD, and NetBSD. - Added native-gated Plan 9 (`plan9/amd64`) fact support for canonical diff --git a/CONTEXT.md b/CONTEXT.md index cd4a4d6d..3b96a434 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -37,6 +37,10 @@ A flat, pre-structured fact name (e.g. `operatingsystem`) from Ruby Facter's dep A fact whose preconditions don't hold on this host (e.g. EC2 metadata off-cloud). It is simply absent from the canonical tree — never an error. Only facts that were supposed to resolve and didn't count as failures. _Avoid_: failed fact, missing fact (that means "no such fact name") +**Disabled fact**: +A fact removed from discovery by the disabled set — the union of the CLI `--disable`, the `FACTS_DISABLE` environment variable, and the `facts.conf` `disable` key (the Facter `blocklist` key is accepted as its compatibility alias). Every fact is enabled by default; there is no opt-in. A disabled fact backed by its own resolver is not resolved at all (resolution-gating, e.g. `packages`); a fact that shares a multi-output resolver, or a disabled sub-fact, is pruned after resolution. `--no-block` re-enables everything for a run. +_Avoid_: blocked fact (use only for the Facter-compatible `blocklist` config spelling), hidden fact (disabling a standalone-resolver fact skips resolution, not just display) + **Supported fact**: A fact documented in the schema as part of Facts' supported output contract for one or more supported release targets. _Avoid_: available fact (too host-specific), implemented fact (too code-centric) @@ -103,6 +107,20 @@ _Avoid_: fact hash, output map A decode of part of the canonical tree into a caller-supplied Go type, failing loudly on shape mismatch. A view never resolves facts independently of the canonical tree. _Avoid_: typed fact, fact struct +### Packages + +**Package**: +A unit of installed software tracked by a package source (a `dpkg` entry, an `rpm` header, a macOS installer receipt, a `nix` profile entry). The `packages` fact reports packages, not arbitrary installable artifacts; an installed `.app` bundle is reported too, but as its own `apps` source, never folded into database packages. +_Avoid_: application, artifact, software (too broad — an unpackaged binary is not a package) + +**Package source**: +An authoritative inventory of installed software on the host — usually an installation database (the rpm database, the dpkg database, the FreeBSD pkg database, macOS installer receipts, the nix store), or a structured filesystem inventory where the OS keeps no database (macOS `.app` bundles → `apps`). A host has several coexisting sources, so the `packages` fact namespaces by source — `packages.` — and never merges records across sources. Frontends that write to the same database are the same source (apt/aptitude → `dpkg`; dnf/yum/zypper → `rpm`), so the source key is the database, never the frontend or the file format. +_Avoid_: package manager (ambiguous — frontend vs database), package format (`deb`/`rpm` name the artifact, not the source) + +**Package record**: +One installed package's entry in a source's list — a map identified by its fields (`name` and `version` always; plus per-source identity fields such as `architecture`, `type`/`tap`, `store_path`, `bundle_id`/`path`, or the Windows uninstall subkey/`ProductCode`), never by position and never by a unique map key. `version` is the package manager's verbatim native string. The same software appearing in two sources (a macOS `.pkg` receipt and its `.app` bundle) is two records, not one. +_Avoid_: package entry, package map (records live in a list, not a name-keyed map) + ## Example dialogue > **Dev**: A consumer's monitoring agent wants `networking.ip` but with their own registered fact overriding it. Is there a global registry they add it to? diff --git a/README.md b/README.md index 250e7b39..06d330ee 100644 --- a/README.md +++ b/README.md @@ -92,6 +92,8 @@ Errors are honest: missing facts are `ErrFactNotFound`, a registered fact that l The shipped binary is `facts`, and it keeps Ruby Facter's output contract — formatting, exit statuses, stderr diagnostics (with the program token rebranded to `Facts`), `facter.conf` semantics. Existing facter inputs keep working as the compat tier: `facter.conf` default paths, `FACTER_*` environment facts, and the puppetlabs fact directories are all still read, with the facts-native names winning when both are present. +Every fact is on by default. Disable any fact or fact group with `--disable`, the `FACTS_DISABLE` environment variable, or the `facts.conf` `disable` key (the Facter `blocklist` key stays as its compatibility alias). Disabling is resolution-gated — a disabled standalone resolver is skipped, not just hidden — and `--no-block` clears every disable. + ```console $ brew install ncode/tap/facts @@ -107,6 +109,9 @@ $ facts --json os.family kernel.version.full $ facts --external-dir ./facts.d site_role web + +$ facts --disable networking os.name # disable facts or groups; also FACTS_DISABLE, facts.conf 'disable' +Darwin ``` ```sh diff --git a/docs/adr/0014-packages-fact-source-namespaced-record-lists.md b/docs/adr/0014-packages-fact-source-namespaced-record-lists.md new file mode 100644 index 00000000..49498b72 --- /dev/null +++ b/docs/adr/0014-packages-fact-source-namespaced-record-lists.md @@ -0,0 +1,22 @@ +# The packages fact namespaces installed software by source, each a list of records + +Facts adds a `packages` fact reporting installed software — a Facts-native extension (Ruby Facter ships no installed-packages fact, so there is no upstream shape to match; ADR-0011 canonical spelling governs the names). It is namespaced by *source*, where a source is an authoritative installed-software inventory on the host — usually an installation database, or a structured filesystem inventory where the OS keeps no database (macOS `.app` bundles). The source key is the database or inventory, never an apt-style frontend and never an artifact format: apt/aptitude/dpkg → `dpkg` (not `apt`, not `deb`), dnf/yum/zypper → `rpm`. The v1 source set is `dpkg` (Debian/Ubuntu), `rpm` (RHEL family; SUSE), `pacman` (Arch), `apk` (Alpine), the cross-distro Linux sources `snap` (`/var/lib/snapd`) and `flatpak` (`/var/lib/flatpak`) wherever present, `pkg` (FreeBSD and DragonFly — the same pkgng SQLite database), `openbsd_pkg` (OpenBSD), `pkgsrc` (NetBSD; an illumos/DragonFly secondary), `ips` (illumos/Solaris — keyed `ips`, *not* `pkg`: IPS's `pkg(1)` CLI only coincidentally shares the FreeBSD tool name, and keying on the tool would clobber two different databases), `nix` (the installed profile set — `/nix/var/nix/profiles/default` and the NixOS system profile — *not* the whole `/nix/store`, which holds every build dependency and GC-rooted version and would massively over-report), macOS `receipts` (the installer(8)/PackageKit `.pkg` database — the dpkg/rpm analogue for MDM-managed servers), macOS `apps` (the `.app` bundle inventory), Windows `registry` (the Add/Remove-Programs uninstall hive), and Windows `appx` (Store/MSIX apps, which the uninstall hive omits). An optional `homebrew` source is auto-detected where a Cellar prefix exists (absent on clean servers); MacPorts and Chocolatey are deferred and would join the same auto-detected-secondary tier. Plan 9 emits nothing — it has no package database, so the source is absent, not empty. + +Each `packages.` is a list of package records, each a map of `{name, version, …}` in which name is a field, not a map key. The obvious shape — a name-keyed map, `packages.dpkg.bash = "5.1"` — is unsound, because a package name is not a unique identifier on most sources: dpkg multiarch (`libc6:amd64` and `libc6:i386` coexist), rpm multilib and install-only multiversion kernels (several `kernel-core` with identical name *and* arch), `openbsd_pkg` parallel installs (`autoconf-2.69p3` and `2.71p3`), homebrew formula-vs-cask (`docker` is both), the Windows uninstall hive (x86 and x64 builds share a DisplayName *and* DisplayVersion), and Nix (multiple versions of one name coexist by design). A name-keyed map silently drops the colliding sibling. Independently, package names contain dots (`python3.11`, `libdb5.3`, `libstdc++6`), which collide with the engine's dot-path addressing if a name is a path segment. A list keeps every record, is uniform across all sources, is dot-safe, and makes "is X installed?" a filter that can honestly return a set. + +Identity inside the list is completed by per-source fields: `architecture` (dpkg, rpm, apk), `type` (formula|cask) and `tap` (homebrew, so two taps' same-name/version formulae stay distinct), `store_path` (nix), `branch` and `architecture` (flatpak, whose app-id alone collides across branches and installations — `snap` by contrast keeps one active revision per name and needs no extra key), `bundle_id` and `path` (the macOS `apps` inventory, since two `.app` of the same name and version can sit at different paths), and for Windows `registry` the uninstall subkey / MSI `ProductCode` plus an `architecture` distinguishing the native hive from `WOW6432Node` — without which a 32-bit and 64-bit build sharing DisplayName and DisplayVersion would be byte-identical records. `version` is the package manager's verbatim native string; where a manager renders no epoch by default the reader asks for it explicitly (the rpm reader queries `%{EPOCH}:%{VERSION}-%{RELEASE}`, because a bare `rpm -qa` omits the epoch that can separate otherwise-identical install-only kernels), and `version` is not decomposed into sub-fields in v1. A per-source install timestamp is omitted in v1; if added it is spelled `install_date` (the `_date` suffix, per the existing `dmi.bios.release_date`), and only where the source records a true install time. + +The scope is system package databases only. Language and dev managers (pip, npm, gem, cargo) are out: they are per-user/per-project, not host facts, and double-list distro-shipped libraries; the source-namespaced shape lets them slot in later as `packages.pip` with no restructuring. System-scope `snap` and `flatpak` are root-owned host-global databases that meet the scope bar and are included in v1: omitting them would make `packages` wrong on the flagship Linux targets, where stock Ubuntu 22.04+ ships Firefox and Chromium only as snaps (no dpkg entry) and Fedora Workstation ships Flatpaks. Flatpak also keeps per-user installations, which fall under the execution-context boundary below. Records are never merged or deduped across sources: a macOS `.pkg` receipt and its `.app` bundle are two records under two keys, not one. + +The fact reports system-global databases plus whatever the collector's own execution context can read; it does not drop privileges or walk every user's home to enumerate other users' per-user installs. That boundary leaves several sources context-bounded rather than truly host-complete — homebrew (the owning user's Cellar), nix per-user profiles, per-user flatpak installations, the Windows per-user `HKCU` uninstall entries, and Windows `appx` (per-user-registered: the reader takes the collector context plus the system-provisioned set via `Get-AppxProvisionedPackage`, so other users' private Store installs are under-reported). The under-report is documented, and a `user` field is the additive path to all-users enumeration later. + +Collection honors one cheap read per source — parse the on-disk database directly where it is world-readable (`/var/lib/dpkg/status`, `/var/lib/pacman/local/*/desc`, the pkgng `local.sqlite`, `/var/db/receipts/*.plist`, the nix profile manifest), or one batch query (`rpm -qa --qf …`, `dpkg-query -W`, `pkg query`, `Get-AppxProvisionedPackage`) — never a process per package, never the network. The readers land as an ADR-0010 per-category module (`packages.go`) with non-GOOS-suffixed, cross-platform-testable parse functions (the on-disk-parse vs batch-query split is the impure-probe + pure-parse seam); the concrete file layout, the schema entries under ADR-0011 canonical spelling, and per-target validation land via a dedicated OpenSpec change that this ADR does not pre-empt — it fixes only the externally observable shape. + +`packages` is on by default, like every other fact (ADR-0015): the fact model is all-on with subtractive disabling, so a host that does not want package collection on every discovery disables it — `--disable packages`, `FACTS_DISABLE=packages`, or the `facts.conf` `disable` key. `packages` registers as a fact group so a disable names it as one unit, and because the disabled set is resolution-gated, disabling skips package collection entirely rather than merely hiding output. Adding a Facts-native `packages` group diverges the Facter-mirrored `--list-block-groups`/`--list-cache-groups` output from Ruby Facter — a deliberate parity divergence, since `packages` is Facts-native. + +## Considered Options + +- **Name-keyed map (`packages.dpkg.bash = "5.1"`)** — rejected: lossy on six sources (drops multiarch siblings, install-only kernels, the cask, the colliding tap, all-but-one Nix version), and package names contain dots that break dot-path addressing. +- **Name-keyed map of lists (`packages.dpkg.libc6 = [ … ]`)** — rejected: keeps the dotted-name hazard and forces a list value on the ~99% single-instance case for the benefit of a key lookup that is unsound anyway. +- **Qualified-key map (`packages.rpm."" = { … }`)** — rejected: heterogeneous key spelling per source, and NEVRA/store-path keys are themselves full of dots, so dot-path queries still mis-split. +- **List of records (chosen)** — uniform across every source, collision-proof once per-source identity fields are carried, and dot-safe; "is X installed?" becomes a filter, which is correct because the honest answer can be a set (a multilib pair, three kernels) rather than a single value. diff --git a/docs/adr/0015-facts-on-by-default-disable-is-opt-out-and-skips-resolution.md b/docs/adr/0015-facts-on-by-default-disable-is-opt-out-and-skips-resolution.md new file mode 100644 index 00000000..17120f56 --- /dev/null +++ b/docs/adr/0015-facts-on-by-default-disable-is-opt-out-and-skips-resolution.md @@ -0,0 +1,26 @@ +# Facts are on by default; disabling is opt-out, env- and flag-controllable, and skips resolution + +Status: proposed. This records the decided model; the `--disable`/`FACTS_DISABLE` surface, the resolver gating, and the reserved control key are implemented by a dedicated OpenSpec change. + +Facts today has only an opt-out blocklist that filters already-resolved facts out of output, configured by the Facter-compatible `blocklist` key. This change makes disabling a first-class facts-native control and gives it teeth. Every fact is on by default — the voluminous `packages` fact (ADR-0014) included — and a fact is removed only by disabling it. The disabled set is the union of three inputs: the CLI `--disable a,b,c` (comma-separated), the `FACTS_DISABLE=a,b,c` environment variable, and the facts-native `facts.conf` `disable` key. `disable` is the native term across all three surfaces; the Facter `blocklist` key keeps working as its compatibility alias — `facter.conf` is part of the binding input contract, so `blocklist` is superseded in name by `disable`, never removed, with native `disable` winning on collision. A disable target is a fact name or a fact group, reusing the existing group expansion (`BlocklistedFactsWithGroups`), so `--disable networking` drops the whole group. There is no opt-in, allowlist, or default-off tier; the model is purely all-on with subtractive disabling. + +Disabling aims to skip work, not just output — but the gating is per-resolver and per-probe, not a blanket per-name skip, because the resolver-to-top-level-fact map is many-to-one. Three classes follow: + +- A top-level fact produced by its own standalone resolver is **resolution-gated**: disabling it skips that resolver. `packages` is this case — `packagesCoreFacts` produces only `packages`, so `--disable packages` skips package collection entirely. (`networking`, `processors`, `memory`, `timezone`, `ssh`, `fips`, `augeas` are likewise single-output. `selinux` is *not* in this set: it emits as `os.selinux.*` descendants, so a name-level `--disable selinux` is a no-op — it stays eager and is disabled via `os.selinux`.) +- A top-level fact that shares a multi-output category resolver — `osCoreFacts` → `os`/`kernel`/`filesystems`, `disksCoreFacts` → `disks`/`partitions`/`mountpoints`/`zfs`/`zpool`, `uptimeCoreFacts` → `load_averages`/`system_uptime` — is gated only when **every** top-level fact that resolver produces is disabled; otherwise the resolver runs and the disabled outputs are pruned. +- A fact built inline in `buildCoreFacts` with no resolver seam (`facterversion`, `is_virtual`/`virtual`, `path`) is always resolve-then-prune. + +Gating is therefore defined per **probe**, not per category: a shared memoized probe — `cachedIdentity`, which feeds both the `identity` fact and the SSH privilege gate; or DMI, which feeds `gce` and virtualization — is skipped only when every one of its consumers is disabled. Where a kept fact still needs a disabled fact's probe, that probe runs and the disabled fact is prune-only (its work is still paid). `packages` shares no probe, so disabling it fully skips its work — the case the design optimizes for. This gating is new machinery: today every core fact builds eagerly (`buildCoreFacts` takes no disabled-set parameter) and the blocklist filters afterward. Implementing it requires a fact/group-to-resolver mapping and plumbing the disabled set into `buildCoreFacts`; what is reused from the existing blocklist is the disabled-set *expansion* (names and groups), not a resolver-skip mechanism, which does not exist today. A disabled sub-fact (`os.release`) likewise cannot skip its resolver without dropping its siblings, so it is resolve-then-prune through the existing descendant-pruning filter; sub-facts are cheap, so no meaningful work is paid. + +Three semantics fix the surface. The inputs *union* — disabling is additive and order-independent. `--no-block` is the master override: it clears the entire disabled set for that run, including `--disable` and `FACTS_DISABLE`, and resolves everything. *Disable beats query* — a disabled fact named in a query returns nothing, matching Facter's blocklist-beats-query and the engine's existing order (the disabled set is applied before query projection). Because the disabled set can come from the ambient environment (`FACTS_DISABLE` exported in a shell rc) or from config, a query for a fact disabled by a source *other than the same command line* emits a one-line stderr diagnostic (e.g. `fact "packages" is disabled by FACTS_DISABLE`) while keeping stdout empty, so a silently-empty result is diagnosable. A cache-ordering invariant must hold: the disabled set is subtracted before the cache is consulted and before any cache write, and a pruned sub-fact is never persisted into a cached group — so a disabled fact is never served stale from cache (this preserves today's order, where `FilterBlockedFacts` precedes cache resolution). + +`FACTS_DISABLE` is a reserved control key, reserved by its **resolved fact name**, not its literal spelling. The `FACTS_*`/`FACTER_*` prefixes are the external-fact environment namespace, and `environmentFactName` lowercases the variable and strips any of `facts_`, `facts`, `facter_`, or `facter` — so `FACTS_DISABLE`, `FACTSDISABLE`, `FACTER_DISABLE`, and `FACTERDISABLE` all resolve to an external fact named `disable`. The environment-fact loader therefore drops any variable whose resolved fact name is `disable` (and the other reserved control names) rather than reserving one literal string, covering every prefix spelling uniformly. The documented cost is that an external environment fact literally named `disable` cannot be defined. + +Adding `--disable`/`FACTS_DISABLE` and any facts-native fact groups (such as the `packages` group) diverges the Facter-mirrored `--list-block-groups`/`--list-cache-groups` output from Ruby Facter. This is accepted: facts-native ergonomics over the same disabled set, consistent with the facts-native-names-with-facter-compat-inputs pattern. + +## Considered Options + +- **Opt-in / default-off optional tier (enable to turn things on)** — rejected: it requires per-fact "optional" metadata and a query-auto-enable path, and it makes `packages` invisible until asked. The all-on model is simpler and predictable, and resolution-gating still skips `packages`' work for anyone who disables it. +- **Display-only disable (resolve, then hide)** — rejected: it would still pay `packages`' full collection cost on every run; resolution-gating skips the work at the resolver seam. +- **Blocklist-only, no CLI or env (Facter parity as-is)** — rejected: it forces a config file for a one-off and offers no environment control; `--disable` and `FACTS_DISABLE` are the facts-native ergonomics operators expect. +- **All-on, disable-only, resolution-gated (chosen)** — predictable, minimal new user-facing surface, reuses the existing blocklist/group set-expansion, and the disable path skips real work for standalone-resolver facts like `packages`. The new machinery it requires — a fact-to-resolver map and a disabled-set parameter into `buildCoreFacts` — is contained and does not change how existing categories resolve. diff --git a/internal/app/app.go b/internal/app/app.go index ac164049..ade0b5fb 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -248,6 +248,11 @@ func runQuery(stdout, stderr io.Writer, args []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 { return err } @@ -284,13 +289,21 @@ func runQuery(stdout, stderr io.Writer, args []string) error { if !*noExternalFacts { discoveryExternalDirs = effectiveExternalDirs(discoveryExternalDirs) } - blockedFactsForFastPath := map[string]bool{} - var blockedFacts map[string]bool + disabledFactsForFastPath := map[string]bool{} + var disabledFacts map[string]bool if *noBlock { - blockedFacts = map[string]bool{} + // --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 { - blockedFactsForFastPath = engine.BlocklistedFactsForFiltering(configOptions.Blocklist, configOptions.FactGroups) + // The fast path must honor every disable source so a disabled + // facterversion query falls through to normal resolution (and + // disable-beats-query), mirroring the engine's union. + fastPathEntries := append([]string(nil), configOptions.Disabled...) + fastPathEntries = append(fastPathEntries, disableEntries...) + fastPathEntries = append(fastPathEntries, engine.EnvironmentDisabledFacts(os.Environ())...) + disabledFactsForFastPath = engine.DisabledFactsForFiltering(fastPathEntries, configOptions.FactGroups) } mergeDottedFacts := configOptions.ForceDotResolution || *forceDotResolution logLevel := firstNonEmpty(flags.Lookup("log-level").Value.String(), flags.Lookup("l").Value.String(), configOptions.LogLevel) @@ -311,7 +324,7 @@ 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, blockedFactsForFastPath, *noExternalFacts, *timing || *timingShort) { + if canUseVersionQueryFastPath(flags.Args(), discoveryExternalDirs, disabledFactsForFastPath, *noExternalFacts, *timing || *timingShort) { return writeVersionQuery(stdout, *jsonOutput || *jsonOutputShort, *yamlOutput || *yamlOutputShort, *hoconOutput) } resolutionStart := time.Now() @@ -325,7 +338,8 @@ func runQuery(stdout, stderr io.Writer, args []string) error { ExternalDirs: cliExternalDirs, UseCache: !*noCache, NoExternalFacts: *noExternalFacts, - BlockedFacts: blockedFacts, + DisabledFacts: disabledFacts, + ExtraDisabled: disableEntries, DefaultExternalDirsSet: true, DefaultExternalDirs: defaultExternalFactDirs(), IncludeTypedDotted: mergeDottedFacts, @@ -470,8 +484,8 @@ func resolvedLogOptionsConflict(debug, verbose bool, logLevel string) bool { return (debug || verbose) && logLevel != "" } -func canUseVersionQueryFastPath(queries, externalDirs []string, blockedFacts map[string]bool, noExternalFacts, timing bool) bool { - if len(queries) != 1 || queries[0] != "facterversion" || timing || blockedFacts["facterversion"] { +func canUseVersionQueryFastPath(queries, externalDirs []string, disabledFacts map[string]bool, noExternalFacts, timing bool) bool { + if len(queries) != 1 || queries[0] != "facterversion" || timing || disabledFacts["facterversion"] { return false } if !noExternalFacts && len(externalDirs) > 0 { diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 6d72ed72..faf07f0c 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -267,8 +267,11 @@ fact-groups : { if got["site_owner"] != "platform" { t.Fatalf("stdout = %q, want site_owner preserved", stdout.String()) } - if stderr.Len() != 0 { - t.Fatalf("stderr = %q, want empty", stderr.String()) + // An explicitly-queried fact disabled by an ambient (config) source emits + // the one-line ADR-0015 diagnostic so a silently-empty result is + // diagnosable; stdout stays empty for the disabled fact. + if got, want := stderr.String(), "WARN Facts - fact \"site_role\" is disabled by the configuration file\n"; got != want { + t.Fatalf("stderr = %q, want config-disable diagnostic %q", got, want) } } @@ -1242,7 +1245,7 @@ func runtimeOSName() string { // resolved fact, so the assertion matches whatever the binary actually // prints rather than the lowercase GOOS (which only matched on BSDs by // accident when the host's hostname happened to equal the OS name). - collection := engine.Collection(engine.CoreFacts(engine.NewSession())) + collection := engine.Collection(engine.CoreFacts(engine.NewSession(), nil)) if osFact, ok := collection["os"].(map[string]any); ok { if name, ok := osFact["name"].(string); ok && name != "" { return name @@ -1583,7 +1586,7 @@ func TestRun_listBlockGroupsPrintsFactGroups(t *testing.T) { if err := Run(&stdout, &stderr, []string{"--list-block-groups"}); err != nil { t.Fatal(err) } - for _, want := range []string{"networking", "- hostname", "operating system", "- os"} { + for _, want := range []string{"networking", "- networking", "operating system", "- os"} { if !strings.Contains(stdout.String(), want) { t.Fatalf("stdout = %q, want substring %q", stdout.String(), want) } @@ -1599,7 +1602,7 @@ func TestRun_listCacheGroupsPrintsFactGroups(t *testing.T) { if err := Run(&stdout, &stderr, []string{"--list-cache-groups"}); err != nil { t.Fatal(err) } - for _, want := range []string{"networking", "- hostname", "processor", "- processors"} { + for _, want := range []string{"networking", "- networking", "processor", "- processors"} { if !strings.Contains(stdout.String(), want) { t.Fatalf("stdout = %q, want substring %q", stdout.String(), want) } diff --git a/internal/app/disable_test.go b/internal/app/disable_test.go new file mode 100644 index 00000000..ada6d5e6 --- /dev/null +++ b/internal/app/disable_test.go @@ -0,0 +1,78 @@ +package app + +import ( + "bytes" + "encoding/json" + "os" + "path/filepath" + "testing" +) + +func TestRun_disableOptionDropsNamedFacts(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "site.yaml"), []byte("alpha: 1\nbeta: 2\ngamma: 3\n"), 0o600); err != nil { + t.Fatal(err) + } + var stdout, stderr bytes.Buffer + + if err := Run(&stdout, &stderr, []string{"--external-dir", dir, "--disable", "alpha,beta", "--json"}); err != nil { + t.Fatal(err) + } + var got map[string]any + if err := json.Unmarshal(stdout.Bytes(), &got); err != nil { + t.Fatalf("stdout is not JSON: %q: %v", stdout.String(), err) + } + if _, ok := got["alpha"]; ok { + t.Fatalf("stdout = %q, want alpha disabled by --disable", stdout.String()) + } + if _, ok := got["beta"]; ok { + t.Fatalf("stdout = %q, want beta disabled by --disable", stdout.String()) + } + if got["gamma"] == nil { + t.Fatalf("stdout = %q, want gamma resolved", stdout.String()) + } +} + +func TestRun_disableOptionRepeatableAcrossFlags(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "site.yaml"), []byte("alpha: 1\nbeta: 2\ngamma: 3\n"), 0o600); err != nil { + t.Fatal(err) + } + var stdout, stderr bytes.Buffer + + if err := Run(&stdout, &stderr, []string{"--external-dir", dir, "--disable", "alpha", "--disable", "beta", "--json"}); err != nil { + t.Fatal(err) + } + var got map[string]any + if err := json.Unmarshal(stdout.Bytes(), &got); err != nil { + t.Fatalf("stdout is not JSON: %q: %v", stdout.String(), err) + } + if _, ok := got["alpha"]; ok { + t.Fatalf("stdout = %q, want alpha disabled by first --disable", stdout.String()) + } + if _, ok := got["beta"]; ok { + t.Fatalf("stdout = %q, want beta disabled by repeated --disable", stdout.String()) + } + if got["gamma"] == nil { + t.Fatalf("stdout = %q, want gamma resolved", stdout.String()) + } +} + +func TestRun_noBlockClearsDisableOption(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "site.yaml"), []byte("alpha: 1\n"), 0o600); err != nil { + t.Fatal(err) + } + var stdout, stderr bytes.Buffer + + if err := Run(&stdout, &stderr, []string{"--external-dir", dir, "--disable", "alpha", "--no-block", "--json", "alpha"}); err != nil { + t.Fatal(err) + } + var got map[string]any + if err := json.Unmarshal(stdout.Bytes(), &got); err != nil { + t.Fatalf("stdout is not JSON: %q: %v", stdout.String(), err) + } + if got["alpha"] == nil { + t.Fatalf("stdout = %q, want --no-block to clear --disable and resolve alpha", stdout.String()) + } +} diff --git a/internal/cli/options.go b/internal/cli/options.go index 85c82286..772eabae 100644 --- a/internal/cli/options.go +++ b/internal/cli/options.go @@ -63,6 +63,15 @@ var optionDefinitions = []Option{ Man: " * -d, --debug: Enable debug output.", }, }, + { + Canonical: "--disable", + Arity: RequiredValue, + Repeatable: true, + Documentation: OptionDocumentation{ + Help: "\t [--disable] Disable facts or fact groups (comma-separated, repeatable). Standalone resolvers are skipped; others are pruned from output, even when queried.", + Man: " * --disable: Disable facts or fact groups (comma-separated, repeatable). Standalone resolvers are skipped; others are pruned from output, even when queried. Unions with the FACTS_DISABLE environment variable and the config file disable list; --no-block clears the whole set.", + }, + }, { Canonical: "--external-dir", Arity: RequiredValue, diff --git a/internal/engine/builtin_groups_test.go b/internal/engine/builtin_groups_test.go new file mode 100644 index 00000000..ea80edf5 --- /dev/null +++ b/internal/engine/builtin_groups_test.go @@ -0,0 +1,48 @@ +package engine + +import ( + "slices" + "testing" +) + +func TestBuiltinFactGroups_keepStructuredRootsDropLegacyFlatNames(t *testing.T) { + groups := map[string][]string{} + for _, g := range BuiltinFactGroups() { + groups[g.Name] = g.Facts + } + + // The structured root of each group must remain so `--disable ` + // still drops the subtree. + wantRoot := map[string]string{ + "memory": "memory", + "networking": "networking", + "operating system": "os", + "processor": "processors", + "path": "path", + } + for name, root := range wantRoot { + facts, ok := groups[name] + if !ok { + t.Fatalf("BuiltinFactGroups() missing group %q", name) + } + if !slices.Contains(facts, root) { + t.Fatalf("group %q = %#v, want structured root %q kept", name, facts, root) + } + } + + // The legacy flat names removed by ADR-0007 must no longer appear in any + // group; they are no-ops today and only clutter the catalog. + legacy := []string{ + "memoryfree", "memoryfree_mb", "memorysize", "memorysize_mb", + "hostname", "ipaddress", "ipaddress6", "netmask", "domain", "fqdn", + "operatingsystem", "osfamily", "operatingsystemrelease", "architecture", + "processorcount", "physicalprocessorcount", "hardwareisa", + } + for _, g := range BuiltinFactGroups() { + for _, fact := range g.Facts { + if slices.Contains(legacy, fact) { + t.Fatalf("group %q still lists legacy flat name %q", g.Name, fact) + } + } + } +} diff --git a/internal/engine/config.go b/internal/engine/config.go index cd804f60..a9b6805e 100644 --- a/internal/engine/config.go +++ b/internal/engine/config.go @@ -10,6 +10,11 @@ import ( ) var blocklistPattern = regexp.MustCompile(`(?is)blocklist\s*[:=]\s*\[(.*?)\]`) + +// disablePattern matches the facts-native `disable` key, the native spelling of +// the Facter-compatible `blocklist`. It is anchored on a key boundary so it does +// not match `disable` embedded in another word or value. +var disablePattern = regexp.MustCompile(`(?is)(?:^|[\s{,])disable\s*[:=]\s*\[(.*?)\]`) var externalDirPattern = regexp.MustCompile(`(?is)(?:^|[\s{,])external-dir\s*[:=]\s*(\[(.*?)\]|"([^"]*)"|'([^']*)'|([^,\n\r}]+))`) var debugPattern = regexp.MustCompile(`(?is)(?:^|[\s{,])debug\s*[:=]\s*(true|false)`) var verbosePattern = regexp.MustCompile(`(?is)(?:^|[\s{,])verbose\s*[:=]\s*(true|false)`) @@ -32,7 +37,7 @@ var DefaultConfigPath = platformDefaultConfigPath // Config contains the supported Facter config values loaded from a config file. type Config struct { - Blocklist []string + Disabled []string ExternalDirs []string Debug bool Verbose bool @@ -127,8 +132,14 @@ func readConfigOptionsFile(path string, log *slog.Logger) (Config, error) { } cliSection := configSection(content, "cli") sequential, sequentialSet := configBoolValue(content, sequentialPattern) + // The facts-native `disable` key supersedes the Facter-compatible + // `blocklist` when present; otherwise `blocklist` is read unchanged. + disabled := lowerConfigValues(configList(content, blocklistPattern)) + if disable := lowerConfigValues(configList(content, disablePattern)); disable != nil { + disabled = disable + } return Config{ - Blocklist: lowerConfigValues(configList(content, blocklistPattern)), + Disabled: disabled, ExternalDirs: configList(content, externalDirPattern), Debug: configBool(cliSection, debugPattern), Verbose: configBool(cliSection, verbosePattern), diff --git a/internal/engine/config_test.go b/internal/engine/config_test.go index ba24a676..2a0fe909 100644 --- a/internal/engine/config_test.go +++ b/internal/engine/config_test.go @@ -40,7 +40,7 @@ fact-groups : { t.Fatal(err) } want := Config{ - Blocklist: []string{"ec2", "networking"}, + Disabled: []string{"ec2", "networking"}, ExternalDirs: []string{"/opt/facts"}, Debug: true, Verbose: true, @@ -513,8 +513,8 @@ func TestParseConfig_emptyReadableConfigReturnsEmptySections(t *testing.T) { if err != nil { t.Fatal(err) } - if config.Blocklist != nil { - t.Fatalf("Config.Blocklist = %#v, want nil", config.Blocklist) + if config.Disabled != nil { + t.Fatalf("Config.Disabled = %#v, want nil", config.Disabled) } if config.TTLs != nil { t.Fatalf("Config.TTLs = %#v, want nil", config.TTLs) @@ -615,9 +615,48 @@ cli : { if err != nil { t.Fatal(err) } - got := config.Blocklist + got := config.Disabled if want := []string{"ec2", "os", "networking"}; !reflect.DeepEqual(got, want) { - t.Fatalf("Config.Blocklist = %#v, want %#v", got, want) + t.Fatalf("Config.Disabled = %#v, want %#v", got, want) + } +} + +func TestParseConfig_nativeDisableKeyPopulatesDisabledSet(t *testing.T) { + path := filepath.Join(t.TempDir(), "facts.conf") + content := `facts : { + disable : [ "EC2", "packages" ], +}` + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + + config, err := ParseConfig(path, discardLog()) + if err != nil { + t.Fatal(err) + } + got := config.Disabled + if want := []string{"ec2", "packages"}; !reflect.DeepEqual(got, want) { + t.Fatalf("Config.Disabled = %#v, want %#v", got, want) + } +} + +func TestParseConfig_nativeDisableKeySupersedesBlocklist(t *testing.T) { + path := filepath.Join(t.TempDir(), "facts.conf") + content := `facts : { + blocklist : [ "networking" ], + disable : [ "packages" ], +}` + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + + config, err := ParseConfig(path, discardLog()) + if err != nil { + t.Fatal(err) + } + got := config.Disabled + if want := []string{"packages"}; !reflect.DeepEqual(got, want) { + t.Fatalf("Config.Disabled = %#v, want %#v (native disable must supersede blocklist)", got, want) } } @@ -634,10 +673,10 @@ func TestParseConfig_acceptsBareEntries(t *testing.T) { if err != nil { t.Fatal(err) } - got := config.Blocklist + got := config.Disabled want := []string{"ec2", "os.name"} if !reflect.DeepEqual(got, want) { - t.Fatalf("Config.Blocklist = %#v, want %#v", got, want) + t.Fatalf("Config.Disabled = %#v, want %#v", got, want) } } @@ -672,9 +711,9 @@ facts : { if err != nil { t.Fatal(err) } - blocklist := config.Blocklist + blocklist := config.Disabled if want := []string{"os"}; !reflect.DeepEqual(blocklist, want) { - t.Fatalf("Config.Blocklist = %#v, want %#v", blocklist, want) + t.Fatalf("Config.Disabled = %#v, want %#v", blocklist, want) } } @@ -969,48 +1008,48 @@ func TestGroupTTLSecondsRejectsMalformedTTLTokens(t *testing.T) { } } -func TestBlocklistedFactsForFiltering_retiredLegacyGroupBlocksNothing(t *testing.T) { - blocked := BlocklistedFactsForFiltering([]string{"legacy"}, nil) +func TestDisabledFactsForFiltering_retiredLegacyGroupBlocksNothing(t *testing.T) { + blocked := DisabledFactsForFiltering([]string{"legacy"}, nil) facts := []ResolvedFact{ {Name: "os.name", Value: "Darwin"}, {Name: "networking.hostname", Value: "host.example.com"}, {Name: "processors.count", Value: 8}, } - got := FilterBlockedFacts(facts, blocked) + got := FilterDisabledFacts(facts, blocked) if !reflect.DeepEqual(got, facts) { - t.Fatalf("FilterBlockedFacts(legacy blocklist) = %#v, want discovery unchanged %#v", got, facts) + t.Fatalf("FilterDisabledFacts(legacy blocklist) = %#v, want discovery unchanged %#v", got, facts) } } -func TestBlocklistedFactsWithGroups_expandsGroupWithoutBlockingGroupName(t *testing.T) { - got := BlocklistedFactsWithGroups([]string{"blocked_group", "blocked_fact"}, []FactGroup{{Name: "blocked_group", Facts: []string{"fact1", "fact2"}}}) +func TestDisabledFactsWithGroups_expandsGroupWithoutBlockingGroupName(t *testing.T) { + got := DisabledFactsWithGroups([]string{"blocked_group", "blocked_fact"}, []FactGroup{{Name: "blocked_group", Facts: []string{"fact1", "fact2"}}}) want := map[string]bool{"fact1": true, "fact2": true, "blocked_fact": true} if !reflect.DeepEqual(got, want) { - t.Fatalf("BlocklistedFactsWithGroups() = %#v, want %#v", got, want) + t.Fatalf("DisabledFactsWithGroups() = %#v, want %#v", got, want) } } -func TestFilterBlockedFacts_blocksExactNameAndRoot(t *testing.T) { +func TestFilterDisabledFacts_blocksExactNameAndRoot(t *testing.T) { facts := []ResolvedFact{ {Name: "os.name", Value: "Darwin", Type: "core"}, {Name: "networking.hostname", Value: "host.example.com", Type: "core"}, } - got := FilterBlockedFacts(facts, map[string]bool{"os.name": true}) + got := FilterDisabledFacts(facts, map[string]bool{"os.name": true}) want := []ResolvedFact{{Name: "networking.hostname", Value: "host.example.com", Type: "core"}} if !reflect.DeepEqual(got, want) { - t.Fatalf("FilterBlockedFacts(os.name) = %#v, want %#v", got, want) + t.Fatalf("FilterDisabledFacts(os.name) = %#v, want %#v", got, want) } - got = FilterBlockedFacts(facts, map[string]bool{"networking": true}) + got = FilterDisabledFacts(facts, map[string]bool{"networking": true}) want = []ResolvedFact{{Name: "os.name", Value: "Darwin", Type: "core"}} if !reflect.DeepEqual(got, want) { - t.Fatalf("FilterBlockedFacts(networking) = %#v, want %#v", got, want) + t.Fatalf("FilterDisabledFacts(networking) = %#v, want %#v", got, want) } } -func TestFilterBlockedFacts_prunesBlockedDescendantsFromStructuredParents(t *testing.T) { +func TestFilterDisabledFacts_prunesDisabledDescendantsFromStructuredParents(t *testing.T) { facts := []ResolvedFact{ { Name: "os", @@ -1025,7 +1064,7 @@ func TestFilterBlockedFacts_prunesBlockedDescendantsFromStructuredParents(t *tes }, } - got := FilterBlockedFacts(facts, map[string]bool{"os.release.major": true}) + got := FilterDisabledFacts(facts, map[string]bool{"os.release.major": true}) want := []ResolvedFact{ { Name: "os", @@ -1039,11 +1078,11 @@ func TestFilterBlockedFacts_prunesBlockedDescendantsFromStructuredParents(t *tes }, } if !reflect.DeepEqual(got, want) { - t.Fatalf("FilterBlockedFacts(os.release.major) = %#v, want %#v", got, want) + t.Fatalf("FilterDisabledFacts(os.release.major) = %#v, want %#v", got, want) } } -func TestFilterBlockedFactsPrunesEmptyMapsAndKeepsOriginalValue(t *testing.T) { +func TestFilterDisabledFactsPrunesEmptyMapsAndKeepsOriginalValue(t *testing.T) { original := map[string]any{ "name": "Ubuntu", "release": map[string]any{ @@ -1053,14 +1092,14 @@ func TestFilterBlockedFactsPrunesEmptyMapsAndKeepsOriginalValue(t *testing.T) { } facts := []ResolvedFact{{Name: "os", Value: original, Type: "core"}} - got := FilterBlockedFacts(facts, map[string]bool{ + got := FilterDisabledFacts(facts, map[string]bool{ "os.release.full": true, "os.release.major": true, "os.missing.nothing": true, }) want := []ResolvedFact{{Name: "os", Value: map[string]any{"name": "Ubuntu"}, Type: "core"}} if !reflect.DeepEqual(got, want) { - t.Fatalf("FilterBlockedFacts() = %#v, want %#v", got, want) + t.Fatalf("FilterDisabledFacts() = %#v, want %#v", got, want) } if _, ok := original["release"]; !ok { t.Fatalf("original value = %#v, want unpruned source map", original) diff --git a/internal/engine/core.go b/internal/engine/core.go index aaa16abf..4ed57ebc 100644 --- a/internal/engine/core.go +++ b/internal/engine/core.go @@ -5,6 +5,7 @@ import ( "os" "path/filepath" "runtime" + "sort" "strconv" "strings" ) @@ -14,23 +15,49 @@ import ( // The literal below is the dev-build fallback when no tag is injected. var Version = "dev" // ponytail: var, not const, so the build pipeline can inject the git tag -// CoreFacts returns the small cross-platform fact set used by the initial Go CLI. -func CoreFacts(s *Session) []ResolvedFact { +// CoreFacts returns the small cross-platform fact set used by the initial Go +// CLI. disabled is the resolution-gating set: single-output categories whose +// top-level fact name it contains skip resolution entirely (ADR-0015). The +// result is memoized per Session, keyed by a fingerprint of the disabled set, so +// a repeat call with the same set reuses the build while a call with a different +// set rebuilds (the gating depends on disabled, so a stale memo would misgate). +func CoreFacts(s *Session, disabled map[string]bool) []ResolvedFact { + fingerprint := disabledFingerprint(disabled) s.coreFacts.mu.Lock() defer s.coreFacts.mu.Unlock() - if s.coreFacts.facts == nil { - s.coreFacts.facts = buildCoreFacts(s) + if !s.coreFacts.built || s.coreFacts.fingerprint != fingerprint { + s.coreFacts.facts = buildCoreFacts(s, disabled) + s.coreFacts.fingerprint = fingerprint + s.coreFacts.built = true } return append([]ResolvedFact(nil), s.coreFacts.facts...) } +// disabledFingerprint is a stable identity for a disabled set — its enabled keys +// sorted and comma-joined. CoreFacts keys its per-Session memo on this so two +// calls gate identically iff their fingerprints match. Only true-valued keys +// gate (buildCoreFacts checks disabled[fact]), so a false-valued key is omitted. +func disabledFingerprint(disabled map[string]bool) string { + if len(disabled) == 0 { + return "" + } + keys := make([]string, 0, len(disabled)) + for name, on := range disabled { + if on { + keys = append(keys, name) + } + } + sort.Strings(keys) + return strings.Join(keys, ",") +} + // buildCoreFacts composes the resolved core-fact set as the ordered union of // each fact category's package-internal assembly function plus the cross-cutting // facts (facterversion, path) and the virtualization-derived facts the cloud // resolvers depend on. Facts are collected into a name-keyed canonical tree, so // the composition order does not affect the resolved output; it mirrors the // historical assembly order for reviewability. -func buildCoreFacts(s *Session) []ResolvedFact { +func buildCoreFacts(s *Session, disabled map[string]bool) []ResolvedFact { virtualization := detectVirtualization(s) virtualFact, isVirtualFact := virtualizationFactValues(virtualization) dmi := s.cachedDMI() @@ -40,20 +67,34 @@ func buildCoreFacts(s *Session) []ResolvedFact { {Name: "path", Value: currentPathEntries(runtime.GOOS, os.Getenv)}, {Name: "virtual", Value: virtualFact}, } - facts = append(facts, networkingCoreFacts(s)...) - facts = append(facts, processorsCoreFacts(s)...) - facts = append(facts, memoryCoreFacts(s)...) + // 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 + } + 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)...) - facts = append(facts, sshCoreFacts(s)...) + gate("ssh", sshCoreFacts) facts = append(facts, identityCoreFacts(s)...) facts = append(facts, uptimeCoreFacts(s)...) facts = append(facts, selinuxCoreFacts(s)...) - facts = append(facts, fipsCoreFacts(s)...) - facts = append(facts, timezoneCoreFacts(s)...) - facts = append(facts, augeasCoreFacts(s)...) - facts = append(facts, xenCoreFacts(s)...) + gate("fips_enabled", fipsCoreFacts) + gate("timezone", timezoneCoreFacts) + gate("augeas", augeasCoreFacts) + gate("xen", xenCoreFacts) facts = append(facts, currentLinuxHypervisorFacts(s)...) facts = append(facts, currentWindowsHypervisorFacts(s)...) facts = append(facts, azureFacts(s.Context(), newAzureClient(azureMetadataBaseURL, nil), virtualization)...) diff --git a/internal/engine/core_benchmark_test.go b/internal/engine/core_benchmark_test.go index 2900ac9f..bbe8f7a9 100644 --- a/internal/engine/core_benchmark_test.go +++ b/internal/engine/core_benchmark_test.go @@ -9,7 +9,7 @@ import ( func BenchmarkCoreFacts(b *testing.B) { b.ReportAllocs() for b.Loop() { - _ = CoreFacts(testSession) + _ = CoreFacts(testSession, nil) } } diff --git a/internal/engine/core_gating_test.go b/internal/engine/core_gating_test.go new file mode 100644 index 00000000..3109d0f1 --- /dev/null +++ b/internal/engine/core_gating_test.go @@ -0,0 +1,220 @@ +package engine + +import ( + "context" + "runtime" + "strings" + "testing" +) + +// rootNames returns the set of distinct top-level (pre-dot) fact roots present +// in facts. +func rootNames(facts []ResolvedFact) map[string]bool { + roots := map[string]bool{} + for _, f := range facts { + root, _, _ := strings.Cut(f.Name, ".") + roots[root] = true + } + return roots +} + +// hasRoot reports whether any fact in facts has the given top-level root. +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", +} + +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 { + 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. + continue + } + gated := buildCoreFacts(NewSession(), map[string]bool{fact: true}) + if hasRoot(gated, fact) { + t.Fatalf("buildCoreFacts(disabled=%q) still emitted %q root; resolver was not gated", fact, fact) + } + } +} + +func TestBuildCoreFacts_multiOutputCategoryStaysEager(t *testing.T) { + // Disabling the multi-output root `os` must NOT gate osCoreFacts: it stays + // eager (resolve-then-prune) so its sibling kernel/filesystems outputs are + // not collateral. buildCoreFacts still emits os.* because it never prunes. + facts := buildCoreFacts(NewSession(), map[string]bool{"os": true}) + if !hasRoot(facts, "os") { + t.Fatal("buildCoreFacts(disabled=os) dropped os.*; multi-output category must stay eager") + } +} + +func TestBuildCoreFacts_multiOutputSubfactStaysEagerThenPruned(t *testing.T) { + disabled := map[string]bool{"os.release": true} + facts := buildCoreFacts(NewSession(), disabled) + // The resolver still runs (os.name present) and still emits os.release; + // only the later FilterDisabledFacts prunes the disabled sub-fact. + var sawName, sawRelease bool + for _, f := range facts { + switch { + case f.Name == "os.name": + sawName = true + case f.Name == "os.release" || strings.HasPrefix(f.Name, "os.release."): + sawRelease = true + } + } + if !sawName { + t.Fatal("buildCoreFacts(disabled=os.release) dropped os.name; sub-fact disable must not gate the resolver") + } + if !sawRelease { + t.Fatal("buildCoreFacts(disabled=os.release) did not emit os.release before pruning; expected resolve-then-prune") + } + filtered := FilterDisabledFacts(facts, disabled) + for _, f := range filtered { + if f.Name == "os.release" || strings.HasPrefix(f.Name, "os.release.") { + t.Fatalf("FilterDisabledFacts kept disabled sub-fact %q", f.Name) + } + } + if !hasRoot(filtered, "os") { + t.Fatal("FilterDisabledFacts dropped the whole os root when only os.release was disabled") + } +} + +func TestBuildCoreFacts_keptCategoriesUnaffectedByGate(t *testing.T) { + // Gating timezone must leave the other categories' roots untouched. + baseline := buildCoreFacts(NewSession(), nil) + gated := buildCoreFacts(NewSession(), map[string]bool{"timezone": true}) + for root := range rootNames(baseline) { + if root == "timezone" { + continue + } + if !hasRoot(gated, root) { + t.Fatalf("gating timezone dropped unrelated root %q", root) + } + } +} + +// newGatingProbeHost builds a fake Linux host so the gated resolvers that branch +// on s.goos() take a path reading a distinctive marker file; the readFile/run +// spies then record whether each resolver actually ran. A Linux fake is used +// regardless of the test host so the s.goos()-keyed categories +// (networking/processors/memory/xen) are observable on darwin, linux, and +// Windows CI alike. +func newGatingProbeHost() *fakeHostOS { + return &fakeHostOS{platform: "linux"} +} + +func gatingProbeSession(host *fakeHostOS) *Session { + s := NewSessionContext(context.Background()) + s.host = host + return s +} + +func hostRanCommand(h *fakeHostOS, substr string) bool { + for _, call := range h.runCalls { + if strings.Contains(call.name, substr) { + return true + } + } + return false +} + +func hostReadFileMatching(h *fakeHostOS, substr string) bool { + for _, path := range h.readFileCalls { + if strings.Contains(path, substr) { + return true + } + } + return false +} + +// TestBuildCoreFacts_resolutionGatingSkipsProbeWork proves the gate skips real +// work, not just output: each gated category's distinctive host probe must run +// when the category is enabled and must NOT run when it is disabled. A display +// filter (resolve-then-hide) would satisfy the root-absence tests above while +// still invoking the probe — these probe-call assertions are what distinguish +// resolution-gating from output filtering (ADR-0015). +func TestBuildCoreFacts_resolutionGatingSkipsProbeWork(t *testing.T) { + markers := []struct { + category string + probed func(h *fakeHostOS) bool + skip func() bool + skipReason string + }{ + { + category: "networking", + probed: func(h *fakeHostOS) bool { return hostReadFileMatching(h, "/etc/resolv.conf") }, + }, + { + category: "processors", + probed: func(h *fakeHostOS) bool { return hostReadFileMatching(h, "/proc/cpuinfo") }, + }, + { + category: "memory", + probed: func(h *fakeHostOS) bool { return hostReadFileMatching(h, "/proc/meminfo") }, + }, + { + category: "xen", + probed: func(h *fakeHostOS) bool { return hostReadFileMatching(h, "/proc/xen/capabilities") }, + }, + { + // augeas runs `augparse --version` on every platform (no goos branch), + // so it is observable through the run spy regardless of the test host. + category: "augeas", + probed: func(h *fakeHostOS) bool { return hostRanCommand(h, "augparse") }, + }, + { + // ssh reads ssh_host_*_key.pub via readFile on every non-Windows host; + // the path set keys off runtime.GOOS, which the fake cannot override, + // so the Windows path (programdata + a different seam) is skipped. + category: "ssh", + probed: func(h *fakeHostOS) bool { return hostReadFileMatching(h, "ssh_host_rsa_key.pub") }, + skip: func() bool { return runtime.GOOS == "windows" }, + skipReason: "ssh host-key paths key off runtime.GOOS; the Windows path uses a different seam", + }, + { + // fips_enabled reads /proc/sys/crypto/fips_enabled only when + // runtime.GOOS is linux (Windows uses a reg query); elsewhere the + // resolver returns nil before any host call, so there is nothing to + // observe on this host. + category: "fips_enabled", + probed: func(h *fakeHostOS) bool { return hostReadFileMatching(h, "/proc/sys/crypto/fips_enabled") }, + skip: func() bool { return runtime.GOOS != "linux" }, + skipReason: "fips_enabled probes only on linux/windows; runtime.GOOS gates the probe away here", + }, + // timezone is intentionally omitted: on every non-Windows host it derives + // the zone from Go's time.Now().Format("MST") with no host probe at all, + // so no recorded seam exists to observe. Its gating is covered by the + // root-absence test above; a probe-work proof is unreachable. + } + + for _, m := range markers { + t.Run(m.category, func(t *testing.T) { + if m.skip != nil && m.skip() { + t.Skipf("%s probe unobservable on %s: %s", m.category, runtime.GOOS, m.skipReason) + } + + enabled := newGatingProbeHost() + buildCoreFacts(gatingProbeSession(enabled), nil) + if !m.probed(enabled) { + t.Fatalf("%s enabled: its probe did not run, so the test cannot prove gating (reads=%v runs=%v)", + m.category, enabled.readFileCalls, enabled.runCalls) + } + + disabled := newGatingProbeHost() + buildCoreFacts(gatingProbeSession(disabled), map[string]bool{m.category: true}) + if m.probed(disabled) { + t.Fatalf("%s disabled: its probe STILL ran — gating only filtered output instead of skipping work", m.category) + } + }) + } +} diff --git a/internal/engine/core_test.go b/internal/engine/core_test.go index 859649d3..320e649b 100644 --- a/internal/engine/core_test.go +++ b/internal/engine/core_test.go @@ -11,7 +11,7 @@ import ( ) func TestCoreFacts_includeIntegrationRootFactGroups(t *testing.T) { - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) for _, name := range []string{"memory", "networking", "os", "path", "processors"} { if _, ok := collection[name]; !ok { @@ -20,10 +20,26 @@ func TestCoreFacts_includeIntegrationRootFactGroups(t *testing.T) { } } +func TestCoreFacts_rebuildsWhenDisabledSetChangesOnSameSession(t *testing.T) { + s := NewSession() + + gated := Collection(CoreFacts(s, map[string]bool{"processors": true})) + if _, ok := gated["processors"]; ok { + t.Fatalf("CoreFacts(disabled=processors) still emitted processors; resolver was not gated: %#v", gated) + } + + // A second call on the SAME session with a different disabled set must + // rebuild from the new set rather than return the first call's stale gating. + full := Collection(CoreFacts(s, nil)) + if _, ok := full["processors"]; !ok { + t.Fatalf("CoreFacts(s, nil) after a gated call is missing processors; the memo ignored the changed disabled set: %#v", full) + } +} + func TestCoreFacts_includePathFromEnvironment(t *testing.T) { entries := []string{"/usr/bin", "/etc", "/usr/sbin", "/usr/ucb", "/usr/bin/X11", "/sbin", "/usr/java6/jre/bin", "/usr/java6/bin"} t.Setenv("PATH", strings.Join(entries, string(os.PathListSeparator))) - collection := Collection(CoreFacts(NewSession())) + collection := Collection(CoreFacts(NewSession(), nil)) got, ok := collection["path"].([]string) if !ok { @@ -95,7 +111,7 @@ func TestRootedPathAndIsSymlinkHelpers(t *testing.T) { } func TestCoreFacts_includeFacterVersion(t *testing.T) { - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) if got := collection["facterversion"]; got != Version { t.Fatalf("facterversion = %#v, want %#v", got, Version) @@ -112,7 +128,7 @@ func TestCoreFacts_omitsRubyAndPuppetPackageVersionFacts(t *testing.T) { s := NewSessionContext(context.Background()) s.host = host - collection := Collection(CoreFacts(s)) + collection := Collection(CoreFacts(s, nil)) for _, name := range []string{"ruby", "aio_agent_version"} { if got, ok := collection[name]; ok { t.Fatalf("CoreFacts() %s = %#v, want absent", name, got) @@ -178,14 +194,14 @@ func assertNoLegacyAliases(t *testing.T, collection map[string]any) { } func TestCoreFacts_excludeAllLegacyAliases(t *testing.T) { - assertNoLegacyAliases(t, Collection(CoreFacts(testSession))) + assertNoLegacyAliases(t, Collection(CoreFacts(testSession, nil))) } // Sweep: a default host discovery emits no top-level fact whose value is an // empty string or empty map — unresolvable facts are absent, never // placeholders (openspec change omit-not-applicable-facts). func TestCoreFacts_defaultDiscoveryHasNoEmptyTopLevelFacts(t *testing.T) { - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) for name, value := range collection { switch v := value.(type) { @@ -204,7 +220,7 @@ func TestCoreFacts_defaultDiscoveryHasNoEmptyTopLevelFacts(t *testing.T) { // Platform-inapplicable facts are absent from the host collection, matching // the platforms where Ruby Facter resolves them. func TestCoreFacts_omitPlatformInapplicableFacts(t *testing.T) { - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) if runtime.GOOS != "linux" && runtime.GOOS != "windows" { if value, ok := collection["fips_enabled"]; ok { diff --git a/internal/engine/disable_diagnostic_test.go b/internal/engine/disable_diagnostic_test.go new file mode 100644 index 00000000..e8db39fb --- /dev/null +++ b/internal/engine/disable_diagnostic_test.go @@ -0,0 +1,139 @@ +package engine + +import ( + "context" + "errors" + "strings" + "testing" +) + +func warnContains(warnings []string, substr string) bool { + for _, w := range warnings { + if strings.Contains(w, substr) { + return true + } + } + return false +} + +func TestDiscover_diagnosesConfigDisabledExplicitQuery(t *testing.T) { + var warnings []string + eng, err := NewEngine(EngineConfig{ + ConfigLoaded: true, + Config: Config{Disabled: []string{"site_role"}}, + NoExternalFacts: true, + Logger: captureLogger(nil, &warnings, nil), + }) + if err != nil { + t.Fatal(err) + } + + snap, err := eng.Discover(context.Background(), "site_role") + if err != nil { + t.Fatal(err) + } + if !warnContains(warnings, `fact "site_role" is disabled by `+disabledSourceConfig) { + t.Fatalf("warnings = %#v, want config-disable diagnostic for site_role", warnings) + } + // The diagnostic and the suppression are one contract: the queried fact must + // also be absent from the snapshot, not merely warned about. + if _, err := snap.Value("site_role"); !errors.Is(err, ErrFactNotFound) { + t.Fatalf("snapshot.Value(site_role) err = %v, want ErrFactNotFound (disabled fact suppressed)", err) + } +} + +func TestDiscover_diagnosesEnvDisabledExplicitQuery(t *testing.T) { + t.Setenv("FACTS_DISABLE", "networking") + var warnings []string + eng, err := NewEngine(EngineConfig{ + CLICompat: true, + NoExternalFacts: true, + Logger: captureLogger(nil, &warnings, nil), + }) + if err != nil { + t.Fatal(err) + } + + snap, err := eng.Discover(context.Background(), "networking") + if err != nil { + t.Fatal(err) + } + if !warnContains(warnings, `fact "networking" is disabled by FACTS_DISABLE`) { + t.Fatalf("warnings = %#v, want FACTS_DISABLE diagnostic for networking", warnings) + } + if _, err := snap.Value("networking"); !errors.Is(err, ErrFactNotFound) { + t.Fatalf("snapshot.Value(networking) err = %v, want ErrFactNotFound (disabled fact suppressed)", err) + } +} + +func TestDiscover_doesNotDiagnoseCLIDisabledExplicitQuery(t *testing.T) { + var warnings []string + eng, err := NewEngine(EngineConfig{ + CLICompat: true, + NoExternalFacts: true, + ExtraDisabled: []string{"networking"}, + Logger: captureLogger(nil, &warnings, nil), + }) + if err != nil { + t.Fatal(err) + } + + if _, err := eng.Discover(context.Background(), "networking"); err != nil { + t.Fatal(err) + } + if warnContains(warnings, "is disabled by") { + t.Fatalf("warnings = %#v, want no diagnostic for a --disable on the same command line", warnings) + } +} + +// A --disable on the command line subsumes an ambient (config/env) disable of a +// descendant, so the ambient diagnostic must stay quiet: the operator already +// asked for the broader disable on this very command line. +func TestDiscover_cliDisableSilencesAmbientDescendantDiagnostic(t *testing.T) { + var warnings []string + eng, err := NewEngine(EngineConfig{ + CLICompat: true, + ConfigLoaded: true, + Config: Config{Disabled: []string{"networking.ip"}}, + ExtraDisabled: []string{"networking"}, + NoExternalFacts: true, + Logger: captureLogger(nil, &warnings, nil), + }) + if err != nil { + t.Fatal(err) + } + + if _, err := eng.Discover(context.Background(), "networking.ip"); err != nil { + t.Fatal(err) + } + if warnContains(warnings, "is disabled by") { + t.Fatalf("warnings = %#v, want no diagnostic: --disable networking subsumes the ambient networking.ip", warnings) + } +} + +// An ambient disable of an ancestor must diagnose (and suppress) a descendant +// query: config disable of os.release fires for a query of os.release.major, +// matching FilterDisabledFacts' descendant pruning. +func TestDiscover_diagnosesAmbientAncestorDisableForDescendantQuery(t *testing.T) { + var warnings []string + eng, err := NewEngine(EngineConfig{ + ConfigLoaded: true, + Config: Config{Disabled: []string{"os.release"}}, + NoExternalFacts: true, + Logger: captureLogger(nil, &warnings, nil), + }) + if err != nil { + t.Fatal(err) + } + + snap, err := eng.Discover(context.Background(), "os.release.major") + if err != nil { + t.Fatal(err) + } + if !warnContains(warnings, `fact "os.release.major" is disabled by `+disabledSourceConfig) { + t.Fatalf("warnings = %#v, want config-disable diagnostic for os.release.major via os.release ancestor", warnings) + } + if _, err := snap.Value("os.release.major"); !errors.Is(err, ErrFactNotFound) { + t.Fatalf("snapshot.Value(os.release.major) err = %v, want ErrFactNotFound (ancestor disable suppresses descendant)", err) + } +} diff --git a/internal/engine/discovery_plan.go b/internal/engine/discovery_plan.go index 7ec72caa..983b0a92 100644 --- a/internal/engine/discovery_plan.go +++ b/internal/engine/discovery_plan.go @@ -1,11 +1,15 @@ package engine -import "slices" +import ( + "slices" + "strings" +) type discoveryPlan struct { externalDirs []string noExternalFacts bool - blockedFacts map[string]bool + disabledFacts map[string]bool + ambientDisabled map[string]string useCache bool cacheTTLs []FactTTL cacheGroups []FactGroup @@ -19,7 +23,7 @@ func (e *Engine) planDiscovery(s *Session, queries []string) (discoveryPlan, []e plan := discoveryPlan{ externalDirs: slices.Clone(e.cfg.ExternalDirs), noExternalFacts: e.cfg.NoExternalFacts, - blockedFacts: cloneBoolMap(e.cfg.BlockedFacts), + disabledFacts: cloneBoolMap(e.cfg.DisabledFacts), useCache: e.cfg.UseCache, loaderMode: externalFactLoaderLibrary, includeEnv: e.cfg.SystemDefaults, @@ -41,9 +45,6 @@ func (e *Engine) planDiscovery(s *Session, queries []string) (discoveryPlan, []e } 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) } @@ -51,8 +52,15 @@ func (e *Engine) planDiscovery(s *Session, queries []string) (discoveryPlan, []e plan.includeTypedDotted = true } } - if plan.blockedFacts == nil { - plan.blockedFacts = map[string]bool{} + // A non-nil DisabledFacts override is used verbatim (this is how --no-block + // clears everything and how a library consumer forces an explicit set); + // otherwise the disabled set is the union of config, FACTS_DISABLE, and the + // --disable CLI entries, each expanded through the configured fact groups. + if e.cfg.DisabledFacts == nil { + plan.disabledFacts, plan.ambientDisabled = e.unionDisabledFacts(s, config, plan.includeEnv) + } + if plan.disabledFacts == nil { + plan.disabledFacts = map[string]bool{} } if !plan.noExternalFacts && len(plan.externalDirs) == 0 && e.cfg.SystemDefaults { plan.externalDirs = e.defaultExternalDirs() @@ -60,6 +68,55 @@ func (e *Engine) planDiscovery(s *Session, queries []string) (discoveryPlan, []e return plan, failures } +// disabledSourceEnv and disabledSourceConfig label the ambient (non-CLI) +// sources of a disabled fact for the explicit-query diagnostic. +const ( + disabledSourceEnv = "FACTS_DISABLE" + disabledSourceConfig = "the configuration file" +) + +// unionDisabledFacts builds the disabled set as the union of the config +// `disable`/`blocklist` list, the FACTS_DISABLE environment control (only when +// includeEnv is set, consistent with environment facts), and the --disable CLI +// entries, expanding each source through the configured fact groups. It also +// returns the ambient map naming the env/config source of each disabled fact +// that is not also named on the command line, for the explicit-query +// diagnostic. +func (e *Engine) unionDisabledFacts(s *Session, config Config, includeEnv bool) (map[string]bool, map[string]string) { + groups := config.FactGroups + disabled := map[string]bool{} + ambient := map[string]string{} + + for name := range DisabledFactsWithGroups(config.Disabled, groups) { + disabled[name] = true + ambient[name] = disabledSourceConfig + } + if includeEnv { + for name := range DisabledFactsWithGroups(environmentDisabledFacts(s.host.environ()), groups) { + disabled[name] = true + ambient[name] = disabledSourceEnv + } + } + // --disable entries are "on the same command line", so they join the + // disabled set but never trigger the ambient diagnostic; a name they cover + // is dropped from the ambient map even if config/env also disabled it. The + // drop also covers descendants, mirroring pruneDisabledDescendants: a + // --disable of `networking` silences an ambient `networking.ip` it subsumes. + for name := range DisabledFactsWithGroups(e.cfg.ExtraDisabled, groups) { + disabled[name] = true + delete(ambient, name) + for k := range ambient { + if strings.HasPrefix(k, name+".") { + delete(ambient, k) + } + } + } + if len(ambient) == 0 { + ambient = nil + } + return disabled, ambient +} + func (e *Engine) configForDiscovery(s *Session) (Config, bool, error) { if e.cfg.ConfigLoaded { return cloneConfig(e.cfg.Config), true, nil @@ -90,7 +147,7 @@ func cloneBoolMap(in map[string]bool) map[string]bool { } func cloneConfig(in Config) Config { - in.Blocklist = slices.Clone(in.Blocklist) + in.Disabled = slices.Clone(in.Disabled) in.ExternalDirs = slices.Clone(in.ExternalDirs) in.TTLs = slices.Clone(in.TTLs) in.FactGroups = cloneFactGroups(in.FactGroups) diff --git a/internal/engine/discovery_plan_test.go b/internal/engine/discovery_plan_test.go new file mode 100644 index 00000000..fd393ba7 --- /dev/null +++ b/internal/engine/discovery_plan_test.go @@ -0,0 +1,116 @@ +package engine + +import ( + "reflect" + "testing" +) + +// sessionWithEnviron returns a Session whose host.environ() reports env, used to +// drive the FACTS_DISABLE branch of planDiscovery without touching the process +// environment. +func sessionWithEnviron(env []string) *Session { + s := NewSession() + s.host = &fakeHostOS{environEntries: env} + return s +} + +func TestPlanDiscoveryUnionsConfigEnvAndCLISources(t *testing.T) { + eng, err := NewEngine(EngineConfig{ + ConfigLoaded: true, + Config: Config{Disabled: []string{"config_fact"}}, + ExtraDisabled: []string{"cli_fact"}, + SystemDefaults: true, + }) + if err != nil { + t.Fatal(err) + } + + plan, failures := eng.planDiscovery(sessionWithEnviron([]string{"FACTS_DISABLE=env_fact"}), nil) + if len(failures) != 0 { + t.Fatalf("failures = %#v, want none", failures) + } + for _, name := range []string{"config_fact", "env_fact", "cli_fact"} { + if !plan.disabledFacts[name] { + t.Fatalf("disabledFacts = %#v, want union to include %q", plan.disabledFacts, name) + } + } +} + +func TestPlanDiscoveryExpandsGroupsAcrossEverySource(t *testing.T) { + // Each source disables a DISTINCT group with disjoint members, so a single + // working source cannot mask a broken one: every group's members must appear + // only if that source's own expansion ran. + groups := []FactGroup{ + {Name: "config_group", Facts: []string{"config_alpha", "config_beta"}}, + {Name: "env_group", Facts: []string{"env_alpha", "env_beta"}}, + {Name: "cli_group", Facts: []string{"cli_alpha", "cli_beta"}}, + } + eng, err := NewEngine(EngineConfig{ + ConfigLoaded: true, + Config: Config{Disabled: []string{"config_group"}, FactGroups: groups}, + ExtraDisabled: []string{"cli_group"}, + SystemDefaults: true, + }) + if err != nil { + t.Fatal(err) + } + + plan, _ := eng.planDiscovery(sessionWithEnviron([]string{"FACTS_DISABLE=env_group"}), nil) + bySource := map[string][]string{ + "config (disable)": {"config_alpha", "config_beta"}, + "FACTS_DISABLE": {"env_alpha", "env_beta"}, + "ExtraDisabled (--disable)": {"cli_alpha", "cli_beta"}, + } + for source, members := range bySource { + for _, name := range members { + if !plan.disabledFacts[name] { + t.Fatalf("disabledFacts = %#v, want %s group member %q expanded", plan.disabledFacts, source, name) + } + } + } +} + +func TestPlanDiscoverySuppressesEnvDisableWhenIncludeEnvFalse(t *testing.T) { + eng, err := NewEngine(EngineConfig{ + ConfigLoaded: true, + Config: Config{Disabled: []string{"config_fact"}}, + ExtraDisabled: []string{"cli_fact"}, + // No SystemDefaults and no CLICompat: includeEnv is false, so the + // ambient FACTS_DISABLE source must be ignored, just like env facts. + }) + if err != nil { + t.Fatal(err) + } + + plan, _ := eng.planDiscovery(sessionWithEnviron([]string{"FACTS_DISABLE=env_fact"}), nil) + if plan.includeEnv { + t.Fatal("includeEnv = true, want false for a hermetic engine") + } + if plan.disabledFacts["env_fact"] { + t.Fatalf("disabledFacts = %#v, want env_fact suppressed when includeEnv is false", plan.disabledFacts) + } + for _, name := range []string{"config_fact", "cli_fact"} { + if !plan.disabledFacts[name] { + t.Fatalf("disabledFacts = %#v, want %q even with env suppressed", plan.disabledFacts, name) + } + } +} + +func TestPlanDiscoveryDisabledFactsOverrideWinsOverUnion(t *testing.T) { + eng, err := NewEngine(EngineConfig{ + DisabledFacts: map[string]bool{"override": true}, + ConfigLoaded: true, + Config: Config{Disabled: []string{"config_fact"}}, + ExtraDisabled: []string{"cli_fact"}, + SystemDefaults: true, + }) + if err != nil { + t.Fatal(err) + } + + plan, _ := eng.planDiscovery(sessionWithEnviron([]string{"FACTS_DISABLE=env_fact"}), nil) + want := map[string]bool{"override": true} + if !reflect.DeepEqual(plan.disabledFacts, want) { + t.Fatalf("disabledFacts = %#v, want override verbatim %#v (--no-block / explicit set wins)", plan.disabledFacts, want) + } +} diff --git a/internal/engine/disks_test.go b/internal/engine/disks_test.go index 1917b5e7..adbfb7d7 100644 --- a/internal/engine/disks_test.go +++ b/internal/engine/disks_test.go @@ -1068,7 +1068,7 @@ func TestCoreFacts_includeFilesystems(t *testing.T) { t.Skipf("filesystems resolution is not implemented on %s", runtime.GOOS) } - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) got, ok := collection["filesystems"].([]string) if !ok || len(got) == 0 { t.Fatalf("filesystems = %#v, want non-empty array of filesystem names", collection["filesystems"]) @@ -1561,7 +1561,7 @@ func TestCoreFacts_includeRootMountpoint(t *testing.T) { t.Skipf("mountpoints resolution is not implemented on %s", runtime.GOOS) } - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) mountpoints, ok := collection["mountpoints"].(map[string]any) if !ok { t.Fatalf("mountpoints fact = %#v, want map", collection["mountpoints"]) diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 547f32b8..7e410483 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -55,8 +55,15 @@ type EngineConfig struct { CLICompat bool // NoExternalFacts skips external-fact loading (--no-external-facts). NoExternalFacts bool - // BlockedFacts overrides the config-derived blocklist when non-nil. - BlockedFacts map[string]bool + // DisabledFacts overrides the config-derived disabled set when non-nil. + // When set it is used verbatim, bypassing the union below — this is how the + // CLI's --no-block clears everything (an empty non-nil map) and how a + // library consumer forces an explicit disabled set. + DisabledFacts map[string]bool + // ExtraDisabled are additional disable entries (fact or group names) unioned + // into the disabled set, carrying the CLI's --disable values. Names expand + // through the configured fact groups like every other disable source. + ExtraDisabled []string // ConfigLoaded carries an already parsed config from internal/app. Library // engines leave it false so ConfigFile/SystemDefaults are parsed fresh on // every Discover. @@ -87,7 +94,8 @@ 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.DisabledFacts = cloneBoolMap(cfg.DisabledFacts) + cfg.ExtraDisabled = slices.Clone(cfg.ExtraDisabled) cfg.DefaultExternalDirs = slices.Clone(cfg.DefaultExternalDirs) cfg.Config = cloneConfig(cfg.Config) cfg.Facts = slices.Clone(cfg.Facts) @@ -107,6 +115,41 @@ func NewEngine(cfg EngineConfig) (*Engine, error) { return &Engine{cfg: cfg, logger: logger, onceSeen: map[string]bool{}}, nil } +// diagnoseAmbientDisabledQueries emits a one-line warn diagnostic for each +// explicitly-queried fact suppressed by an ambient (FACTS_DISABLE or config) +// disable source. The disabled set is applied before query projection, so a +// silently-empty result is otherwise undiagnosable; --disable entries are +// excluded by planDiscovery so a disable on the same command line stays quiet. +func diagnoseAmbientDisabledQueries(logger *slog.Logger, queries []string, ambient map[string]string) { + if logger == nil || len(ambient) == 0 { + return + } + for _, query := range queries { + name := strings.ToLower(query) + if source, ok := ambientDisableSource(name, ambient); ok { + logger.Warn(fmt.Sprintf("fact %q is disabled by %s", name, source)) + } + } +} + +// 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`. +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] + } +} + // warnOnce emits message at warn level the first time it is seen on this // Engine, within and across discoveries. func (e *Engine) warnOnce(message string) { @@ -139,6 +182,7 @@ func (e *Engine) Discover(ctx context.Context, queries ...string) (*Snapshot, er plan, planFailures := e.planDiscovery(s, queries) failures = append(failures, planFailures...) + diagnoseAmbientDisabledQueries(s.logger, plan.queries, plan.ambientDisabled) finish := func() (*Snapshot, error) { if err := ctx.Err(); err != nil { @@ -155,7 +199,7 @@ func (e *Engine) Discover(ctx context.Context, queries ...string) (*Snapshot, er loader := externalFactLoader{ s: s, dirs: plan.externalDirs, - blocked: plan.blockedFacts, + blocked: plan.disabledFacts, } if plan.loaderMode == externalFactLoaderCLI { loader.mode = plan.loaderMode @@ -205,10 +249,10 @@ func (e *Engine) Discover(ctx context.Context, queries ...string) (*Snapshot, er return finish() } - facts = CoreFacts(s) + facts = CoreFacts(s, plan.disabledFacts) facts = append(facts, registeredFacts...) facts = append(facts, externalFacts...) - facts = FilterBlockedFacts(facts, plan.blockedFacts) + facts = FilterDisabledFacts(facts, plan.disabledFacts) if len(plan.queries) > 0 { facts = NewProjection(facts, plan.includeTypedDotted).Select(plan.queries) diff --git a/internal/engine/engine_test.go b/internal/engine/engine_test.go index 1d813f56..f41cd7b1 100644 --- a/internal/engine/engine_test.go +++ b/internal/engine/engine_test.go @@ -43,7 +43,7 @@ func TestNewEngineFreezesConfigAndNormalizesFactNames(t *testing.T) { blocked := map[string]bool{"networking": true} defaultDirs := []string{"/default"} config := Config{ - Blocklist: []string{"ssh"}, + Disabled: []string{"ssh"}, ExternalDirs: []string{"/config"}, TTLs: []FactTTL{{Fact: "site_role", TTL: "30 days"}}, FactGroups: []FactGroup{{Name: "site", Facts: []string{"site_role"}}}, @@ -57,7 +57,7 @@ func TestNewEngineFreezesConfigAndNormalizesFactNames(t *testing.T) { eng, err := NewEngine(EngineConfig{ ExternalDirs: externalDirs, - BlockedFacts: blocked, + DisabledFacts: blocked, ConfigLoaded: true, Config: config, DefaultExternalDirsSet: true, @@ -71,7 +71,7 @@ func TestNewEngineFreezesConfigAndNormalizesFactNames(t *testing.T) { externalDirs[0] = "/mutated-explicit" blocked["networking"] = false defaultDirs[0] = "/mutated-default" - config.Blocklist[0] = "mutated-blocklist" + config.Disabled[0] = "mutated-blocklist" config.ExternalDirs[0] = "/mutated-config" config.TTLs[0] = FactTTL{Fact: "mutated", TTL: "1 second"} config.FactGroups[0].Facts[0] = "mutated_group_fact" @@ -80,14 +80,14 @@ func TestNewEngineFreezesConfigAndNormalizesFactNames(t *testing.T) { if got, want := eng.cfg.ExternalDirs, []string{"/explicit"}; !reflect.DeepEqual(got, want) { t.Fatalf("ExternalDirs = %#v, want %#v", got, want) } - if got := eng.cfg.BlockedFacts["networking"]; !got { - t.Fatalf("BlockedFacts[networking] = false, want frozen true") + if got := eng.cfg.DisabledFacts["networking"]; !got { + t.Fatalf("DisabledFacts[networking] = false, want frozen true") } if got, want := eng.cfg.DefaultExternalDirs, []string{"/default"}; !reflect.DeepEqual(got, want) { t.Fatalf("DefaultExternalDirs = %#v, want %#v", got, want) } - if got, want := eng.cfg.Config.Blocklist, []string{"ssh"}; !reflect.DeepEqual(got, want) { - t.Fatalf("Config.Blocklist = %#v, want %#v", got, want) + if got, want := eng.cfg.Config.Disabled, []string{"ssh"}; !reflect.DeepEqual(got, want) { + t.Fatalf("Config.Disabled = %#v, want %#v", got, want) } if got, want := eng.cfg.Config.ExternalDirs, []string{"/config"}; !reflect.DeepEqual(got, want) { t.Fatalf("Config.ExternalDirs = %#v, want %#v", got, want) @@ -127,7 +127,7 @@ func TestPlanDiscoveryMergesLoadedConfig(t *testing.T) { config := Config{ ExternalDirs: []string{"/config"}, NoExternalFacts: true, - Blocklist: []string{"site"}, + Disabled: []string{"site"}, TTLs: []FactTTL{{Fact: "site_role", TTL: "1 day"}}, FactGroups: []FactGroup{{Name: "site", Facts: []string{"site_role", "site_location"}}}, ForceDotResolution: true, @@ -163,8 +163,8 @@ func TestPlanDiscoveryMergesLoadedConfig(t *testing.T) { if got, want := plan.cacheGroups, []FactGroup{{Name: "site", Facts: []string{"site_role", "site_location"}}}; !reflect.DeepEqual(got, want) { t.Fatalf("cacheGroups = %#v, want %#v", got, want) } - if !plan.blockedFacts["site_role"] || !plan.blockedFacts["site_location"] { - t.Fatalf("blockedFacts = %#v, want configured group expanded", plan.blockedFacts) + if !plan.disabledFacts["site_role"] || !plan.disabledFacts["site_location"] { + t.Fatalf("disabledFacts = %#v, want configured group expanded", plan.disabledFacts) } if plan.loaderMode != externalFactLoaderCLI { t.Fatalf("loaderMode = %v, want CLI", plan.loaderMode) @@ -183,9 +183,9 @@ func TestPlanDiscoveryMergesLoadedConfig(t *testing.T) { func TestPlanDiscoveryPreservesExplicitInputsAndUsesDefaultDirs(t *testing.T) { eng, err := NewEngine(EngineConfig{ ExternalDirs: []string{"/explicit"}, - BlockedFacts: map[string]bool{"explicit": true}, + DisabledFacts: map[string]bool{"explicit": true}, ConfigLoaded: true, - Config: Config{ExternalDirs: []string{"/config"}, Blocklist: []string{"config"}}, + Config: Config{ExternalDirs: []string{"/config"}, Disabled: []string{"config"}}, SystemDefaults: true, DefaultExternalDirsSet: true, DefaultExternalDirs: []string{"/default"}, @@ -200,8 +200,8 @@ func TestPlanDiscoveryPreservesExplicitInputsAndUsesDefaultDirs(t *testing.T) { if got, want := plan.externalDirs, []string{"/explicit"}; !reflect.DeepEqual(got, want) { t.Fatalf("externalDirs = %#v, want explicit dirs %#v", got, want) } - if got, want := plan.blockedFacts, map[string]bool{"explicit": true}; !reflect.DeepEqual(got, want) { - t.Fatalf("blockedFacts = %#v, want explicit blocklist %#v", got, want) + if got, want := plan.disabledFacts, map[string]bool{"explicit": true}; !reflect.DeepEqual(got, want) { + t.Fatalf("disabledFacts = %#v, want explicit disabled %#v", got, want) } defaultEng, err := NewEngine(EngineConfig{ diff --git a/internal/engine/external.go b/internal/engine/external.go index aa6fd4f2..33d0aaca 100644 --- a/internal/engine/external.go +++ b/internal/engine/external.go @@ -29,6 +29,12 @@ var ErrExternalFactTooLarge = errors.New("external fact exceeds size limit") const externalFactResolutionEnv = "FACTER_EXTERNAL_FACTS_RUNNING" +// reservedDisableControlName is the resolved external-fact name reserved for the +// disabled-set control variable. Any FACTS_*/FACTER_* variable resolving to this +// name (FACTS_DISABLE, FACTSDISABLE, FACTER_DISABLE, FACTERDISABLE) feeds the +// disabled set instead of becoming an external fact named "disable". +const reservedDisableControlName = "disable" + var errExternalFactOpen = errors.New("open external fact") var externalFactCommandTimeout = 30 * time.Second var externalFactMaxBytes = 1 << 20 @@ -224,6 +230,11 @@ func loadExternalEnvFacts(env []string) ([]ResolvedFact, error) { continue } factName = strings.ToLower(factName) + if factName == reservedDisableControlName { + // FACTS_DISABLE / FACTER_DISABLE are reserved control keys that feed + // the disabled set; they never resolve as external facts. + continue + } if err := validateExternalString(factName); err != nil { return nil, fmt.Errorf("external fact name %q: %w", factName, err) } @@ -254,6 +265,73 @@ func loadExternalEnvFacts(env []string) ([]ResolvedFact, error) { return facts, nil } +// environmentDisabledFacts extracts the disabled-set entries from the reserved +// FACTS_DISABLE / FACTER_DISABLE control variables. The facts-native FACTS_* +// value wins over the facter-compatible FACTER_* value when both are set. +func environmentDisabledFacts(env []string) []string { + var nativeValue, compatValue string + var haveNative, haveCompat bool + for _, entry := range env { + name, value, ok := strings.Cut(entry, "=") + if !ok { + continue + } + factName, native, ok := environmentFactName(name) + if !ok || strings.ToLower(factName) != reservedDisableControlName { + continue + } + if native { + nativeValue, haveNative = value, true + } else { + compatValue, haveCompat = value, true + } + } + // Native-wins is presence-based, not value-based: an empty FACTS_DISABLE="" still + // wins over a non-empty FACTER_DISABLE (it disables nothing), matching how the + // FACTS_* external-fact value shadows FACTER_* regardless of content. + switch { + case haveNative: + return splitDisableList(nativeValue) + case haveCompat: + return splitDisableList(compatValue) + default: + return nil + } +} + +// SplitDisableList splits a comma-separated --disable value into trimmed, +// lowercased entries, dropping empties, with the same semantics as the +// FACTS_DISABLE environment variable. It is the exported seam internal/app uses +// to feed EngineConfig.ExtraDisabled. +func SplitDisableList(value string) []string { + return splitDisableList(value) +} + +// EnvironmentDisabledFacts extracts the disabled-set entries from the reserved +// FACTS_DISABLE / FACTER_DISABLE control variables in env (native wins). It is +// the exported seam internal/app uses to honor ambient disables in the +// facterversion fast path. +func EnvironmentDisabledFacts(env []string) []string { + return environmentDisabledFacts(env) +} + +// splitDisableList splits a comma-separated disable list into trimmed, +// lowercased entries, dropping empties. It is shared by the FACTS_DISABLE +// environment variable and the --disable CLI option. +func splitDisableList(value string) []string { + parts := strings.Split(value, ",") + out := make([]string, 0, len(parts)) + for _, part := range parts { + if part = strings.ToLower(strings.TrimSpace(part)); part != "" { + out = append(out, part) + } + } + if len(out) == 0 { + return nil + } + return out +} + // environmentFactName maps an environment variable name to an external fact // name. The facts-native FACTS_* prefix and the facter-compatible FACTER_* // prefix are accepted with identical semantics; native reports whether the diff --git a/internal/engine/external_test.go b/internal/engine/external_test.go index b6165cca..e9a0412b 100644 --- a/internal/engine/external_test.go +++ b/internal/engine/external_test.go @@ -552,6 +552,51 @@ func TestLoadExternalFacts_loadsFactsNativeEnvironmentFacts(t *testing.T) { } } +func TestLoadExternalEnvFacts_reservesDisableControlKey(t *testing.T) { + got, err := loadExternalEnvFacts([]string{"FACTS_DISABLE=packages,os", "FACTS_site_role=web"}) + if err != nil { + t.Fatal(err) + } + want := []ResolvedFact{{Name: "site_role", Value: "web", Type: "external"}} + if !reflect.DeepEqual(got, want) { + t.Fatalf("loadExternalEnvFacts() = %#v, want %#v (FACTS_DISABLE must be reserved, not a fact)", got, want) + } +} + +func TestLoadExternalEnvFacts_reservesDisableAcrossAllPrefixes(t *testing.T) { + for _, name := range []string{"FACTS_DISABLE", "FACTSDISABLE", "FACTER_DISABLE", "FACTERDISABLE"} { + got, err := loadExternalEnvFacts([]string{name + "=packages"}) + if err != nil { + t.Fatal(err) + } + if len(got) != 0 { + t.Fatalf("%s produced %#v, want no facts (reserved control key)", name, got) + } + } +} + +func TestEnvironmentDisabledFacts_extractsNativeWinningCommaList(t *testing.T) { + got := environmentDisabledFacts([]string{"FACTER_DISABLE=os", "FACTS_DISABLE=Packages, networking"}) + want := []string{"packages", "networking"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("environmentDisabledFacts() = %#v, want %#v (native wins, comma-split, trimmed, lowercased)", got, want) + } +} + +func TestEnvironmentDisabledFacts_compatWhenNoNative(t *testing.T) { + got := environmentDisabledFacts([]string{"FACTER_DISABLE=os,dmi"}) + want := []string{"os", "dmi"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("environmentDisabledFacts() = %#v, want %#v (facter compat when no native)", got, want) + } +} + +func TestEnvironmentDisabledFacts_absentReturnsNil(t *testing.T) { + if got := environmentDisabledFacts([]string{"FACTS_site_role=web"}); got != nil { + t.Fatalf("environmentDisabledFacts() = %#v, want nil when unset", got) + } +} + func TestLoadExternalFacts_factsNativeEnvironmentFactsWinCollisions(t *testing.T) { // Native wins regardless of environment ordering; names set through only // one prefix resolve from that prefix. diff --git a/internal/engine/fips_test.go b/internal/engine/fips_test.go index 2779a8c9..2f8aef24 100644 --- a/internal/engine/fips_test.go +++ b/internal/engine/fips_test.go @@ -10,7 +10,7 @@ import ( ) func TestCoreFacts_fipsEnabledOnlyOnLinuxAndWindows(t *testing.T) { - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) value, ok := collection["fips_enabled"] switch runtime.GOOS { diff --git a/internal/engine/groups.go b/internal/engine/groups.go index 2d7c69ee..dc469170 100644 --- a/internal/engine/groups.go +++ b/internal/engine/groups.go @@ -16,13 +16,17 @@ type FactGroup struct { } // BuiltinFactGroups returns the static fact group catalog for the Go port. +// Each group keeps only its structured root fact; the legacy flat aliases +// (memoryfree, hostname, operatingsystem, processorcount, …) were removed by +// 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", "memoryfree", "memoryfree_mb", "memorysize", "memorysize_mb"}}, - {Name: "networking", Facts: []string{"networking", "hostname", "ipaddress", "ipaddress6", "netmask", "domain", "fqdn"}}, - {Name: "operating system", Facts: []string{"os", "operatingsystem", "osfamily", "operatingsystemrelease", "architecture"}}, + {Name: "memory", Facts: []string{"memory"}}, + {Name: "networking", Facts: []string{"networking"}}, + {Name: "operating system", Facts: []string{"os"}}, {Name: "path", Facts: []string{"path"}}, - {Name: "processor", Facts: []string{"processors", "processorcount", "physicalprocessorcount", "hardwareisa"}}, + {Name: "processor", Facts: []string{"processors"}}, } } @@ -172,10 +176,10 @@ func ttlUnitScale(unit string) (multiplier, divisor int64, ok bool) { return 0, 0, false } -// BlocklistedFactsWithGroups expands config blocklist entries into concrete fact +// DisabledFactsWithGroups expands the disabled set into concrete fact // names using the built-in group catalog plus any configured groups. -func BlocklistedFactsWithGroups(entries []string, configured []FactGroup) map[string]bool { - blocked := make(map[string]bool) +func DisabledFactsWithGroups(entries []string, configured []FactGroup) map[string]bool { + disabled := make(map[string]bool) groups := MergeFactGroups(BuiltinFactGroups(), configured) for _, entry := range entries { matchedGroup := false @@ -185,48 +189,48 @@ func BlocklistedFactsWithGroups(entries []string, configured []FactGroup) map[st } matchedGroup = true for _, fact := range group.Facts { - blocked[fact] = true + disabled[fact] = true } } if !matchedGroup { - blocked[entry] = true + disabled[entry] = true } } - return blocked + return disabled } -// BlocklistedFactsForFiltering expands config blocklist entries for resolver filtering. -func BlocklistedFactsForFiltering(entries []string, configured []FactGroup) map[string]bool { - return BlocklistedFactsWithGroups(entries, configured) +// DisabledFactsForFiltering expands the disabled set for resolver filtering. +func DisabledFactsForFiltering(entries []string, configured []FactGroup) map[string]bool { + return DisabledFactsWithGroups(entries, configured) } -// FilterBlockedFacts removes facts whose root name is blocklisted. -func FilterBlockedFacts(facts []ResolvedFact, blocked map[string]bool) []ResolvedFact { - if len(blocked) == 0 { +// FilterDisabledFacts removes facts whose root name is disabled. +func FilterDisabledFacts(facts []ResolvedFact, disabled map[string]bool) []ResolvedFact { + if len(disabled) == 0 { return facts } filtered := make([]ResolvedFact, 0, len(facts)) for _, fact := range facts { root, _, _ := strings.Cut(fact.Name, ".") - if blocked[fact.Name] || blocked[root] { + if disabled[fact.Name] || disabled[root] { continue } - fact.Value = pruneBlockedDescendants(fact.Name, fact.Value, blocked) + fact.Value = pruneDisabledDescendants(fact.Name, fact.Value, disabled) filtered = append(filtered, fact) } return filtered } -func pruneBlockedDescendants(name string, value any, blocked map[string]bool) any { +func pruneDisabledDescendants(name string, value any, disabled map[string]bool) any { var pruned any - for blockedName := range blocked { - if !strings.HasPrefix(blockedName, name+".") { + for disabledName := range disabled { + if !strings.HasPrefix(disabledName, name+".") { continue } if pruned == nil { pruned = deepCopyValue(value) } - pruned = pruneDottedValue(pruned, strings.Split(strings.TrimPrefix(blockedName, name+"."), ".")) + pruned = pruneDottedValue(pruned, strings.Split(strings.TrimPrefix(disabledName, name+"."), ".")) } if pruned == nil { return value diff --git a/internal/engine/identity_test.go b/internal/engine/identity_test.go index 81b7270c..a35d1d2b 100644 --- a/internal/engine/identity_test.go +++ b/internal/engine/identity_test.go @@ -186,7 +186,7 @@ func TestCoreFacts_includeMacOSReleaseKernelHardwareAndIdentity(t *testing.T) { if runtime.GOOS != "darwin" { t.Skipf("macOS host fact integration runs only on darwin, not %s", runtime.GOOS) } - collection := Collection(CoreFacts(NewSession())) + collection := Collection(CoreFacts(NewSession(), nil)) osFact, ok := collection["os"].(map[string]any) if !ok { t.Fatalf("os = %#v, want map", collection["os"]) diff --git a/internal/engine/memory_test.go b/internal/engine/memory_test.go index 840617f2..b84fa20a 100644 --- a/internal/engine/memory_test.go +++ b/internal/engine/memory_test.go @@ -93,7 +93,7 @@ func TestCoreFacts_includeRealSystemMemoryTotal(t *testing.T) { t.Skipf("memory total resolution is not implemented on %s", runtime.GOOS) } - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) memory, ok := collection["memory"].(map[string]any) if !ok { t.Fatalf("memory fact = %#v, want map", collection["memory"]) @@ -116,7 +116,7 @@ func TestCoreFacts_includeMemorySystemTotal(t *testing.T) { t.Skipf("memory total resolution is not implemented on %s", runtime.GOOS) } - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) memory, ok := collection["memory"].(map[string]any) if !ok { t.Fatalf("memory fact = %#v, want map", collection["memory"]) @@ -132,7 +132,7 @@ func TestCoreFacts_includeMemorySystemTotal(t *testing.T) { } func TestCoreFacts_includeMemoryUsageAndSwap(t *testing.T) { - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) memory, ok := collection["memory"].(map[string]any) if !ok { t.Fatalf("memory fact = %#v, want map", collection["memory"]) diff --git a/internal/engine/networking_test.go b/internal/engine/networking_test.go index 5ec549ab..c70c6c7e 100644 --- a/internal/engine/networking_test.go +++ b/internal/engine/networking_test.go @@ -2142,7 +2142,7 @@ func TestLinuxDHCPServerFallsBackToDHCPCDCommand(t *testing.T) { } func TestCoreFacts_networkingIncludesIP6(t *testing.T) { - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) networking, ok := collection["networking"].(map[string]any) if !ok { t.Fatalf("networking fact = %#v, want map", collection["networking"]) @@ -2154,7 +2154,7 @@ func TestCoreFacts_networkingIncludesIP6(t *testing.T) { } func TestCoreFacts_networkingIncludesPrimaryIPv4Binding(t *testing.T) { - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) networking, ok := collection["networking"].(map[string]any) if !ok { t.Fatalf("networking fact = %#v, want map", collection["networking"]) @@ -2169,7 +2169,7 @@ func TestCoreFacts_networkingIncludesPrimaryIPv4Binding(t *testing.T) { } func TestCoreFacts_networkingIncludesPrimaryIPv6Binding(t *testing.T) { - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) networking, ok := collection["networking"].(map[string]any) if !ok { t.Fatalf("networking fact = %#v, want map", collection["networking"]) @@ -2479,7 +2479,7 @@ func TestInterfaceBindingIPv6NetmaskMatchesRubyBuildBinding(t *testing.T) { } func TestCoreFacts_networkingIncludesPrimaryMTU(t *testing.T) { - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) networking, ok := collection["networking"].(map[string]any) if !ok { t.Fatalf("networking fact = %#v, want map", collection["networking"]) @@ -2502,7 +2502,7 @@ func TestCoreFacts_networkingIncludesPrimaryMTU(t *testing.T) { } func TestCoreFacts_includeNetworkingInterfaces(t *testing.T) { - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) networking, ok := collection["networking"].(map[string]any) if !ok { t.Fatalf("networking fact = %#v, want map", collection["networking"]) diff --git a/internal/engine/os_test.go b/internal/engine/os_test.go index 5365fd93..aa77c43a 100644 --- a/internal/engine/os_test.go +++ b/internal/engine/os_test.go @@ -852,7 +852,7 @@ func TestCurrentOSReleaseIllumosFallsBackToKernelRelease(t *testing.T) { } func TestCoreFacts_includeOSHardware(t *testing.T) { - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) osFact, ok := collection["os"].(map[string]any) if !ok { t.Fatalf("os fact = %#v, want map", collection["os"]) diff --git a/internal/engine/processors_test.go b/internal/engine/processors_test.go index 86d9e3a2..9349781f 100644 --- a/internal/engine/processors_test.go +++ b/internal/engine/processors_test.go @@ -591,7 +591,7 @@ func TestCurrentProcessorISAWindowsFallsBackWhenWMIHasNoISA(t *testing.T) { } func TestCoreFacts_processorSpeedOmittedWhenProbeYieldsNothing(t *testing.T) { - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) processors, ok := collection["processors"].(map[string]any) if !ok { t.Fatalf("processors fact = %#v, want map", collection["processors"]) @@ -667,7 +667,7 @@ func TestParseDarwinProcessorsKeepsRubyCoreCountAsCoresPerSocket(t *testing.T) { } func TestCoreFacts_includeProcessorTopology(t *testing.T) { - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) processors, ok := collection["processors"].(map[string]any) if !ok { t.Fatalf("processors fact = %#v, want map", collection["processors"]) diff --git a/internal/engine/session.go b/internal/engine/session.go index 0a1479ac..e0119bee 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -28,6 +28,7 @@ type hostOS interface { lstat(string) (os.FileInfo, error) glob(string) ([]string, error) statMountpoint(string) (mountStat, bool) + environ() []string } type osHost struct{} @@ -171,6 +172,10 @@ func (osHost) statMountpoint(path string) (mountStat, bool) { return statMountpoint(path) } +func (osHost) environ() []string { + return os.Environ() +} + // Session carries the state of one resolution run: memoized host probes and // resolution-scoped caches. Resolvers share a Session so facts derived from // the same probe agree within a run; a fresh Session re-reads the host, which @@ -184,8 +189,14 @@ type Session struct { logger *slog.Logger coreFacts struct { - mu sync.Mutex - facts []ResolvedFact + mu sync.Mutex + // built distinguishes "never built" from "built with an empty disabled + // set"; fingerprint identifies the disabled set the memo was built for so + // a later call with a different set rebuilds instead of returning stale + // gating. + built bool + fingerprint string + facts []ResolvedFact } augeasVersion memo[string] diff --git a/internal/engine/session_test.go b/internal/engine/session_test.go index 1d842464..86d2fae3 100644 --- a/internal/engine/session_test.go +++ b/internal/engine/session_test.go @@ -28,6 +28,8 @@ type fakeHostOS struct { lstats map[string]os.FileInfo globs map[string][]string mountStats map[string]mountStat + environEntries []string + readFileCalls []string readDirCalls []string globCalls []string statMountpointCalls []string @@ -63,6 +65,7 @@ func (h *fakeHostOS) goos() string { } func (h *fakeHostOS) readFile(path string) ([]byte, error) { + h.readFileCalls = append(h.readFileCalls, fakeHostPath(path)) data, ok := h.files[fakeHostPath(path)] if !ok { return nil, os.ErrNotExist @@ -110,6 +113,10 @@ func (h *fakeHostOS) statMountpoint(path string) (mountStat, bool) { return stat, ok } +func (h *fakeHostOS) environ() []string { + return h.environEntries +} + func fakeHostPath(path string) string { return filepath.ToSlash(path) } diff --git a/internal/engine/uptime_test.go b/internal/engine/uptime_test.go index ce0be304..de295d4b 100644 --- a/internal/engine/uptime_test.go +++ b/internal/engine/uptime_test.go @@ -9,7 +9,7 @@ import ( ) func TestCoreFacts_includeSystemUptime(t *testing.T) { - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) systemUptime, ok := collection["system_uptime"].(map[string]any) if !ok { t.Fatalf("system_uptime fact = %#v, want map", collection["system_uptime"]) @@ -490,7 +490,7 @@ func TestCoreFacts_includeLoadAverages(t *testing.T) { t.Skipf("load averages resolution is not implemented on %s", runtime.GOOS) } - collection := Collection(CoreFacts(testSession)) + collection := Collection(CoreFacts(testSession, nil)) loadAverages, ok := collection["load_averages"].(map[string]any) if !ok { t.Fatalf("load_averages fact = %#v, want map", collection["load_averages"]) diff --git a/man/man8/facts.8 b/man/man8/facts.8 index 2ed8674e..b924b561 100644 --- a/man/man8/facts.8 +++ b/man/man8/facts.8 @@ -48,6 +48,12 @@ The location of the config file\. Enable debug output\. . .TP +\fB\-\-disable\fR: +. +.IP +Disable facts or fact groups (comma\-separated, repeatable)\. Standalone resolvers are skipped; others are pruned from output, even when queried\. Unions with the \fBFACTS_DISABLE\fR environment variable and the config file disable list; \fB\-\-no\-block\fR clears the whole set\. +. +.TP \fB\-\-external\-dir\fR: . .IP diff --git a/openspec/changes/add-fact-disable-controls/design.md b/openspec/changes/add-fact-disable-controls/design.md new file mode 100644 index 00000000..cc2a42aa --- /dev/null +++ b/openspec/changes/add-fact-disable-controls/design.md @@ -0,0 +1,18 @@ +## Decisions + +- **All-on, disable-only (ADR-0015).** No opt-in tier, so no per-fact visibility metadata is introduced. +- **One disabled set, unioned** from `--disable`, `FACTS_DISABLE`, and the `disable` config key. `disable` is the facts-native spelling; the Facter `blocklist` key is its compatibility alias (native wins on collision) and is never removed, because `facter.conf` is part of the binding input contract. +- **Resolution-gating by class.** Standalone-resolver facts skip their resolver when disabled (`packages`). Multi-output category resolvers — `osCoreFacts` → `os`/`kernel`/`filesystems`, `disksCoreFacts` → `disks`/`partitions`/`mountpoints`/`zfs`/`zpool`, `uptimeCoreFacts` → `load_averages`/`system_uptime` — skip only when *all* their outputs are disabled, otherwise resolve-then-prune. Inline facts (`facterversion`, `is_virtual`/`virtual`, `path`) and sub-facts are resolve-then-prune. Gating is per shared **probe**: a memoized probe is skipped only when all its consumers are disabled (`cachedIdentity` feeds `identity` and the SSH privilege gate; DMI feeds `gce`/virtualization). `packages` shares no probe, so it gates cleanly. +- **`--no-block` is the master override** (clears the set). **Disable beats query**, matching the engine's existing order (the disabled set is applied before query projection). An explicitly-queried fact disabled by a non-command-line source emits a one-line stderr diagnostic with empty stdout, so a silently-empty result is diagnosable. +- **`FACTS_DISABLE` reserved by resolved name.** `environmentFactName` lowercases and strips any of `facts_`/`facts`/`facter_`/`facter`, so the loader drops any variable whose resolved fact name is `disable` (covering `FACTS_DISABLE`, `FACTSDISABLE`, `FACTER_DISABLE`, `FACTERDISABLE` uniformly) and routes it to the disabled set. Cost: no external environment fact may be named `disable`. +- **Cache invariant.** Subtract the disabled set before the cache is consulted and before any cache write; never persist a pruned sub-fact into a cached group. + +## Implementation Notes + +- `internal/engine/config.go`: parse a `disable` config key into the disabled set; keep `blocklist` feeding the same set; native wins on collision. +- `internal/cli/options.go` + `internal/app/app.go`: add `--disable` (valued, comma-split, repeatable) to the shared option vocabulary, feed the disabled set, and ensure `--no-block` clears the unioned set; document it for the CLI-option-contract tests. +- `internal/engine/external.go` (`environmentFactName` / loader): drop any env var whose resolved fact name is `disable` and route it to the disabled set instead of creating a fact. +- `internal/engine/discovery_plan.go`: union CLI + env + config into the disabled set. +- `internal/engine/core.go` / `buildCoreFacts`: introduce a fact/group → resolver mapping and pass the disabled set in; skip a resolver only when all its top-level outputs are disabled; gate per shared probe; standalone resolvers (`packages`) skip cleanly. +- `internal/engine/engine.go`: keep disabled-set subtraction before cache resolution and `Select`; emit the stderr diagnostic for an explicitly-queried-but-disabled fact. +- Re-audit `BuiltinFactGroups` to drop the legacy flat names removed by ADR-0007 from group membership (no-ops today), so `--disable ` relies on the structured root entry. diff --git a/openspec/changes/add-fact-disable-controls/proposal.md b/openspec/changes/add-fact-disable-controls/proposal.md new file mode 100644 index 00000000..0768b0e2 --- /dev/null +++ b/openspec/changes/add-fact-disable-controls/proposal.md @@ -0,0 +1,19 @@ +## Why + +Facts has only an opt-out blocklist that filters already-resolved facts out of output (Facter `blocklist` parity). It has no CLI or environment control, no facts-native spelling, and it still resolves every fact before discarding it — so a voluminous fact like `packages` (ADR-0014) would pay its full collection cost on every run even when unwanted. ADR-0015 decides the model; this change implements it. + +## What Changes + +- Every fact is on by default; a fact is removed only by disabling it. There is no opt-in, allowlist, or default-off tier. +- The disabled set is the union of three inputs: `--disable a,b,c` (CLI), `FACTS_DISABLE=a,b,c` (environment), and the `facts.conf` `disable` key. `disable` is the facts-native term across all three; the Facter `blocklist` key is retained as its compatibility alias (native wins on collision) and is never removed. +- A disable target is a fact name or a fact group, reusing the existing group expansion. +- Disabling is **resolution-gated**: a fact produced by its own resolver is not resolved when disabled (e.g. `packages`); a fact sharing a multi-output category resolver is gated only when *all* that resolver's outputs are disabled, otherwise resolve-then-prune; a disabled sub-fact is resolve-then-prune. Gating is per shared probe. +- `--no-block` clears the entire disabled set for a run. Disable beats query; a fact disabled by environment/config (not the same command line) that is explicitly queried emits a one-line stderr diagnostic with empty stdout. +- `FACTS_DISABLE` is reserved by its resolved fact name `disable`, so it is not ingested as an external environment fact under any accepted prefix. +- The disabled set is subtracted before the cache is consulted and before any cache write. + +## Impact + +- **Behavior**: new `--disable` / `FACTS_DISABLE` / `disable` controls; disabling now skips resolution for standalone-resolver facts; a Facts-native `packages` group diverges the Facter-mirrored `--list-block-groups`/`--list-cache-groups` output. +- **Compatibility**: additive — the Facter `blocklist` config key keeps working unchanged, and existing `--no-block` semantics are preserved. +- **Out of scope**: any opt-in / default-off / allowlist tier and per-fact "optional" metadata; the `packages` fact itself (separate change `add-packages-fact`); splitting existing multi-output category resolvers into single-output resolvers (they stay resolve-then-prune until split). diff --git a/openspec/changes/add-fact-disable-controls/specs/fact-disable-controls/spec.md b/openspec/changes/add-fact-disable-controls/specs/fact-disable-controls/spec.md new file mode 100644 index 00000000..b6f58e07 --- /dev/null +++ b/openspec/changes/add-fact-disable-controls/specs/fact-disable-controls/spec.md @@ -0,0 +1,80 @@ +## ADDED 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 diff --git a/openspec/changes/add-fact-disable-controls/specs/facts-cli-option-contract/spec.md b/openspec/changes/add-fact-disable-controls/specs/facts-cli-option-contract/spec.md new file mode 100644 index 00000000..32690f95 --- /dev/null +++ b/openspec/changes/add-fact-disable-controls/specs/facts-cli-option-contract/spec.md @@ -0,0 +1,16 @@ +## ADDED Requirements + +### 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 diff --git a/openspec/changes/add-fact-disable-controls/specs/facts-native-input-surface/spec.md b/openspec/changes/add-fact-disable-controls/specs/facts-native-input-surface/spec.md new file mode 100644 index 00000000..d66d08f6 --- /dev/null +++ b/openspec/changes/add-fact-disable-controls/specs/facts-native-input-surface/spec.md @@ -0,0 +1,27 @@ +## ADDED Requirements + +### 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 diff --git a/openspec/changes/add-fact-disable-controls/tasks.md b/openspec/changes/add-fact-disable-controls/tasks.md new file mode 100644 index 00000000..82f1f2f6 --- /dev/null +++ b/openspec/changes/add-fact-disable-controls/tasks.md @@ -0,0 +1,29 @@ +## 1. Tests First + +- [x] 1.1 Test all facts resolve by default with an empty disabled set, `packages` included. +- [x] 1.2 Test the disabled set is the union of `--disable`, `FACTS_DISABLE`, and config `disable`/`blocklist`, accepting fact names and group names, with `disable` winning over the `blocklist` alias. +- [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.) + +## 2. Implementation + +- [x] 2.1 Parse the `disable` config key (native) alongside `blocklist` (compat) into one disabled set; native wins on collision (`config.go`). +- [x] 2.2 Add the `--disable` option (valued, comma-split, repeatable) to the shared option vocabulary, feed the disabled set, and make `--no-block` clear it (`cli/options.go`, `app.go`). +- [x] 2.3 Reserve the resolved env name `disable` in the env-fact loader (covering `facts_`/`facts`/`facter_`/`facter`), routing the variable into the disabled set instead of creating a fact (`external.go`). +- [x] 2.4 Union the three sources into the disabled set (`discovery_plan.go`). +- [x] 2.5 Add a fact/group → resolver mapping and pass the disabled set into `buildCoreFacts`; skip a resolver only when all its top-level outputs are disabled; gate per shared probe (`core.go`). +- [x] 2.6 Emit the stderr diagnostic for an explicitly-queried fact disabled by env/config; keep disabled-set subtraction before cache resolution and `Select` (`engine.go`). +- [x] 2.7 Re-audit `BuiltinFactGroups` to drop legacy flat names removed by ADR-0007 from group membership. + +## 3. Docs + +- [x] 3.1 Document `--disable`, `FACTS_DISABLE`, and the `disable` config key (with `blocklist` compat) in help, man, and the configuration-compatibility document. +- [x] 3.2 Update README and CHANGELOG. + +## 4. Verification + +- [x] 4.1 Run `go test ./...` and `go vet ./...`. +- [x] 4.2 Run the CLI option-contract tests (help/man drift on `--disable`). +- [x] 4.3 Run `openspec validate add-fact-disable-controls --strict`. diff --git a/openspec/changes/add-packages-fact/design.md b/openspec/changes/add-packages-fact/design.md new file mode 100644 index 00000000..804fc70e --- /dev/null +++ b/openspec/changes/add-packages-fact/design.md @@ -0,0 +1,19 @@ +## Decisions + +- **Source-namespaced lists of records (ADR-0014).** `packages.` is an array of record maps; `name` is a field, not a key. A name-keyed map is unsound: name is not unique on `dpkg` (multiarch `libc6:amd64` + `:i386`), `rpm` (multilib and install-only multiversion kernels — same name and arch), `openbsd_pkg` (parallel `autoconf-2.69p3` + `2.71p3`), `homebrew` (formula vs cask `docker`), `registry` (x86/x64 share DisplayName + DisplayVersion), `flatpak` (branches), and `nix` (versions coexist). Package names also contain dots (`python3.11`, `libdb5.3`) that fight the engine's dot-path addressing. +- **Key on the database, never the frontend/format.** apt/aptitude → `dpkg`; dnf/yum/zypper → `rpm`. `pkg` is shared by FreeBSD and DragonFly (one pkgng SQLite database). illumos/Solaris IPS is keyed `ips`, not `pkg`, because the IPS `pkg(1)` CLI only coincidentally shares the FreeBSD tool name and keying on the tool would clobber two different databases. +- **Per-source identity fields complete uniqueness.** `architecture` (dpkg, rpm, apk), `type` + `tap` (homebrew), `branch` + `architecture` (flatpak), `store_path` (nix), `bundle_id` + `path` (macOS `apps`), and for Windows `registry` the uninstall subkey / MSI `product_code` plus an `architecture` derived from the native hive vs `WOW6432Node`. +- **`version` is verbatim and not decomposed.** Epoch and release are kept as the manager renders them. The rpm reader queries `%{EPOCH}:%{VERSION}-%{RELEASE}` because a bare `rpm -qa` omits the epoch that can separate otherwise-identical install-only kernels. No `install_date` in v1; if added later it is spelled `install_date` (per `dmi.bios.release_date`). +- **nix means the installed profile set** (`/nix/var/nix/profiles/default` and the NixOS system profile), never the whole `/nix/store`, which holds every build dependency and would massively over-report. +- **macOS is server-first.** `receipts` (the installer(8)/PackageKit `.pkg` database, read from `/var/db/receipts/*.plist`) is primary — MDM/Jamf/Munki deploy `.pkg`, so it is the dpkg/rpm analogue on a managed server. `apps` (the `.app` bundle inventory) is secondary and never merged into `receipts`. `homebrew` is optional and auto-detected (emitted only when a Cellar prefix exists). `pkgutil --pkg-info`, `system_profiler`, and `mas` are rejected as collection mechanisms (spawn-per-item or multi-second). +- **Windows.** `registry` reads both HKLM uninstall hives (native + `WOW6432Node`); `appx` reads the system-provisioned set (`Get-AppxProvisionedPackage`) plus the collector context. `winget`/`choco`/`scoop` are out of v1 (winget re-surfaces ARP; the others are deferred secondaries). +- **Execution-context boundary.** Report system-global databases plus the collector's own context; do not drop privileges or enumerate other users' homes/hives. `homebrew`, nix per-user profiles, per-user `flatpak`, Windows `HKCU`, and per-user `appx` are therefore under-reported; this is documented, and a future `user` field is the additive path to all-users enumeration. +- **One cheap read per source.** Parse the on-disk database directly where world-readable, or one batch query; never spawn a process per package, never hit the network (IPS reads the local catalog, no refresh). +- **Visibility deferred.** `packages` registers as a fact group so a future fact enable/disable mechanism can toggle it; the default direction (in the default dump vs opt-in) is not decided here. + +## Implementation Notes + +- One per-category resolver module `internal/engine/packages.go` exposing `packagesCoreFacts(s *Session) []ResolvedFact`, composed into `buildCoreFacts` (ADR-0010). Per-source logic is pure `parse*` functions tested cross-platform via fixtures and injected probes; **no GOOS-suffixed files** (`packages_windows.go` etc.), preserving the ADR-0010 cross-platform parse-test seam. +- Per-source readers: `dpkg` parse `/var/lib/dpkg/status` (keep only `Status: install ok installed`); `rpm` one `rpm -qa --qf '%{NAME}|%{EPOCH}:%{VERSION}-%{RELEASE}|%{ARCH}\n'` (filter `gpg-pubkey`); `pacman` scan `/var/lib/pacman/local/*/desc`; `apk` parse `/lib/apk/db/installed`; `snap` read `/var/lib/snapd/state.json` or `snap list`; `flatpak` enumerate the system installation (and the collector-context user installation); `pkg` query `local.sqlite`; `openbsd_pkg` read the `/var/db/pkg` subdirectory names; `pkgsrc` discover `PKG_DBDIR` (do not hardcode); `ips` `pkg list -H` against the local image (no refresh); `nix` read the profile manifest; `receipts` glob `/var/db/receipts/*.plist`; `apps` read `*/Contents/Info.plist` under `/Applications`, `/System/Applications`; `homebrew` walk `Cellar`/`Caskroom`; `registry` read both HKLM uninstall hives; `appx` `Get-AppxProvisionedPackage`. +- Each source is omitted entirely when its database is absent (ADR omit-not-applicable); Plan 9 emits no `packages` subtree. +- Update `docs/schema/facts.yaml`, the generated supported-facts pages, and the per-target validation gates in the same change so docs and live gates fail on drift. diff --git a/openspec/changes/add-packages-fact/proposal.md b/openspec/changes/add-packages-fact/proposal.md new file mode 100644 index 00000000..a9356850 --- /dev/null +++ b/openspec/changes/add-packages-fact/proposal.md @@ -0,0 +1,43 @@ +## Why + +Facts has no inventory of installed software, and Ruby Facter never shipped one, so an operator doing compliance, drift detection, or asking "is package X installed, at what version?" has to shell out per platform. This change adds a `packages` fact — a Facts-native extension governed by ADR-0014 — that reports installed software across every supported target, namespaced by the installed-package database. + +## What Changes + +- Add a `packages` fact: `packages.` is an **array of package records** `{name, version, …per-source identity fields}`. Name is a field, never a map key, and records are never merged or deduped across sources. +- A **source is the installed-package database**, never a frontend and never an artifact format: `dpkg` (apt/aptitude collapse here), `rpm` (dnf/yum/zypper), `pacman`, `apk`, `snap`, `flatpak`, `pkg` (FreeBSD and DragonFly — one pkgng database), `openbsd_pkg`, `pkgsrc` (NetBSD; illumos secondary), `ips` (illumos/Solaris — keyed `ips`, not `pkg`), `nix` (the installed profile set, not the whole store), macOS `receipts` + `apps` + optional `homebrew`, and Windows `registry` + `appx`. Plan 9 emits nothing — it has no package database. +- Per-record contract: `name` and `version` (the package manager's verbatim native version string) are always present; per-source identity fields (`architecture`, `type`/`tap`, `branch`, `store_path`, `bundle_id`/`path`, Windows `product_code`) are present where they are needed to keep two genuinely different installs distinct. +- Collection is **one cheap read per source** — parse the on-disk database where it is world-readable, or one batch query — never a process per package and never the network. +- The fact reports system-global databases plus whatever the collector's own execution context can read; it does not drop privileges or walk other users' homes. +- Document `packages.*` in `docs/schema/facts.yaml` under ADR-0011 canonical spelling and in the generated supported-facts pages. +- Register `packages` as a fact group; it is on by default like every other fact (ADR-0015) and removed only by disabling it (`--disable packages`, `FACTS_DISABLE=packages`, or the `facts.conf` `disable` key). The fact enable/disable mechanism itself is a separate change. + +## Target Shape + +```json +{ + "packages": { + "dpkg": [ + {"name": "libc6", "version": "2.38-1ubuntu6", "architecture": "amd64"}, + {"name": "libc6", "version": "2.38-1ubuntu6", "architecture": "i386"} + ], + "snap": [{"name": "firefox", "version": "126.0"}], + "rpm": [ + {"name": "kernel-core", "version": "5.14.0-427.el9", "architecture": "x86_64"}, + {"name": "kernel-core", "version": "5.14.0-362.el9", "architecture": "x86_64"} + ], + "homebrew": [ + {"name": "docker", "version": "27.0.3", "type": "formula", "tap": "homebrew/core"}, + {"name": "docker", "version": "4.31.0", "type": "cask", "tap": "homebrew/cask"} + ], + "nix": [{"name": "hello", "version": "2.12.1", "store_path": "/nix/store/abc-hello-2.12.1"}] + } +} +``` + +## Impact + +- **Behavior**: a new `packages` subtree on supported platforms; absent on Plan 9 and absent for any source whose database is not present. +- **Schema/docs**: `docs/schema/facts.yaml` gains `packages.*`; supported-facts pages list the per-source record shapes. +- **Compatibility**: additive — no existing fact changes shape. +- **Out of scope**: language/dev managers (pip, npm, gem, cargo); MacPorts and Chocolatey (deferred auto-detected secondaries); the fact enable/disable mechanism itself (a separate change — `packages` is default-on per ADR-0015); a separate cross-platform `applications` fact; multi-user enumeration of other users' per-user installs. diff --git a/openspec/changes/add-packages-fact/specs/facts-schema/spec.md b/openspec/changes/add-packages-fact/specs/facts-schema/spec.md new file mode 100644 index 00000000..9155f983 --- /dev/null +++ b/openspec/changes/add-packages-fact/specs/facts-schema/spec.md @@ -0,0 +1,56 @@ +## ADDED Requirements + +### Requirement: Installed packages are reported as source-namespaced record lists + +Facts SHALL document the `packages` fact as a map from package-source key to an array of package records, where each record is one installed package identified by its fields, and records are never merged across sources. + +#### Scenario: packages schema is source-namespaced arrays + +- **WHEN** a contributor reads `docs/schema/facts.yaml` +- **THEN** `packages` MUST be documented as a map keyed by source +- **AND** each `packages.` MUST have type `array` +- **AND** each array element MUST be a package record (a map), never a bare scalar +- **AND** the documented source keys MUST be `dpkg`, `rpm`, `pacman`, `apk`, `snap`, `flatpak`, `pkg`, `openbsd_pkg`, `pkgsrc`, `ips`, `nix`, `receipts`, `apps`, `homebrew`, `registry`, and `appx` + +#### Scenario: package records carry name and version + +- **WHEN** a package record is documented +- **THEN** it MUST include `name` and `version` as always-present strings +- **AND** `version` MUST be the package manager's verbatim native version string, not decomposed into separate epoch or release fields + +#### Scenario: per-source identity fields are documented + +- **WHEN** a source needs more than `name` and `version` to keep two installs distinct +- **THEN** the schema MUST document its identity fields: `architecture` for `dpkg`/`rpm`/`apk`, `type` and `tap` for `homebrew`, `branch` and `architecture` for `flatpak`, `store_path` for `nix`, `bundle_id` and `path` for `apps`, and `product_code` and `architecture` for `registry` + +### Requirement: Package sources are keyed by the installed-package database + +Facts SHALL key each package source by its installed-package database, never by a package-manager frontend or an artifact file format. + +#### Scenario: frontends collapse to their database + +- **WHEN** packages are read on a host managed with apt/aptitude or dnf/yum/zypper +- **THEN** they MUST be reported under `packages.dpkg` or `packages.rpm` respectively +- **AND** `packages.apt`, `packages.deb`, `packages.dnf`, `packages.yum`, and `packages.zypper` MUST NOT be documented as sources + +#### Scenario: IPS is keyed ips, not pkg + +- **WHEN** packages are read on illumos or Solaris +- **THEN** they MUST be reported under `packages.ips` +- **AND** `packages.pkg` MUST refer only to the FreeBSD and DragonFly pkgng database + +### Requirement: Package records preserve every installed instance + +Facts SHALL keep one record per installed package instance so that same-named installs are never collapsed. + +#### Scenario: colliding installs are all kept + +- **WHEN** a host has two installs sharing a name (dpkg multiarch, rpm multiversion kernels, homebrew formula and cask, flatpak branches, Windows x86 and x64) +- **THEN** `packages.` MUST contain a distinct record for each +- **AND** the records MUST differ in at least one per-source identity field + +#### Scenario: absent sources are omitted and never merged + +- **WHEN** a package source's database is not present on the host +- **THEN** its `packages.` key MUST be absent, not an empty array +- **AND** records from different sources MUST NOT be merged or deduplicated into one list diff --git a/openspec/changes/add-packages-fact/specs/go-port-supported-platform-facts/spec.md b/openspec/changes/add-packages-fact/specs/go-port-supported-platform-facts/spec.md new file mode 100644 index 00000000..a26b8d0d --- /dev/null +++ b/openspec/changes/add-packages-fact/specs/go-port-supported-platform-facts/spec.md @@ -0,0 +1,57 @@ +## ADDED Requirements + +### Requirement: Supported targets report their native installed-package sources + +Facts SHALL resolve the `packages` fact on each supported release target using the package source(s) native to that target. + +#### Scenario: Linux targets report their package database and cross-distro sources + +- **WHEN** packages are discovered on a supported Linux target +- **THEN** Debian and Ubuntu MUST report `packages.dpkg` +- **AND** the RHEL family and SUSE MUST report `packages.rpm` +- **AND** Arch MUST report `packages.pacman` and Alpine MUST report `packages.apk` +- **AND** `packages.snap` and `packages.flatpak` MUST be reported wherever those system databases are present +- **AND** `packages.nix` MUST be reported wherever a Nix profile is present + +#### Scenario: BSD and illumos targets report their package database + +- **WHEN** packages are discovered on a supported BSD or illumos target +- **THEN** FreeBSD and DragonFly MUST report `packages.pkg` +- **AND** OpenBSD MUST report `packages.openbsd_pkg` +- **AND** NetBSD MUST report `packages.pkgsrc` +- **AND** illumos MUST report `packages.ips` + +#### Scenario: macOS reports receipts, apps, and optional homebrew + +- **WHEN** packages are discovered on macOS +- **THEN** `packages.receipts` MUST be reported from the installer receipt database +- **AND** `packages.apps` MUST be reported from the installed `.app` inventory and MUST NOT be merged into `receipts` +- **AND** `packages.homebrew` MUST be reported only when a Homebrew prefix exists + +#### Scenario: Windows reports registry and appx + +- **WHEN** packages are discovered on Windows +- **THEN** `packages.registry` MUST be reported from both HKLM uninstall hives +- **AND** `packages.appx` MUST be reported from the provisioned set plus the collector context + +#### Scenario: Plan 9 reports no packages + +- **WHEN** packages are discovered on Plan 9 +- **THEN** no `packages` subtree MUST be emitted + +### Requirement: Package collection stays cheap and context-bounded + +Facts SHALL collect each package source with a single cheap read and SHALL report only system-global databases plus the collector's own execution context. + +#### Scenario: no per-package process spawning or network + +- **WHEN** a package source is collected +- **THEN** it MUST be read with one on-disk database read or one batch query +- **AND** it MUST NOT spawn one process per package +- **AND** it MUST NOT perform any network request + +#### Scenario: other users' per-user installs are not enumerated + +- **WHEN** a host has per-user installs owned by other users (homebrew, nix profiles, Windows HKCU, per-user flatpak or appx) +- **THEN** discovery MUST NOT drop privileges or walk other users' homes to enumerate them +- **AND** the under-report MUST be documented diff --git a/openspec/changes/add-packages-fact/tasks.md b/openspec/changes/add-packages-fact/tasks.md new file mode 100644 index 00000000..2295cd48 --- /dev/null +++ b/openspec/changes/add-packages-fact/tasks.md @@ -0,0 +1,30 @@ +## 1. Tests First + +- [ ] 1.1 Add record-shape tests: every record carries `name` and a verbatim `version`; `packages.` is an array, never a name-keyed map. +- [ ] 1.2 Add collision tests proving siblings are kept: dpkg multiarch (`libc6:amd64` + `:i386`), rpm install-only multiversion kernels (same name + arch), homebrew formula-vs-cask `docker`, flatpak branches, Windows x86/x64 same DisplayName. +- [ ] 1.3 Add per-source identity-field tests: `architecture` (dpkg/rpm/apk), `type`+`tap` (homebrew), `branch`+`architecture` (flatpak), `store_path` (nix), `bundle_id`+`path` (apps), `product_code`+`architecture` (registry). +- [ ] 1.4 Add tests that a source is omitted when its database is absent, that records are never merged across sources, and that Plan 9 emits no `packages` subtree. +- [ ] 1.5 Add reader/parse fixtures per source (dpkg status, `rpm -qa` output, pacman `desc`, apk `installed`, snapd state, flatpak list, pkgng query, openbsd `/var/db/pkg`, pkgsrc PKG_DBDIR, ips `pkg list`, nix profile manifest, macOS receipts plist + Info.plist, registry hive, appx). + +## 2. Implementation + +- [ ] 2.1 Add `internal/engine/packages.go` with `packagesCoreFacts(s *Session) []ResolvedFact`; compose it into `buildCoreFacts`. Pure `parse*` functions per source, no GOOS-suffixed files (ADR-0010). +- [ ] 2.2 Implement the Linux readers: `dpkg`, `rpm` (explicit epoch-bearing query), `pacman`, `apk`, `snap`, `flatpak`. +- [ ] 2.3 Implement the BSD/illumos readers: `pkg` (FreeBSD + DragonFly), `openbsd_pkg`, `pkgsrc` (NetBSD), `ips` (illumos). +- [ ] 2.4 Implement the macOS readers: `receipts` (primary), `apps` (secondary, never merged), `homebrew` (optional auto-detected); and `nix` where a profile is present. +- [ ] 2.5 Implement the Windows readers: `registry` (both HKLM hives, `product_code`+`architecture`) and `appx` (provisioned + collector context). +- [ ] 2.6 Enforce the collection rule (one cheap read per source; no spawn-per-package; no network) and the system-global + collector-context boundary. +- [ ] 2.7 Register `packages` as a fact group (no default-visibility change in this work). + +## 3. Schema and Docs + +- [ ] 3.1 Add `packages.*` to `docs/schema/facts.yaml` under ADR-0011 canonical spelling: the per-source arrays, the always-present `name`/`version`, and the per-source identity fields, with per-platform applicability. +- [ ] 3.2 Regenerate `docs/supported-facts/` to show the per-source record shapes. +- [ ] 3.3 Update README, man page, and CHANGELOG to mention the `packages` fact and its scope (system package databases; language managers out). + +## 4. Verification + +- [ ] 4.1 Run focused resolver/parser tests for every source. +- [ ] 4.2 Run `go test ./...` and `go vet ./...`. +- [ ] 4.3 Validate the fact on each supported target's native smoke/release gate (dpkg, rpm, pacman, apk, snap/flatpak where present, pkg, openbsd_pkg, pkgsrc, ips, macOS receipts/apps, windows registry/appx); confirm Plan 9 emits nothing. +- [ ] 4.4 Run `openspec validate add-packages-fact --strict`. diff --git a/openspec/changes/optimize-bsd-disks-probe/design.md b/openspec/changes/optimize-bsd-disks-probe/design.md new file mode 100644 index 00000000..cebd5896 --- /dev/null +++ b/openspec/changes/optimize-bsd-disks-probe/design.md @@ -0,0 +1,11 @@ +## Decisions + +- **Denylist known pseudo-devices, do not whitelist real ones.** Filter the `kern.disks` device list to exclude the pseudo-device prefixes `md` (memory disk), `vn` (vnode/file-backed disk), and `cd` (optical) before probing. A denylist of well-known pseudo-devices is robust to new real-disk driver prefixes; a whitelist would silently drop unfamiliar real disks. +- **DragonFly: stop the unconditional five-target fan-out.** Instead of spawning `disklabel` on ``, `s1`…`s4` for every device, probe the device and only the slices that actually exist. Investigate whether one `disklabel ` already enumerates the slices, avoiding per-slice spawns entirely. +- **Behavior-preserving for real disks.** The `disks`/`partitions` facts for real storage must be byte-identical to today; only pseudo-devices are removed and only wasted subprocesses are eliminated. + +## Implementation Notes + +- Add a shared helper (`isPseudoDisk(name) bool` / `filterRealDisks([]string) []string`) and apply it wherever a BSD probe iterates `kern.disks`: `currentDragonFlyPartitions`, `currentOpenBSDPartitions`, `currentNetBSDPartitions`, and the disks-list enumeration in `disks.go`. +- TDD: table tests with a fake `commandRunner` asserting (a) `md*`/`vn*`/`cd*` devices are skipped — no `disklabel` spawned for them (record the runner's calls), (b) real devices are still probed and parsed, (c) partition output for real disks is unchanged. +- Validate on the nlab DragonFly/OpenBSD/NetBSD guests: discovery time and `disklabel` spawn count drop sharply, `partitions` excludes `md*`, real-disk facts are unchanged, and the native release gates still pass. diff --git a/openspec/changes/optimize-bsd-disks-probe/proposal.md b/openspec/changes/optimize-bsd-disks-probe/proposal.md new file mode 100644 index 00000000..7c43c207 --- /dev/null +++ b/openspec/changes/optimize-bsd-disks-probe/proposal.md @@ -0,0 +1,16 @@ +## Why + +The BSD disks/partitions probe enumerates every device from `kern.disks` and probes each with `disklabel`; on DragonFly it runs `disklabel` on five slice targets (`s1`–`s4` plus the device) per device. On a host with many pseudo-devices this fans out to hundreds of subprocess spawns per discovery. The nlab/CI DragonFly guest lists **132 devices, 127 of them `md` memory disks**, so one discovery spawns **660 `disklabel` processes** (~1m42s of system time), and every full `Run()` discovery pays it — which is why the emulated DragonFly VM's `internal/app` integration suite runs for ~11 minutes. The `md`/`vn` pseudo-devices are also not physical host storage and should not be reported as disks/partitions at all. + +## What Changes + +- The BSD disks/partitions probes **skip pseudo-devices** — memory disks (`md*`), vnode/file-backed disks (`vn*`), and optical devices (`cd*`) — when enumerating `kern.disks`. +- The DragonFly probe stops blindly spawning `disklabel` on non-existent slice targets for every device; it probes only real devices and their actual slices. +- Result: on hosts with many pseudo-devices, discovery drops from hundreds of subprocess spawns to a handful, and `disks`/`partitions` no longer list phantom memory/vnode disks. + +## Impact + +- **Behavior**: `disks`/`partitions` on the BSDs exclude `md*`/`vn*`/`cd*` pseudo-devices — a correctness improvement (these were never real host storage). +- **Performance**: full discovery is markedly faster on BSD hosts with many pseudo-devices; the emulated CI/lab DragonFly `internal/app` suite speeds up. +- **Compatibility**: real disks (`da*`, `ada*`, `ad*`, `wd*`, `sd*`, `vtbd*`, …) and their partitions are unchanged. +- **Out of scope**: the FreeBSD path (single `sysctl kern.geom.confxml` call, no per-device fan-out); the Linux/darwin/Windows/illumos disk probes; the CI `go test -timeout` bump (already landed on the disable-mechanism PR). diff --git a/openspec/changes/optimize-bsd-disks-probe/specs/go-port-supported-platform-facts/spec.md b/openspec/changes/optimize-bsd-disks-probe/specs/go-port-supported-platform-facts/spec.md new file mode 100644 index 00000000..dca98e03 --- /dev/null +++ b/openspec/changes/optimize-bsd-disks-probe/specs/go-port-supported-platform-facts/spec.md @@ -0,0 +1,21 @@ +## ADDED Requirements + +### Requirement: BSD disk probes skip pseudo-devices and bound subprocess fan-out + +Facts SHALL exclude pseudo-devices — memory disks (`md*`), vnode/file-backed disks (`vn*`), and optical devices (`cd*`) — from the BSD `disks`/`partitions` probes, and SHALL NOT spawn a partition-probe subprocess for non-existent slice targets, so discovery stays fast on hosts with many pseudo-devices while real-disk facts are unchanged. + +#### Scenario: pseudo-devices are neither probed nor reported + +- **WHEN** `kern.disks` lists `md*`, `vn*`, or `cd*` pseudo-devices alongside real disks +- **THEN** the disks/partitions probe MUST NOT spawn a `disklabel` (or equivalent) subprocess for the pseudo-devices +- **AND** `disks` and `partitions` MUST NOT report the pseudo-devices + +#### Scenario: real-disk facts are unchanged + +- **WHEN** a real disk such as `da0`, `ada0`, or `wd0` is present +- **THEN** its `disks` and `partitions` facts MUST be identical to before this change + +#### Scenario: DragonFly does not fan out to non-existent slices + +- **WHEN** the DragonFly probe enumerates a device's partitions +- **THEN** it MUST NOT spawn a `disklabel` subprocess for slice targets that do not exist on the device diff --git a/openspec/changes/optimize-bsd-disks-probe/tasks.md b/openspec/changes/optimize-bsd-disks-probe/tasks.md new file mode 100644 index 00000000..066dcfcd --- /dev/null +++ b/openspec/changes/optimize-bsd-disks-probe/tasks.md @@ -0,0 +1,16 @@ +## 1. Tests First + +- [ ] 1.1 Add a probe test (fake `commandRunner` recording calls) proving `md*`/`vn*`/`cd*` devices from `kern.disks` are skipped — no `disklabel` (or equivalent) subprocess is issued for them. +- [ ] 1.2 Add a test proving real devices (`da0`, `ada0`, `wd0`, …) are still probed and their partitions parsed unchanged. +- [ ] 1.3 Add a DragonFly test proving the probe does not spawn `disklabel` for non-existent slice targets of every device. + +## 2. Implementation + +- [ ] 2.1 Add a shared `isPseudoDisk`/`filterRealDisks` helper and apply it in `currentDragonFlyPartitions`, `currentOpenBSDPartitions`, `currentNetBSDPartitions`, and the disks-list enumeration (`disks.go`). +- [ ] 2.2 DragonFly: bound the slice-target fan-out to slices that actually exist (or derive slices from a single `disklabel `). + +## 3. Verification + +- [ ] 3.1 Run `go test ./...` and `go vet ./...`. +- [ ] 3.2 nlab validation on the DragonFly/OpenBSD/NetBSD guests: measure the drop in `disklabel` spawns and discovery time, confirm `partitions` excludes `md*`, confirm real-disk facts unchanged, and run the native release gates. +- [ ] 3.3 Run `openspec validate optimize-bsd-disks-probe --strict`.