harden fact discovery contracts

.github/workflows/integration_tests.yaml

@@ -73,7 +73,7 @@ jobs:
             expected_description_prefix: Arch Linux
             expected_os_release_major: ""
             expected_distro_release_major: ""
-            require_release: "true"
+            require_release: "false"
           - name: Oracle Linux 9
             image: oraclelinux:9
             expected_os_name: OracleLinux
@@ -100,7 +100,9 @@ jobs:
           CGO_ENABLED: 0
           GOOS: linux
           GOARCH: amd64
-        run: go build -o dist/facts-linux-amd64 ./cmd/facts
+        run: |
+          mkdir -p dist
+          go build -o dist/facts-linux-amd64 ./cmd/facts
 
       - name: Validate distro facts
         shell: bash

.gitignore

@@ -1,7 +1,9 @@
 *.gem
 *.rbc
 /facts
+/facts.exe
 /.config
+/coverage.out
 /coverage/
 /InstalledFiles
 /pkg/

Makefile

@@ -6,8 +6,8 @@ COUNT ?= 5
 
 LIMACTL ?= limactl
 LIMA_INSTANCE_PREFIX ?= facts
-LIMA_GO_VERSION ?= 1.25.0
-LIMA_GO_SERIES ?= 1.25
+LIMA_GO_VERSION ?= 1.26.4
+LIMA_GO_SERIES ?= 1.26
 LIMA_GOARCH ?= arm64
 LIMA_CPUS ?= 6
 LIMA_MEMORY ?= 10

engine.go

@@ -120,6 +120,9 @@ func WithSystemDefaults() Option {
 // early, err satisfies errors.Is(err, ctx.Err()). Not-applicable facts are
 // absent from the Snapshot and contribute nothing to err.
 func (e *Engine) Discover(ctx context.Context, queries ...string) (*Snapshot, error) {
+	if e == nil || e.inner == nil {
+		return nil, errors.New("facts: uninitialized Engine")
+	}
 	inner, err := e.inner.Discover(ctx, queries...)
 	return &Snapshot{inner: inner}, err
 }

engine_test.go

@@ -62,6 +62,18 @@ func TestNew_defaultEngineIsHermetic(t *testing.T) {
 	}
 }
 
+func TestEngineDiscover_uninitializedReceiverReturnsError(t *testing.T) {
+	var nilEngine *Engine
+	if snap, err := nilEngine.Discover(context.Background()); err == nil || snap != nil {
+		t.Fatalf("nil Engine Discover() = %#v, %v, want nil snapshot and error", snap, err)
+	}
+
+	var zero Engine
+	if snap, err := zero.Discover(context.Background()); err == nil || snap != nil {
+		t.Fatalf("zero Engine Discover() = %#v, %v, want nil snapshot and error", snap, err)
+	}
+}
+
 func TestWithExternalDirs_loadsExactlyOptedDirs(t *testing.T) {
 	t.Setenv("FACTER_env_probe", "leaked")
 	dir := t.TempDir()
@@ -595,6 +607,24 @@ func TestAs_shapeMismatchFailsLoudly(t *testing.T) {
 	}
 }
 
+func TestAs_rejectsMapAnyKeyStringCollisions(t *testing.T) {
+	eng, err := New(WithFact("ambiguous", func(context.Context) (any, error) {
+		return map[any]any{"1": "string-key", 1: "int-key"}, nil
+	}))
+	if err != nil {
+		t.Fatal(err)
+	}
+	snap, err := eng.Discover(context.Background())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	_, err = As[map[string]string](snap, "ambiguous")
+	if err == nil || !strings.Contains(err.Error(), `duplicate map key after string normalization: "1"`) {
+		t.Fatalf("As ambiguous err = %v, want duplicate normalized key error", err)
+	}
+}
+
 func TestAs_missingFactReturnsErrFactNotFound(t *testing.T) {
 	snap := hermeticSnapshot()
 

internal/app/app.go

@@ -498,6 +498,10 @@ func writeVersionQuery(stdout io.Writer, jsonOutput, yamlOutput, hoconOutput boo
 	if err != nil {
 		return err
 	}
+	if strings.HasSuffix(out, "\n") {
+		_, err = fmt.Fprint(stdout, out)
+		return err
+	}
 	_, err = fmt.Fprintln(stdout, out)
 	return err
 }

internal/app/app_test.go

@@ -708,6 +708,17 @@ func TestRun_queryYAML(t *testing.T) {
 	}
 }
 
+func TestRun_queryFacterversionYAMLHasSingleTrailingNewline(t *testing.T) {
+	var stdout, stderr bytes.Buffer
+
+	if err := Run(&stdout, &stderr, []string{"--yaml", "facterversion"}); err != nil {
+		t.Fatal(err)
+	}
+	if got, want := stdout.String(), "facterversion: "+engine.Version+"\n"; got != want {
+		t.Fatalf("stdout = %q, want %q", got, want)
+	}
+}
+
 func TestRun_queryHOCON(t *testing.T) {
 	var stdout, stderr bytes.Buffer
 

internal/app/loghandler.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"io"
 	"log/slog"
+	"sync"
 )
 
 // stderrLogHandler renders engine diagnostics as Ruby-compatible stderr lines
@@ -17,6 +18,7 @@ type stderrLogHandler struct {
 	color   bool
 	debug   bool
 	verbose bool
+	mu      sync.Mutex
 }
 
 func (h *stderrLogHandler) Enabled(_ context.Context, level slog.Level) bool {
@@ -33,6 +35,9 @@ func (h *stderrLogHandler) Enabled(_ context.Context, level slog.Level) bool {
 }
 
 func (h *stderrLogHandler) Handle(_ context.Context, record slog.Record) error {
+	h.mu.Lock()
+	defer h.mu.Unlock()
+
 	switch {
 	case record.Level >= slog.LevelError:
 	case record.Level >= slog.LevelWarn:

internal/app/loghandler_test.go

@@ -3,6 +3,7 @@ package app
 import (
 	"bytes"
 	"log/slog"
+	"sync"
 	"testing"
 )
 
@@ -37,3 +38,18 @@ func TestStderrLogHandlerDropsErrorClassKeepsWarnDebug(t *testing.T) {
 		}
 	})
 }
+
+func TestStderrLogHandlerConcurrentHandle(t *testing.T) {
+	var stderr bytes.Buffer
+	logger := slog.New(&stderrLogHandler{stderr: &stderr})
+
+	var wg sync.WaitGroup
+	for range 8 {
+		wg.Go(func() {
+			for range 50 {
+				logger.Warn("heads up")
+			}
+		})
+	}
+	wg.Wait()
+}

internal/cli/arguments.go

@@ -12,8 +12,18 @@ func PrepareArguments(args []string) []string {
 
 	priority := make([]string, 0, len(prepared))
 	normal := make([]string, 0, len(prepared))
+	afterDelimiter := false
 	for i := 0; i < len(prepared); i++ {
 		arg := prepared[i]
+		if afterDelimiter {
+			normal = append(normal, arg)
+			continue
+		}
+		if arg == "--" {
+			normal = append(normal, arg)
+			afterDelimiter = true
+			continue
+		}
 		if IsTaskFlag(arg) || IsTask(arg) {
 			priority = append(priority, arg)
 			continue
@@ -29,7 +39,11 @@ func PrepareArguments(args []string) []string {
 
 func expandShortOptions(args []string) []string {
 	expanded := make([]string, 0, len(args))
-	for _, arg := range args {
+	for i, arg := range args {
+		if arg == "--" {
+			expanded = append(expanded, args[i:]...)
+			break
+		}
 		if len(arg) <= 2 || arg[0] != '-' || arg[1] == '-' || strings.ContainsRune(arg, '=') {
 			expanded = append(expanded, arg)
 			continue
@@ -48,6 +62,9 @@ func expandShortOptions(args []string) []string {
 func containsKnownTaskOrMappedFlag(args []string) bool {
 	for i := 0; i < len(args); i++ {
 		arg := args[i]
+		if arg == "--" {
+			return false
+		}
 		if IsTask(arg) || IsTaskFlag(arg) {
 			return true
 		}

internal/cli/arguments_test.go

@@ -30,6 +30,14 @@ func TestPrepareArguments_reordersShortVersionFlag(t *testing.T) {
 	}
 }
 
+func TestPrepareArguments_preservesFlagsAfterDelimiterAsQueries(t *testing.T) {
+	got := PrepareArguments([]string{"--", "-v"})
+	want := []string{"query", "--", "-v"}
+	if !slices.Equal(got, want) {
+		t.Fatalf("PrepareArguments() = %v, want %v", got, want)
+	}
+}
+
 func TestPrepareArguments_doesNotPromoteTaskFlagWithInlineValue(t *testing.T) {
 	got := PrepareArguments([]string{"--help=topic"})
 	want := []string{"query", "--help=topic"}
@@ -98,7 +106,7 @@ func TestValidateOptions_allowsRepeatedExternalDir(t *testing.T) {
 }
 
 func TestValidateOptions_rejectsMissingRequiredOptionValue(t *testing.T) {
-	err := ValidateOptions([]string{"query", "--external-dir", "--no-external-facts", "site"})
+	err := ValidateOptions([]string{"query", "--external-dir"})
 	if err == nil {
 		t.Fatal("ValidateOptions() err = nil, want missing option value error")
 	}
@@ -107,6 +115,27 @@ func TestValidateOptions_rejectsMissingRequiredOptionValue(t *testing.T) {
 	}
 }
 
+func TestValidateOptions_allowsDashPrefixedOptionValues(t *testing.T) {
+	err := ValidateOptions([]string{"query", "--external-dir", "-facts", "site"})
+	if err != nil {
+		t.Fatalf("ValidateOptions() err = %v, want nil", err)
+	}
+}
+
+func TestValidateOptions_stopsAtDelimiter(t *testing.T) {
+	err := ValidateOptions([]string{"query", "--", "-v"})
+	if err != nil {
+		t.Fatalf("ValidateOptions() err = %v, want nil", err)
+	}
+}
+
+func TestValidateOptions_stopsAtFirstQuery(t *testing.T) {
+	err := ValidateOptions([]string{"query", "os.name", "--missing-fact-name"})
+	if err != nil {
+		t.Fatalf("ValidateOptions() err = %v, want nil", err)
+	}
+}
+
 func TestValidateOptions_rejectsUnknownConcatenatedShortFlag(t *testing.T) {
 	args := PrepareArguments([]string{"-jdtz"})
 	err := ValidateOptions(args)

internal/cli/validation.go

@@ -47,6 +47,15 @@ func validateOptions(args []string) error {
 	logLevel := ""
 	for i := 0; i < len(args); i++ {
 		arg := args[i]
+		if arg == "--" {
+			break
+		}
+		if !strings.HasPrefix(arg, "-") {
+			if IsTask(arg) {
+				continue
+			}
+			break
+		}
 		if strings.HasPrefix(arg, "-") {
 			seenRaw[rawOption(arg)] = true
 			option, ok := LookupOption(arg)
@@ -64,7 +73,7 @@ func validateOptions(args []string) error {
 			}
 		}
 		if OptionTakesSeparateValue(arg) {
-			if i+1 >= len(args) || strings.HasPrefix(args[i+1], "-") {
+			if i+1 >= len(args) {
 				return fmt.Errorf("%s requires a value", CanonicalOption(arg))
 			}
 			i++

internal/engine/cache.go

@@ -19,7 +19,7 @@ const cacheFormatVersion = 1
 
 var (
 	cacheRemove    = os.Remove
-	cacheWriteFile = os.WriteFile
+	cacheWriteFile = writeCacheFile
 )
 
 // DefaultCachePath returns the platform default directory for cached fact groups.
@@ -191,7 +191,7 @@ func (fc *FactCache) CacheFacts(facts []ResolvedFact) error {
 	}
 	grouped := make(map[string]map[string]any)
 	for _, fact := range facts {
-		group, _, ok := fc.cacheGroupForFact(fact.Name)
+		group, _, ok := fc.cacheGroupForResolvedFact(fact)
 		if !ok {
 			continue
 		}
@@ -241,6 +241,38 @@ func warnCacheWriteFailure(err error, log *slog.Logger) bool {
 	return true
 }
 
+func writeCacheFile(path string, data []byte, perm os.FileMode) error {
+	dir := filepath.Dir(path)
+	tmp, err := os.CreateTemp(dir, filepath.Base(path)+".tmp-*")
+	if err != nil {
+		return err
+	}
+	tmpPath := tmp.Name()
+	cleanup := true
+	defer func() {
+		if cleanup {
+			_ = os.Remove(tmpPath)
+		}
+	}()
+
+	if _, err := tmp.Write(data); err != nil {
+		_ = tmp.Close()
+		return err
+	}
+	if err := tmp.Chmod(perm); err != nil {
+		_ = tmp.Close()
+		return err
+	}
+	if err := tmp.Close(); err != nil {
+		return err
+	}
+	if err := os.Rename(tmpPath, path); err != nil {
+		return err
+	}
+	cleanup = false
+	return nil
+}
+
 func (fc *FactCache) cacheGroupForFact(name string) (string, time.Duration, bool) {
 	bestGroup := ""
 	bestTTL := time.Duration(0)
@@ -404,7 +436,7 @@ func parseTTLDuration(value string) (time.Duration, bool) {
 		return 0, false
 	}
 	amount, err := strconv.Atoi(fields[0])
-	if err != nil {
+	if err != nil || amount < 0 {
 		return 0, false
 	}
 	unit := ""
@@ -418,6 +450,9 @@ func parseTTLDuration(value string) (time.Duration, bool) {
 	if !ok {
 		return 0, false
 	}
+	if multiplier > 0 && time.Duration(amount) > time.Duration(1<<63-1)/multiplier {
+		return 0, false
+	}
 	duration := time.Duration(amount) * multiplier
 	if multiplier < time.Second {
 		duration = duration.Truncate(time.Second)