From 9db31cf1b3565c8e3a4c153f6cdb0bec4c53d051 Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Tue, 8 Sep 2026 17:54:44 +0100 Subject: [PATCH 01/21] Updated and aligned platforms docs Signed-off-by: Mark Bolwell --- CHANGELOG.md | 11 +++++++++++ docs/platforms.md | 33 +++++++++++++++++++++++++-------- 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 83335b3..55e4336 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,17 @@ resource quietly disappearing shows up as a rise in skips rather than a failure +- docs + - the command support matrix in `docs/platforms.md` understated macOS and + Windows. It marked `serve` on Windows as never tried, and `serve` and + `validate` on macOS as working but without automated tests. All three run on + every push and pass, as do `add` and `help` on both platforms. The cells are + now measured from CI job logs rather than estimated, and the page says which + log lines prove them, because the workflow derives its target from `go env` + and so tells you a lane is wired up rather than that it ran. `autoadd` and + `render` are unchanged: `autoadd`'s fixture is skipped on both platforms and + `render` has no fixture anywhere + ## 0.11.2 based on krameff/goss v0.6.0 - signed SBOMs and a patched base image - supply chain diff --git a/docs/platforms.md b/docs/platforms.md index ba8b592..64fed0c 100644 --- a/docs/platforms.md +++ b/docs/platforms.md @@ -215,14 +215,31 @@ passed before may now fail where it was never actually being checked. ## Commands support matrix -| Test | Linux | macOS | Windows | -|:-----------|------------------------|---------------------|----------------------| -| `add` | {{ fully_supported }} | {{ no_data }} | {{ work_partially }} | -| `autoadd` | {{ fully_supported }} | {{ no_data }} | {{ no_data }} | -| `help` | {{ fully_supported }} | {{ no_data }} | {{ work_partially }} | -| `render` | {{ fully_supported }} | {{ no_data }} | {{ no_data }} | -| `serve` | {{ fully_supported }} | {{ not_automated }} | {{ no_data }} | -| `validate` | {{ fully_supported }} | {{ not_automated }} | {{ work_partially }} | +| Test | Linux | macOS | Windows | +|:-----------|-----------------------|-------------------------|-------------------------| +| `add` | {{ fully_supported }} | {{ work_partially }} | {{ work_partially }} | +| `autoadd` | {{ fully_supported }} | {{ no_data }} | {{ no_data }} | +| `help` | {{ fully_supported }} | {{ work_partially }} | {{ work_partially }} | +| `render` | {{ fully_supported }} | {{ no_data }} | {{ no_data }} | +| `serve` | {{ fully_supported }} | {{community_supported}} | {{community_supported}} | +| `validate` | {{ fully_supported }} | {{ work_partially }} | {{ work_partially }} | + +The macOS and Windows cells above are measured from CI, not estimated. Every +`add`, `help`, `serve` and `validate` cell describes a lane that runs on every +push and passes. `autoadd` is genuinely untested on both: its fixture carries +`skip: true`, so it asserts nothing. `render` has no fixture on any platform. + +**Check rather than trust this table.** The workflow derives its target from +`go env`, so reading the workflow tells you a lane is wired up, not that it ran. +Read the job log: + +```bash +gh run list --branch main --workflow=golangci.yaml --limit 1 +# then, in the "Integration tests (macos-latest)" and "(windows-latest)" jobs, +# look for these lines: +# test-int-validate-- ... -: N fixtures, N assertions, N skipped +# test-int-serve-- ... serve tests passed +``` ### `command` testing notes From 88d32956a12b274f0bcc950716928f8a53710b97 Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Tue, 8 Sep 2026 18:27:20 +0100 Subject: [PATCH 02/21] fix(mount): report unsupported platforms honestly, without touching the shared pat Signed-off-by: Mark Bolwell --- system/mount.go | 30 ++++++++++ system/mount_posix.go | 5 ++ system/mount_supported_test.go | 102 +++++++++++++++++++++++++++++++++ system/mount_windows.go | 40 +++++++------ 4 files changed, 159 insertions(+), 18 deletions(-) create mode 100644 system/mount_supported_test.go diff --git a/system/mount.go b/system/mount.go index f6e4913..6915653 100644 --- a/system/mount.go +++ b/system/mount.go @@ -21,6 +21,27 @@ import ( // error. See FEAT-010 SW-10 / Trap 1's reasoning, applied here. var ErrMountpointNotFound = errors.New("mountpoint not found") +// ErrMountUnsupported is returned when the platform has no mount lookup at +// all, as distinct from a lookup that ran and found nothing. Following the +// sentinel-error idiom already used by registry (ErrRegistryUnsupported), +// package (ErrNullPackage) and file (ErrFileOwnershipUnsupported). +// +// WHY THIS EXISTS RATHER THAN A REORDER. setup() calls getMount before the +// platform getUsage, and on Windows the vendored mountinfo returns an EMPTY +// TABLE rather than an error -- its own comment says "Do NOT return an +// error!" -- which getMount converts to ErrMountpointNotFound. So every +// Windows mount check blamed the operator's mountpoint for what is actually a +// missing implementation, and mount_windows.go's honest error was unreachable. +// FEAT-010 SW-7 recorded that and deferred it; FEAT-011 W2-12(a) scheduled it. +// +// The obvious fix is to run getUsage first. It was rejected: getMount and +// getUsage are shared by every OS, and reordering them changes which error a +// non-existent path produces on Linux and macOS, where mount: is fully +// supported and heavily used. A platform capability check placed BEFORE both +// is a no-op everywhere the platform is supported, so Linux and macOS +// behaviour is unchanged by construction rather than by inspection. +var ErrMountUnsupported = errors.New("mount: not supported on this platform") + type Mount interface { MountPoint() string Exists() (bool, error) @@ -54,6 +75,15 @@ func (m *DefMount) setup() error { } m.loaded = true + // Before anything else: does this platform have a mount lookup at all? See + // ErrMountUnsupported for why this is a separate check rather than a + // reordering of the two calls below. + if err := mountSupported(); err != nil { + m.exists = false + m.err = err + return m.err + } + mountInfo, err := getMount(m.mountPoint, m.Timeout) if err != nil { m.exists = false diff --git a/system/mount_posix.go b/system/mount_posix.go index a68eac0..5615b0a 100644 --- a/system/mount_posix.go +++ b/system/mount_posix.go @@ -8,6 +8,11 @@ import ( "syscall" ) +// mountSupported reports that this platform has a working mount lookup. It is +// a no-op by design: the whole point of the check is that it changes nothing +// where mount: is implemented. See ErrMountUnsupported in mount.go. +func mountSupported() error { return nil } + func getUsage(mountpoint string) (int, error) { statfsOut := &syscall.Statfs_t{} err := syscall.Statfs(mountpoint, statfsOut) diff --git a/system/mount_supported_test.go b/system/mount_supported_test.go new file mode 100644 index 0000000..85a05ea --- /dev/null +++ b/system/mount_supported_test.go @@ -0,0 +1,102 @@ +package system + +import ( + "context" + "errors" + "runtime" + "testing" + "time" + + "github.com/krameff/syver/util" +) + +// These tests exist for FEAT-013 Task 4 / FEAT-011 W2-12(a). The change makes +// Windows report honestly that mount: is not implemented, instead of blaming +// the operator's mountpoint. Making Windows truthful is the easy half. The +// acceptance criterion is the other half: that Linux and macOS behaviour is +// unchanged. That is what these assert. + +// newTestConfig builds the minimal util.Config these tests need. NewConfig +// returns an error, and a test that ignored it could mask a config change with +// a nil-pointer panic further down. +func newTestConfig(t *testing.T) util.Config { + t.Helper() + c, err := util.NewConfig() + if err != nil { + t.Fatalf("util.NewConfig(): %v", err) + } + // The zero-value Timeout means getMount times out immediately, which would + // mask the very distinction these tests exist to check. + c.Timeout = 5 * time.Second + return *c +} + +// TestMountSupportedMatchesPlatform pins the capability answer itself. If a +// future change makes mountSupported return an error on a platform where +// mount: is implemented, every mount assertion on that platform starts failing +// and this catches it in one line. +func TestMountSupportedMatchesPlatform(t *testing.T) { + err := mountSupported() + if runtime.GOOS == "windows" { + if !errors.Is(err, ErrMountUnsupported) { + t.Fatalf("mountSupported() = %v, want ErrMountUnsupported on windows", err) + } + return + } + if err != nil { + t.Fatalf("mountSupported() = %v, want nil on %s", err, runtime.GOOS) + } +} + +// TestMountLookupStillRunsOnSupportedPlatforms is the regression guard for the +// half of this change that carries risk. +// +// The capability check was deliberately placed BEFORE getMount rather than +// reordering getMount and getUsage, because those two are shared by every OS +// and their order determines which error a missing path produces. This asserts +// the shared path still reaches the real lookup: a bogus mountpoint must come +// back as ErrMountpointNotFound, the everyday "ran and found nothing" answer, +// and must NOT come back as ErrMountUnsupported. +// +// Getting those two confused is precisely the defect being fixed, in reverse. +func TestMountLookupStillRunsOnSupportedPlatforms(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("windows has no mount lookup; covered by TestMountSupportedMatchesPlatform") + } + + m := NewDefMount(context.Background(), "/syver-no-such-mountpoint-ffffffff", nil, newTestConfig(t)) + _, err := m.Exists() + if err == nil { + t.Fatal("Exists() on a bogus mountpoint returned nil error, want ErrMountpointNotFound") + } + if errors.Is(err, ErrMountUnsupported) { + t.Fatalf("Exists() = %v; the capability check short-circuited a platform that DOES support mount", err) + } + if !errors.Is(err, ErrMountpointNotFound) { + t.Fatalf("Exists() = %v, want ErrMountpointNotFound", err) + } +} + +// TestMountResolvesARealMountpoint proves the lookup does more than fail +// consistently. Without it, a mountSupported() that wrongly returned an error +// would still pass the test above if the error text happened to match. +func TestMountResolvesARealMountpoint(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("windows has no mount lookup") + } + + m := NewDefMount(context.Background(), "/", nil, newTestConfig(t)) + exists, err := m.Exists() + if err != nil { + t.Fatalf("Exists() on \"/\" returned %v, want it to resolve", err) + } + if !exists { + t.Fatal("Exists() on \"/\" = false, want true") + } + if _, err := m.Filesystem(); err != nil { + t.Errorf("Filesystem() on \"/\" returned %v, want a filesystem type", err) + } + if _, err := m.Usage(); err != nil { + t.Errorf("Usage() on \"/\" returned %v, want a usage percentage", err) + } +} diff --git a/system/mount_windows.go b/system/mount_windows.go index b01088f..51671cc 100644 --- a/system/mount_windows.go +++ b/system/mount_windows.go @@ -3,25 +3,29 @@ package system -import "errors" - -// errNotImplemented is currently UNREACHABLE in practice (FEAT-010 SW-7, -// deliberate, documented deferral -- not missed). system/mount.go's -// setup() calls getMount() first; on Windows the vendored mountinfo -// implementation returns an empty table rather than an error (its own -// comment says "Do NOT return an error!"), which getMount converts to -// ErrMountpointNotFound before getUsage (this function) is ever reached. -// So every Windows mount check fails with "mountpoint not found" -- -// misleading (it blames the mountpoint, not the missing implementation) -// but loud, which this spec's own priority order (a misleading-but-loud -// error is a far smaller problem than a silent pass) treats as acceptable -// to leave as-is here. +// mountSupported reports that Windows has no mount lookup. +// +// Until FEAT-013 this package could not say so. system/mount.go's setup() +// called getMount first, and on Windows the vendored mountinfo returns an +// EMPTY TABLE rather than an error -- its own comment says "Do NOT return an +// error!" -- which getMount converted to ErrMountpointNotFound. Every Windows +// mount check therefore failed by blaming the operator's mountpoint for what +// is actually a missing implementation, and the honest error below was +// unreachable. Recorded as FEAT-010 SW-7, scheduled as FEAT-011 W2-12(a). +// +// setup() now consults this before getMount, so the answer is truthful. See +// ErrMountUnsupported in mount.go for why the fix is a capability check rather +// than a reordering of the shared code path. // -// Making this reachable means reordering getMount, which is cross-platform -// code shared by every OS -- not "obviously safe" to restructure inside -// this spec's Windows-focused, Linux-verified scope, so it is left as-is. -var errNotImplemented = errors.New("not implemented") +// A real Windows backend is FEAT-011 W2-12(b), specced as FEAT-018: +// GetLogicalDriveStringsW, GetVolumeInformationW and GetDiskFreeSpaceExW. +// `opts` and `source` have no Windows meaning and stay unsupported rather than +// being faked. +func mountSupported() error { return ErrMountUnsupported } +// getUsage is unreachable while mountSupported returns an error, and is kept +// so the platform still satisfies the same shape as mount_posix.go. FEAT-018 +// replaces its body. func getUsage(mountpoint string) (int, error) { - return 0, errNotImplemented + return 0, ErrMountUnsupported } From 5ca14f0f7a6b7b2ccaa10938f3273dc598465dce Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Tue, 8 Sep 2026 18:28:25 +0100 Subject: [PATCH 03/21] fix(process): fail when every status read fails, not just when one does Signed-off-by: Mark Bolwell --- system/process.go | 65 ++++++++++++++++++++++------- system/process_collect_test.go | 76 ++++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 15 deletions(-) create mode 100644 system/process_collect_test.go diff --git a/system/process.go b/system/process.go index 9924966..dc4d0d6 100644 --- a/system/process.go +++ b/system/process.go @@ -2,6 +2,7 @@ package system import ( "context" + "fmt" "github.com/samber/lo" "github.com/shirou/gopsutil/v4/process" @@ -63,37 +64,71 @@ func (p *DefProcess) Running() (bool, error) { // Status returns the distinct process states (e.g. "running", "zombie") seen // across every PID matching this executable. A process that disappears or // can't be read between the snapshot and this call is skipped rather than -// failing the whole result, same as GetProcs. +// failing the whole result, same as GetProcs -- except when every one of them +// fails. See collectPerProcess. func (p *DefProcess) Status() ([]string, error) { if p.err != nil { return nil, p.err } - var statuses []string - for _, proc := range p.procMap[p.executable] { - s, err := processStatus(proc) - if err != nil { - continue - } - statuses = append(statuses, s...) - } - return lo.Uniq(statuses), nil + return collectPerProcess(p.procMap[p.executable], "status", processStatus) } // User returns the distinct usernames owning every PID matching this -// executable. +// executable. Same all-or-nothing rule as Status; see collectPerProcess. func (p *DefProcess) User() ([]string, error) { if p.err != nil { return nil, p.err } - var users []string - for _, proc := range p.procMap[p.executable] { + return collectPerProcess(p.procMap[p.executable], "user", func(proc *process.Process) ([]string, error) { u, err := processUser(proc) if err != nil { + return nil, err + } + return []string{u}, nil + }) +} + +// collectPerProcess gathers one attribute across every PID matching an +// executable, tolerating individual failures but not universal ones. +// +// Skipping a single failure is deliberate and predates this: a process can +// exit between the snapshot and the read, and one such race should not fail an +// assertion about the others. +// +// Failing when EVERY read fails is FEAT-013 / FEAT-011 W2-11 (SW-6). On +// Windows gopsutil returns a not-implemented error for `status` on every +// process, so the loop skipped all of them and returned an empty slice with a +// NIL ERROR. The check ran, found the process, reported nothing about it and +// passed -- the silent-wrongness class FEAT-010 exists to remove, and +// indistinguishable from an honest empty result unless the failures are +// counted. +// +// The rule is deliberately "all of them" rather than "any of them": a systemic +// failure is universal, whereas the race this tolerates is not. It needs no +// platform check and no gopsutil sentinel, which matters because gopsutil's +// ErrNotImplementedError lives in an internal package and cannot be compared +// against from here. +// +// User() gets the same treatment as Status(), not because it is known broken +// anywhere, but because the shape was identical and leaving one of two +// adjacent silent-pass paths in place is an arbitrary line to draw. +func collectPerProcess(procs []*process.Process, attr string, read func(*process.Process) ([]string, error)) ([]string, error) { + var out []string + var lastErr error + failed := 0 + for _, proc := range procs { + v, err := read(proc) + if err != nil { + failed++ + lastErr = err continue } - users = append(users, u) + out = append(out, v...) + } + if len(procs) > 0 && failed == len(procs) { + return nil, fmt.Errorf("reading %s failed for all %d matching processes: %w", attr, len(procs), lastErr) } - return lo.Uniq(users), nil + return lo.Uniq(out), nil } // listProcesses, processName, processStatus, and processUser are indirected diff --git a/system/process_collect_test.go b/system/process_collect_test.go new file mode 100644 index 0000000..d059eea --- /dev/null +++ b/system/process_collect_test.go @@ -0,0 +1,76 @@ +package system + +import ( + "errors" + "strings" + "testing" + + "github.com/shirou/gopsutil/v4/process" +) + +// These cover FEAT-013 Task 3 / FEAT-011 W2-11 (SW-6): Status() returned an +// empty slice and a NIL ERROR on Windows, because gopsutil fails for every +// process there and the loop skipped them all. The check ran, found the +// process, said nothing about it, and passed. +// +// collectPerProcess is tested directly rather than through Status(), because +// the distinction being made is about how many reads failed, and driving that +// through the real process table would make it a test of the host rather than +// of the rule. + +func TestCollectPerProcessToleratesSomeFailures(t *testing.T) { + procs := []*process.Process{{Pid: 1}, {Pid: 2}, {Pid: 3}} + calls := 0 + got, err := collectPerProcess(procs, "status", func(p *process.Process) ([]string, error) { + calls++ + if p.Pid == 2 { + return nil, errors.New("process vanished") + } + return []string{"running"}, nil + }) + if err != nil { + t.Fatalf("one failure out of three returned %v, want it tolerated", err) + } + if calls != 3 { + t.Errorf("read called %d times, want 3", calls) + } + if len(got) != 1 || got[0] != "running" { + t.Errorf("got %v, want the deduplicated results of the successful reads", got) + } +} + +func TestCollectPerProcessFailsWhenEveryReadFails(t *testing.T) { + procs := []*process.Process{{Pid: 1}, {Pid: 2}} + sentinel := errors.New("not implemented yet") + got, err := collectPerProcess(procs, "status", func(*process.Process) ([]string, error) { + return nil, sentinel + }) + if err == nil { + t.Fatalf("all reads failed but collectPerProcess returned %v with a nil error", got) + } + if !errors.Is(err, sentinel) { + t.Errorf("error %v does not wrap the underlying cause; the operator needs to see why", err) + } + if !strings.Contains(err.Error(), "status") { + t.Errorf("error %q does not name the attribute that failed", err) + } + if got != nil { + t.Errorf("got %v alongside the error, want nil", got) + } +} + +// An executable with no matching processes is not a failure. `Exists()` is what +// answers that question, and making Status() error here would turn an ordinary +// absent-process assertion into a hard failure. +func TestCollectPerProcessAcceptsNoProcesses(t *testing.T) { + got, err := collectPerProcess(nil, "status", func(*process.Process) ([]string, error) { + t.Fatal("read must not be called when there are no processes") + return nil, nil + }) + if err != nil { + t.Fatalf("no matching processes returned %v, want nil", err) + } + if len(got) != 0 { + t.Errorf("got %v, want empty", got) + } +} From 9adacfefd70bd7675e80481f364eb952d8433182 Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Tue, 8 Sep 2026 18:28:55 +0100 Subject: [PATCH 04/21] fix(paths): resolve absolute Windows includes and backslash home paths Signed-off-by: Mark Bolwell --- store.go | 20 +++++++- store_includepath_test.go | 70 ++++++++++++++++++++++++++ system/file.go | 30 +++++++---- system/file_realpath_test.go | 97 ++++++++++++++++++++++++++++++++++++ 4 files changed, 207 insertions(+), 10 deletions(-) create mode 100644 store_includepath_test.go create mode 100644 system/file_realpath_test.go diff --git a/store.go b/store.go index f7a3600..207a752 100644 --- a/store.go +++ b/store.go @@ -21,6 +21,24 @@ import ( "github.com/krameff/syver/util" ) +// isRootedIncludePath reports whether a `gossfile:` include should be used as +// given, rather than resolved relative to the file that included it. +// +// This used to be a bare strings.HasPrefix(path, "/"), which no `C:\...` path +// satisfies, so a genuinely absolute Windows include was joined onto the +// including file's directory and silently resolved somewhere else entirely. +// See FEAT-013 / FEAT-011 W2-9 (SW-12). +// +// BOTH tests are kept, deliberately. On Unix they are the same test -- +// filepath.IsAbs is exactly this prefix check -- so Linux and macOS behaviour +// is provably unchanged. On Windows they differ: filepath.IsAbs("/shared/x") +// is false there, because a drive-relative path is not absolute. Dropping the +// prefix test would have changed how an existing Windows gossfile resolves, +// which is a silent behaviour change rather than a fix. +func isRootedIncludePath(p string) bool { + return filepath.IsAbs(p) || strings.HasPrefix(p, "/") +} + const ( UNSET = iota JSON @@ -355,7 +373,7 @@ func mergeJSONData(syverConfig SyverConfig, depth int, path string) (SyverConfig for _, k := range keys { g := syverConfig.Syverfiles[k] var fpath string - if strings.HasPrefix(g.GetSyverfile(), "/") { + if isRootedIncludePath(g.GetSyverfile()) { fpath = g.GetSyverfile() } else { fpath = filepath.Join(path, g.GetSyverfile()) diff --git a/store_includepath_test.go b/store_includepath_test.go new file mode 100644 index 0000000..9c3aee6 --- /dev/null +++ b/store_includepath_test.go @@ -0,0 +1,70 @@ +package syver + +import ( + "runtime" + "testing" +) + +// TestIsRootedIncludePath covers FEAT-013 / FEAT-011 W2-9 (SW-12). +// +// The cases are split three ways on purpose. The shared cases must hold on +// every platform; the Unix and Windows blocks assert the two places the answer +// legitimately differs. Writing them as one table with a GOOS switch inside +// would hide exactly the distinction this fix is about. +func TestIsRootedIncludePath(t *testing.T) { + shared := []struct { + path string + want bool + }{ + {"/etc/syver/base.yaml", true}, + {"/base.yaml", true}, + {"base.yaml", false}, + {"./base.yaml", false}, + {"../shared/base.yaml", false}, + {"sub/dir/base.yaml", false}, + {"", false}, + } + for _, tc := range shared { + if got := isRootedIncludePath(tc.path); got != tc.want { + t.Errorf("isRootedIncludePath(%q) = %v, want %v", tc.path, got, tc.want) + } + } + + if runtime.GOOS == "windows" { + // The regression this fix exists for: an absolute Windows path was + // treated as relative and joined onto the including file's directory. + windows := []struct { + path string + want bool + }{ + {`C:\syver\base.yaml`, true}, + {`c:\syver\base.yaml`, true}, + {`C:/syver/base.yaml`, true}, + {`\\server\share\base.yaml`, true}, + {`C:base.yaml`, false}, // drive-relative, not absolute + {`base.yaml`, false}, + } + for _, tc := range windows { + if got := isRootedIncludePath(tc.path); got != tc.want { + t.Errorf("windows: isRootedIncludePath(%q) = %v, want %v", tc.path, got, tc.want) + } + } + return + } + + // On Unix a `C:\...` string is an ordinary relative filename. Asserting + // that keeps the fix from quietly changing Linux behaviour, which is the + // half of this change that carries real risk. + unix := []struct { + path string + want bool + }{ + {`C:\syver\base.yaml`, false}, + {`\\server\share\base.yaml`, false}, + } + for _, tc := range unix { + if got := isRootedIncludePath(tc.path); got != tc.want { + t.Errorf("unix: isRootedIncludePath(%q) = %v, want %v", tc.path, got, tc.want) + } + } +} diff --git a/system/file.go b/system/file.go index e2ea4ed..02e0f42 100644 --- a/system/file.go +++ b/system/file.go @@ -14,6 +14,7 @@ import ( "path/filepath" "strconv" "strings" + "unicode/utf8" "github.com/krameff/syver/util" ) @@ -170,25 +171,36 @@ func realPath(path string) (string, error) { if !strings.HasPrefix(path, "~") { return path, nil } - pathS := strings.Split(path, "/") - f := pathS[0] + // Split off the leading `~` or `~user` segment at the first PATH + // SEPARATOR, not at the first "/". This used to Split and Join on "/" + // unconditionally, so on Windows `~\Documents\x` had no "/" to split on: + // the whole string became one segment, `user.Lookup` was handed + // `\Documents\x` as an account name, and expansion failed. See FEAT-013 / + // FEAT-011 W2-9 (SW-13). + // + // os.IsPathSeparator is the platform's own answer: "/" on Unix, "/" or + // "\" on Windows. On Unix that makes this identical to the old split, so + // Linux and macOS behaviour is unchanged. + idx := strings.IndexFunc(path, func(r rune) bool { + return r < utf8.RuneSelf && os.IsPathSeparator(byte(r)) + }) + head, rest := path, "" + if idx >= 0 { + head, rest = path[:idx], path[idx:] + } var usr *user.User var err error - if f == "~" { + if head == "~" { usr, err = user.Current() } else { - usr, err = user.Lookup(f[1:]) + usr, err = user.Lookup(head[1:]) } if err != nil { return "", err } - pathS[0] = usr.HomeDir - - realPath := strings.Join(pathS, "/") - realPath, err = filepath.Abs(realPath) - return realPath, err + return filepath.Abs(filepath.Join(usr.HomeDir, rest)) } func (f *DefFile) hash(hashFunc hashFuncType) (string, error) { diff --git a/system/file_realpath_test.go b/system/file_realpath_test.go new file mode 100644 index 0000000..8151093 --- /dev/null +++ b/system/file_realpath_test.go @@ -0,0 +1,97 @@ +package system + +import ( + "os/user" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// TestRealPathLeavesNonTildePathsAlone is the invariant that must not move: +// realPath is only supposed to do anything at all for a `~` prefix. +func TestRealPathLeavesNonTildePathsAlone(t *testing.T) { + for _, p := range []string{ + "/etc/passwd", + "relative/path", + "", + `C:\Windows\System32`, + "file~with~tildes~inside", + } { + got, err := realPath(p) + if err != nil { + t.Fatalf("realPath(%q) returned error: %v", p, err) + } + if got != p { + t.Errorf("realPath(%q) = %q, want it returned unchanged", p, got) + } + } +} + +// TestRealPathExpandsTilde covers FEAT-013 / FEAT-011 W2-9 (SW-13). +// +// realPath used to Split and Join on "/" unconditionally. On Windows that left +// `~\Documents\x` as a single segment, so `\Documents\x` was handed to +// user.Lookup as an account name and expansion failed. Splitting at the +// platform's own path separator fixes Windows and is identical to the old +// behaviour on Unix, which is what the shared cases below pin down. +func TestRealPathExpandsTilde(t *testing.T) { + usr, err := user.Current() + if err != nil { + t.Skipf("cannot determine current user: %v", err) + } + home := usr.HomeDir + + cases := []struct { + name string + in string + want string + }{ + {"bare tilde", "~", home}, + {"tilde with slash path", "~/Documents/x", filepath.Join(home, "Documents", "x")}, + {"tilde with single segment", "~/x", filepath.Join(home, "x")}, + } + if runtime.GOOS == "windows" { + // The case the old implementation could not handle at all. + cases = append(cases, + struct { + name string + in string + want string + }{"tilde with backslash path", `~\Documents\x`, filepath.Join(home, "Documents", "x")}, + ) + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := realPath(tc.in) + if err != nil { + t.Fatalf("realPath(%q) returned error: %v", tc.in, err) + } + want, err := filepath.Abs(tc.want) + if err != nil { + t.Fatalf("filepath.Abs(%q): %v", tc.want, err) + } + if got != want { + t.Errorf("realPath(%q) = %q, want %q", tc.in, got, want) + } + }) + } +} + +// TestRealPathRejectsUnknownUser pins the error path. A `~someone` prefix for +// an account that does not exist must fail rather than silently returning +// something plausible, which is the same principle FEAT-010 applied throughout. +func TestRealPathRejectsUnknownUser(t *testing.T) { + const absent = "~syver-no-such-account-ffffffff/x" + got, err := realPath(absent) + if err == nil { + t.Fatalf("realPath(%q) = %q with nil error, want an error", absent, got) + } + if got != "" { + t.Errorf("realPath(%q) returned %q alongside its error, want the empty string", absent, got) + } + if strings.Contains(got, "syver-no-such-account") { + t.Errorf("realPath(%q) leaked the unresolved name into its result", absent) + } +} From dd647cf203b357329de437cfafd80cdf87050c4f Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Tue, 8 Sep 2026 18:29:44 +0100 Subject: [PATCH 05/21] fix(autoadd): report a resource skipped because its lookup failed Signed-off-by: Mark Bolwell --- resource/resource_map.go | 60 +++++++++++++++++++++------ resource/resource_map_absence_test.go | 60 +++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 13 deletions(-) create mode 100644 resource/resource_map_absence_test.go diff --git a/resource/resource_map.go b/resource/resource_map.go index 716a538..cc8f83c 100644 --- a/resource/resource_map.go +++ b/resource/resource_map.go @@ -2,7 +2,9 @@ package resource import ( "encoding/json" + "errors" "fmt" + "log" "reflect" "strings" @@ -53,6 +55,25 @@ func (r ResourceMap[T, ST, PT]) AppendSysResource(sr string, sys *system.System, return res, nil } +// isExpectedAbsence reports whether an Exists() error means "the lookup ran and +// found nothing" rather than "the lookup could not run". +// +// This is FEAT-010's Trap 1 distinction, applied where AppendSysResourceIfExists +// needs it. Without it, warning on every Exists() error would fire on the most +// ordinary outcome there is: `syver autoadd` walking a path that simply is not a +// separate mount, or a service name that is not registered. Those are answers, +// not failures, and resource/mount.go already excludes ErrMountpointNotFound for +// exactly this reason. +// +// Anything NOT listed here is treated as a real failure and reported. That is the +// safe default: a new sentinel that should be quiet produces one noisy warning, +// whereas a new sentinel wrongly listed here would silently hide a broken lookup, +// which is the defect this whole path exists to remove. +func isExpectedAbsence(err error) bool { + return errors.Is(err, system.ErrMountpointNotFound) || + errors.Is(err, system.ErrServiceNotFound) +} + // AppendSysResourceIfExists is AppendSysResource, but only stores the result // if the underlying system resource actually exists -- used by `syver // autoadd`. The bool return reports whether it existed (and was therefore @@ -64,22 +85,35 @@ func (r ResourceMap[T, ST, PT]) AppendSysResourceIfExists(sr string, sys *system if err != nil { return nil, sysRes, false, err } - // FEAT-010 SW-10 Trap 2 (deliberate, documented deferral -- not missed): - // this is the one Exists()-error-discard site left unfixed by the + // FEAT-010 SW-10 Trap 2, RESOLVED in FEAT-013 (FEAT-011 W2-8): skip the + // entry, and say so. + // + // This was the one Exists()-error-discard site left unfixed by the // otherwise-identical sweep applied to resource/registry.go, group.go, - // interface.go, mount.go and user.go. Unlike those per-type sites, this - // generic fan-out backs `syver autoadd` for all seven auto-addable - // types on every platform -- propagating here would mean a single - // unreadable resource aborts the entire autoadd run instead of skipping - // just that one entry, which is a different (and probably worse) - // failure mode than the other five sites' fix. `add` already reports - // partial results elsewhere, so the likely-correct shape is "keep - // going, surface a warning" -- but that needs a warning channel this - // generic path does not have today, and deciding it needs its own - // review, not a byproduct of this sweep, so it is left as-is. + // interface.go, mount.go and user.go. Those five propagate. This one does + // not, and the difference is deliberate: this generic fan-out backs + // `syver autoadd` for all seven auto-addable types, so propagating would + // abort the whole run because one resource happened to be unreadable. + // Discovering nine resources and failing on the tenth is a worse outcome + // than discovering nine and reporting why the tenth was left out. + // + // The original deferral said the right shape was "keep going, surface a + // warning" but that no warning channel existed here. That is no longer + // true: `log.Printf("[WARN] ...")` is the established convention, used by + // store.go, syver_config.go and cmd/syver. So the entry is skipped AND the + // reason is reported, which is the part that was missing. An unreadable + // resource is now visible in the output rather than being indistinguishable + // from one that genuinely does not exist. exists := false if er, ok := any(sysRes).(system.Resource); ok { - exists, _ = er.Exists() + var err error + exists, err = er.Exists() + if err != nil { + exists = false + if !isExpectedAbsence(err) { + log.Printf("[WARN] autoadd: skipping %q: %v", sr, err) + } + } } if !exists { return res, sysRes, false, nil diff --git a/resource/resource_map_absence_test.go b/resource/resource_map_absence_test.go new file mode 100644 index 0000000..d3d6a63 --- /dev/null +++ b/resource/resource_map_absence_test.go @@ -0,0 +1,60 @@ +package resource + +import ( + "errors" + "fmt" + "testing" + + "github.com/krameff/syver/system" +) + +// FEAT-013 Task 1 / FEAT-011 W2-8. AppendSysResourceIfExists discarded its +// Exists() error entirely, so an unreadable resource was indistinguishable +// from one that genuinely did not exist. It now skips and warns -- but only +// for errors that mean the lookup FAILED, not for the ones that mean it ran +// and found nothing. +// +// Getting that boundary wrong is the whole risk. Warn on too much and every +// `syver autoadd` over an ordinary directory emits noise about paths that are +// not mounts; warn on too little and the original silent skip comes straight +// back. + +func TestIsExpectedAbsenceAcceptsFoundNothing(t *testing.T) { + for _, err := range []error{ + system.ErrMountpointNotFound, + system.ErrServiceNotFound, + fmt.Errorf("wrapped: %w", system.ErrMountpointNotFound), + fmt.Errorf("wrapped: %w", system.ErrServiceNotFound), + } { + if !isExpectedAbsence(err) { + t.Errorf("isExpectedAbsence(%v) = false; this is a lookup that ran and found nothing, and must not warn", err) + } + } +} + +func TestIsExpectedAbsenceRejectsRealFailures(t *testing.T) { + for _, err := range []error{ + system.ErrRegistryUnsupported, + system.ErrMountUnsupported, + system.ErrKernelParamUnsupported, + system.ErrFileOwnershipUnsupported, + errors.New("permission denied"), + errors.New("getMount operation timed out"), + } { + if isExpectedAbsence(err) { + t.Errorf("isExpectedAbsence(%v) = true; this is a lookup that could not run, and must be reported", err) + } + } +} + +// The unsupported sentinels are the case that motivated this. On a platform +// where a resource type is not implemented, every Exists() returns one of +// them, and the old code turned all of them into a silent "does not exist". +func TestUnsupportedSentinelsAreNotTreatedAsAbsence(t *testing.T) { + if isExpectedAbsence(system.ErrMountUnsupported) { + t.Fatal("ErrMountUnsupported was classified as absence; a platform with no mount lookup would autoadd silently") + } + if isExpectedAbsence(system.ErrRegistryUnsupported) { + t.Fatal("ErrRegistryUnsupported was classified as absence; a non-Windows autoadd would hide the reason") + } +} From 011c10ef004933cd7006e1cfd86d4b54ee664df6 Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Tue, 8 Sep 2026 18:30:13 +0100 Subject: [PATCH 06/21] docs(changelog): FEAT-013 Windows correctness cluster Signed-off-by: Mark Bolwell --- CHANGELOG.md | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 83335b3..cf8ca70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,36 @@ resource quietly disappearing shows up as a rise in skips rather than a failure +- windows + - a `mount:` check on Windows said the **mountpoint was not found**, blaming + the path the operator wrote for what is actually a missing implementation. + It now says it is not supported on this platform. The same misleading error + appeared on macOS, which is equally unimplemented, and is fixed there too. + Nothing changes on Linux, where `mount:` is fully supported: the fix is a + platform capability check placed before the shared lookup rather than a + reordering of it, so supported platforms take exactly the path they did + - `process: status` returned an **empty list and no error** on Windows, where + the underlying library cannot read process state at all. The check ran, + found the process, reported nothing about it and passed. It now errors when + every matching process fails to read, while still tolerating the single + process that exits between being listed and being read, which is the case + that skipping was there for. `process: user` had the same shape and gets the + same rule + - a gossfile include written as an absolute Windows path (`C:\...`) was + resolved relative to the including file instead, because the absoluteness + test was a literal check for a leading `/`. Paths beginning `/` still behave + exactly as before on every platform + - `~\Documents\x` did not expand on Windows. Home-directory expansion split + the path on `/` only, so the whole string was read as an account name + +- autoadd + - `syver autoadd` **silently skipped** any resource whose existence check + failed, making an unreadable resource indistinguishable from one that is + genuinely absent. It now reports the reason and carries on, rather than + either hiding it or aborting the whole run over one entry. Lookups that ran + and found nothing, such as a path that is not a mount or a service that is + not registered, stay quiet: those are answers, not failures + ## 0.11.2 based on krameff/goss v0.6.0 - signed SBOMs and a patched base image - supply chain From fa0bed548f93da971809cb39a6c93b76767e457a Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Tue, 8 Sep 2026 18:50:07 +0100 Subject: [PATCH 07/21] test(windows): assert the mount fix end to end, and forbid the silent outcome Signed-off-by: Mark Bolwell --- system/mount_windows_test.go | 104 +++++++++++++++++++++++++++++++ system/process_invariant_test.go | 77 +++++++++++++++++++++++ 2 files changed, 181 insertions(+) create mode 100644 system/mount_windows_test.go create mode 100644 system/process_invariant_test.go diff --git a/system/mount_windows_test.go b/system/mount_windows_test.go new file mode 100644 index 0000000..6a332b7 --- /dev/null +++ b/system/mount_windows_test.go @@ -0,0 +1,104 @@ +//go:build windows +// +build windows + +package system + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + "github.com/krameff/syver/util" +) + +// FEAT-013 Task 4 / FEAT-011 W2-12(a), asserted end to end rather than through +// the helper. +// +// mount_supported_test.go proves mountSupported() returns the right answer. +// That is not the same as proving an operator sees it: the whole defect was +// that the honest error existed and was unreachable, so a test of the honest +// error alone would have passed before this fix as well. +// +// These drive DefMount.Exists(), the path a gossfile actually takes. + +func windowsTestConfig(t *testing.T) util.Config { + t.Helper() + c, err := util.NewConfig() + if err != nil { + t.Fatalf("util.NewConfig(): %v", err) + } + // A zero Timeout makes getMount time out immediately, which would mask + // which error is being returned. + c.Timeout = 5 * time.Second + return *c +} + +// TestMountReportsUnsupportedNotNotFound is the regression this fix exists for. +// +// Before it, every Windows mount check failed with "mountpoint not found", +// because the vendored mountinfo returns an empty table on Windows and +// getMount converts that to ErrMountpointNotFound. The message blamed the +// operator's path for a missing implementation, and for a hardening audit a +// confidently wrong reason is worse than no answer. +func TestMountReportsUnsupportedNotNotFound(t *testing.T) { + // A drive that certainly exists. The point is that even a real, present + // mount point reports unsupported, because there is no backend at all. + for _, mountPoint := range []string{`c:`, `C:\`, `/`} { + t.Run(mountPoint, func(t *testing.T) { + m := NewDefMount(context.Background(), mountPoint, nil, windowsTestConfig(t)) + exists, err := m.Exists() + if err == nil { + t.Fatalf("Exists() on %q returned (%v, nil); Windows has no mount backend and must say so", mountPoint, exists) + } + if errors.Is(err, ErrMountpointNotFound) { + t.Fatalf("Exists() on %q = %v; this is the regression -- it blames the mountpoint for a missing implementation", mountPoint, err) + } + if !errors.Is(err, ErrMountUnsupported) { + t.Fatalf("Exists() on %q = %v, want ErrMountUnsupported", mountPoint, err) + } + }) + } +} + +// TestMountAttributesAlsoReportUnsupported checks the other accessors take the +// same path. Each calls setup() independently, so one of them could report +// differently from Exists() without this noticing. +func TestMountAttributesAlsoReportUnsupported(t *testing.T) { + m := NewDefMount(context.Background(), `c:`, nil, windowsTestConfig(t)) + + if _, err := m.Filesystem(); !errors.Is(err, ErrMountUnsupported) { + t.Errorf("Filesystem() = %v, want ErrMountUnsupported", err) + } + if _, err := m.Usage(); !errors.Is(err, ErrMountUnsupported) { + t.Errorf("Usage() = %v, want ErrMountUnsupported", err) + } + if _, err := m.Opts(); !errors.Is(err, ErrMountUnsupported) { + t.Errorf("Opts() = %v, want ErrMountUnsupported", err) + } + if _, err := m.Source(); !errors.Is(err, ErrMountUnsupported) { + t.Errorf("Source() = %v, want ErrMountUnsupported", err) + } +} + +// TestMountErrorIsActionable guards the operator-facing half. The error is the +// only thing a user sees, and "not supported on this platform" is what tells +// them to stop debugging their path. A future refactor that preserved the +// sentinel identity but lost the wording would pass every test above. +func TestMountErrorIsActionable(t *testing.T) { + m := NewDefMount(context.Background(), `c:`, nil, windowsTestConfig(t)) + _, err := m.Exists() + if err == nil { + t.Fatal("Exists() returned nil error") + } + msg := err.Error() + for _, want := range []string{"mount", "not supported"} { + if !strings.Contains(msg, want) { + t.Errorf("error %q does not mention %q; an operator cannot act on it", msg, want) + } + } + if strings.Contains(msg, "not found") { + t.Errorf("error %q still reads as a missing mountpoint", msg) + } +} diff --git a/system/process_invariant_test.go b/system/process_invariant_test.go new file mode 100644 index 0000000..f150893 --- /dev/null +++ b/system/process_invariant_test.go @@ -0,0 +1,77 @@ +package system + +import ( + "context" + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/krameff/syver/util" +) + +// TestProcessNeverReportsNothingSuccessfully is the invariant FEAT-013 Task 3 +// introduces, written so that it does not presume what the underlying library +// does on any given platform. +// +// The defect was that Status() returned an empty slice AND a nil error on +// Windows, because gopsutil fails for every process there and the loop skipped +// them all. The check ran, found the process, said nothing about it, and +// passed. +// +// Asserting "Status() errors on Windows" would encode a claim about gopsutil +// that has not been measured on a Windows host, and this project has been +// burnt by exactly that kind of assumption. So the assertion is the weaker and +// more durable one: for a process that demonstrably EXISTS, reporting nothing +// while also reporting success is not an available outcome. Either the +// attribute is known, or the reason it is not is. +// +// It runs everywhere on purpose. On Linux and macOS it passes because the +// lookup works, which keeps the invariant honest rather than making it a +// Windows-only special case. +func TestProcessNeverReportsNothingSuccessfully(t *testing.T) { + exe, err := os.Executable() + if err != nil { + t.Skipf("cannot determine own executable: %v", err) + } + name := filepath.Base(exe) + + cfg, err := util.NewConfig() + if err != nil { + t.Fatalf("util.NewConfig(): %v", err) + } + + p := NewDefProcess(context.Background(), name, New(""), *cfg) + + exists, err := p.Exists() + if err != nil { + t.Skipf("process lookup itself failed on %s (%v); this test has nothing to assert about", runtime.GOOS, err) + } + if !exists { + t.Skipf("could not find own process %q in the table on %s; environment-dependent, not a defect", name, runtime.GOOS) + } + + t.Run("status", func(t *testing.T) { + got, err := p.Status() + if err != nil { + t.Logf("Status() on %s reported: %v", runtime.GOOS, err) + return + } + if len(got) == 0 { + t.Fatalf("Status() returned an empty result AND a nil error for a process that exists; "+ + "that is the silent-nothing outcome FEAT-013 removed (GOOS=%s)", runtime.GOOS) + } + }) + + t.Run("user", func(t *testing.T) { + got, err := p.User() + if err != nil { + t.Logf("User() on %s reported: %v", runtime.GOOS, err) + return + } + if len(got) == 0 { + t.Fatalf("User() returned an empty result AND a nil error for a process that exists; "+ + "same silent-nothing outcome (GOOS=%s)", runtime.GOOS) + } + }) +} From 87b5efa3031a41e1550ef092c7422eff6327e871 Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Wed, 9 Sep 2026 07:15:14 +0100 Subject: [PATCH 08/21] fix(autoadd): honour --log-level, as every other subcommand does Signed-off-by: Mark Bolwell --- add.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/add.go b/add.go index f2fbdd7..58f369b 100644 --- a/add.go +++ b/add.go @@ -84,6 +84,17 @@ func AddResource(fileName string, syverConfig SyverConfig, resourceName, key str // AutoAddResources is a simple wrapper to add multiple resources func AutoAddResources(fileName string, keys []string, c *util.Config) error { var err error + // autoadd was the one subcommand that never installed the level filter. + // AddResources, validate and serve all call this; autoadd did not, and it + // did not matter while nothing here logged. FEAT-013 added a [WARN] for a + // resource skipped because its lookup failed, which made it matter: without + // this, `syver -L ERROR autoadd ...` could not silence that line, and it + // printed in Go's raw default format rather than the RFC3339 one every + // other [WARN] in this codebase uses. + err = setLogLevel(c) + if err != nil { + return err + } outStoreFormat, err = getStoreFormatFromFileName(fileName) if err != nil { return err From 083f2ab273f4465144ceb21e3344c618209cdec0 Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Wed, 9 Sep 2026 07:15:37 +0100 Subject: [PATCH 09/21] test(process): pin the n=1 boundary the all-or-nothing rule cannot express Signed-off-by: Mark Bolwell --- system/process.go | 16 ++++++++++++++-- system/process_collect_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/system/process.go b/system/process.go index dc4d0d6..a8094b9 100644 --- a/system/process.go +++ b/system/process.go @@ -103,12 +103,24 @@ func (p *DefProcess) User() ([]string, error) { // indistinguishable from an honest empty result unless the failures are // counted. // -// The rule is deliberately "all of them" rather than "any of them": a systemic -// failure is universal, whereas the race this tolerates is not. It needs no +// The rule is deliberately "all of them" rather than "any of them". It needs no // platform check and no gopsutil sentinel, which matters because gopsutil's // ErrNotImplementedError lives in an internal package and cannot be compared // against from here. // +// KNOW THE BOUNDARY, because an earlier version of this comment overstated it. +// "All of them failed" and "the one race case" are THE SAME EVENT when exactly +// one process matches, which is the common case for a single-instance daemon. +// So a singleton whose read races DOES surface as an error here; it is not +// tolerated. The rule cannot distinguish the two at n=1 and no counting rule +// could. +// +// That is accepted rather than worked around. The alternative is returning an +// empty result with a nil error for a process that demonstrably exists, which +// is precisely the silent-nothing outcome this function was written to remove. +// An error naming the attribute and the cause is the better failure. See +// TestCollectPerProcessCannotTellARaceFromSystemicAtOne, which pins it. +// // User() gets the same treatment as Status(), not because it is known broken // anywhere, but because the shape was identical and leaving one of two // adjacent silent-pass paths in place is an arbitrary line to draw. diff --git a/system/process_collect_test.go b/system/process_collect_test.go index d059eea..0d4c71b 100644 --- a/system/process_collect_test.go +++ b/system/process_collect_test.go @@ -59,6 +59,36 @@ func TestCollectPerProcessFailsWhenEveryReadFails(t *testing.T) { } } +// TestCollectPerProcessCannotTellARaceFromSystemicAtOne pins the boundary the +// all-or-nothing rule cannot express, found by review rather than by design. +// +// With exactly one matching process -- the common case for a single-instance +// daemon -- "every read failed" and "the one read raced" are the same event. +// The transient failure the rule claims to tolerate is therefore NOT tolerated +// at n=1. That is accepted: the alternative is an empty result with a nil error +// for a process that demonstrably exists, which is the silent-nothing outcome +// this function exists to remove. +// +// This test exists so the boundary is stated rather than discovered. If a +// future change makes n=1 tolerant again, it fails and forces the choice to be +// deliberate. +func TestCollectPerProcessCannotTellARaceFromSystemicAtOne(t *testing.T) { + race := errors.New("process vanished between listing and reading") + got, err := collectPerProcess([]*process.Process{{Pid: 1}}, "status", + func(*process.Process) ([]string, error) { return nil, race }) + + if err == nil { + t.Fatalf("n=1 with a racing read returned %v and a nil error; that is the "+ + "silent-nothing outcome, not tolerance", got) + } + if !errors.Is(err, race) { + t.Errorf("error %v does not wrap the underlying cause", err) + } + if !strings.Contains(err.Error(), "all 1 matching processes") { + t.Errorf("error %q should say how many processes it was speaking for", err) + } +} + // An executable with no matching processes is not a failure. `Exists()` is what // answers that question, and making Status() error here would turn an ordinary // absent-process assertion into a hard failure. From e6b179c7fb83a518930ff9a4e686366d6a61a66e Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Wed, 9 Sep 2026 07:16:03 +0100 Subject: [PATCH 10/21] docs: correct four false statements found in review Signed-off-by: Mark Bolwell --- CHANGELOG.md | 37 ++++++++++++++----- docs/windows.md | 14 +++++-- .../syver/windows/tests/mount.goss.yaml | 19 +++++++--- resource/resource_map_absence_test.go | 32 ++++++++++++++-- 4 files changed, 78 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf8ca70..c10816b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,26 +68,43 @@ - windows - a `mount:` check on Windows said the **mountpoint was not found**, blaming the path the operator wrote for what is actually a missing implementation. - It now says it is not supported on this platform. The same misleading error - appeared on macOS, which is equally unimplemented, and is fixed there too. - Nothing changes on Linux, where `mount:` is fully supported: the fix is a - platform capability check placed before the shared lookup rather than a - reordering of it, so supported platforms take exactly the path they did + It now says it is not supported on this platform. This is Windows-only: + `mount:` is already fully supported on macOS, through the same POSIX lookup + Linux uses, so neither platform's behaviour changes. The fix is a platform + capability check placed before the shared lookup rather than a reordering + of it, so supported platforms take exactly the path they did - `process: status` returned an **empty list and no error** on Windows, where the underlying library cannot read process state at all. The check ran, found the process, reported nothing about it and passed. It now errors when - every matching process fails to read, while still tolerating the single - process that exits between being listed and being read, which is the case - that skipping was there for. `process: user` had the same shape and gets the - same rule + every matching process fails to read, while still tolerating a process that + exits between being listed and being read when others were read + successfully. **Note the boundary:** where exactly one process matches, + which is the common case for a single-instance daemon, those two are the + same event and the check errors rather than tolerating it. That is the + intended trade: an error naming the cause is better than an empty result + reported as success for a process that demonstrably exists. + `process: user` had the same shape and gets the same rule + - `syver add mount` on Windows now **fails** instead of silently writing an + `exists: false` entry it never verified. It is the same fix seen from the + `add` side: the old mountpoint-not-found error was excluded from + propagation as an ordinary "not a mount" answer, and the honest + not-supported error is not - a gossfile include written as an absolute Windows path (`C:\...`) was resolved relative to the including file instead, because the absoluteness test was a literal check for a leading `/`. Paths beginning `/` still behave - exactly as before on every platform + exactly as before on every platform. **A UNC path (`\\server\share\...`) + now resolves too**, where it was previously joined onto the including file's + directory and silently failed to resolve. That follows from using the + platform's own definition of absolute, and makes a gossfile on a Windows + file share usable as a shared include - `~\Documents\x` did not expand on Windows. Home-directory expansion split the path on `/` only, so the whole string was read as an account name - autoadd + - `syver autoadd` now honours `--log-level` / `SYVER_LOGLEVEL`, and its + output carries the same timestamped format as every other subcommand. It + was the one verb that never installed the level filter, which did not matter + while nothing in that path logged. The warning below made it matter - `syver autoadd` **silently skipped** any resource whose existence check failed, making an unreadable resource indistinguishable from one that is genuinely absent. It now reports the reason and carries on, rather than diff --git a/docs/windows.md b/docs/windows.md index 74d8101..73acd3b 100644 --- a/docs/windows.md +++ b/docs/windows.md @@ -112,8 +112,12 @@ token, and every Windows token carries a mandatory integrity label than returning a wrong or empty list, so it will not mislead you, but do not use `groups:` in a Windows spec. -**`process:` `status`.** Returns an empty list with no error when the per-process -lookup fails, so an assertion can pass having learned nothing. +**`process:` `status`.** Every assertion errors, because gopsutil has no +Windows implementation for this field and every matching process therefore +fails to read. It used to return an empty list with no error instead, so an +assertion could pass having learned nothing; it now fails loudly rather than +silently. `process:` `user` shares the same all-or-nothing rule but is +unaffected in practice: it works on Windows. ### Not implemented yet @@ -123,8 +127,10 @@ flag to explicitly set it`. It does not silently answer "not installed", which it did before this release. A backend over the Add/Remove Programs registry hives is planned. -**`mount:`.** Reports a mountpoint-not-found error rather than an -unimplemented one. Loud, but it blames the wrong thing. +**`mount:`.** Every assertion errors with "not supported on this platform". +It used to report a mountpoint-not-found error instead -- loud, but blaming +the operator's path for what was actually a missing implementation. A real +backend over `GetLogicalDriveStringsW` and related Win32 calls is planned. ## What is actually tested diff --git a/integration-tests/syver/windows/tests/mount.goss.yaml b/integration-tests/syver/windows/tests/mount.goss.yaml index e5780ed..4ef6869 100644 --- a/integration-tests/syver/windows/tests/mount.goss.yaml +++ b/integration-tests/syver/windows/tests/mount.goss.yaml @@ -3,12 +3,19 @@ # different number of assertions on a real Windows host than when # driven from another platform. Seed it from a Windows run only. --- -# NOT touched by FEAT-010. SW-7 (mount_windows.go's honest errNotImplemented -# is unreachable -- system/mount.go's getMount fails first with the -# misleading-but-loud ErrMountpointNotFound) was deliberately deferred to -# FEAT-011: reaching errNotImplemented needs reordering getMount, which is -# cross-platform code shared by every OS and not "obviously safe" to -# restructure within this spec's scope. See system/mount_windows.go. +# SW-7 IS RESOLVED. This comment used to say the honest "not implemented" +# error was unreachable, because system/mount.go's getMount failed first with +# the misleading ErrMountpointNotFound, and that FEAT-011 would fix it by +# reordering getMount. FEAT-013 resolved it WITHOUT that reorder: a +# mountSupported() capability check now runs before getMount, so a Windows +# mount check reports ErrMountUnsupported. The reorder was rejected because +# getMount is shared by every OS and its order decides which error a missing +# path produces where mount: actually works. See system/mount_windows.go. +# +# STILL SKIPPED, and for a different reason now. There is no Windows backend, +# so this asserts nothing. Un-skipping it is FEAT-018's acceptance criterion, +# at which point it also needs an `# expect-skipped:` seeded from a real +# Windows run -- not inferred from another platform. mount: 'c:': exists: true diff --git a/resource/resource_map_absence_test.go b/resource/resource_map_absence_test.go index d3d6a63..16fd8b2 100644 --- a/resource/resource_map_absence_test.go +++ b/resource/resource_map_absence_test.go @@ -14,10 +14,34 @@ import ( // for errors that mean the lookup FAILED, not for the ones that mean it ran // and found nothing. // -// Getting that boundary wrong is the whole risk. Warn on too much and every -// `syver autoadd` over an ordinary directory emits noise about paths that are -// not mounts; warn on too little and the original silent skip comes straight -// back. +// Getting that boundary wrong is the whole risk: warn on too much and autoadd +// emits noise for outcomes that are answers rather than failures; warn on too +// little and the original silent skip comes straight back. +// +// CORRECTION, 2026-09-08, after Vision checked the basis rather than the +// pattern. An earlier version of this comment justified the mount case as +// "every `syver autoadd` over an ordinary directory emits noise about paths +// that are not mounts". That scenario cannot occur. `mount:` has no +// `AutoAddSpec` -- only file, group, package, port, process, service and user +// do -- so `ErrMountpointNotFound` never reaches this function today, and +// `file:`'s Exists() is an os.Lstat that touches no mount lookup at all. +// +// SECOND CORRECTION, same day, same mistake one level down. The line above +// originally went on to claim `ErrServiceNotFound` IS reachable, because +// `service:` has an AutoAddSpec and returns that sentinel. Wrong again, and +// wrong the same way: it checked that the type is autoadd-capable and that the +// sentinel exists, without checking WHICH METHOD returns it. +// `AppendSysResourceIfExists` calls only `Exists()`, and +// `ServiceWindows.Exists()` returns `(false, nil)` for a missing service +// (system/service_windows.go). `ErrServiceNotFound` comes from `Enabled()` and +// `Running()`, which this path never calls. +// +// So BOTH listed sentinels are currently unreachable from the only call site, +// and this function is forward-looking insurance rather than live protection. +// It is kept deliberately: FEAT-018 makes the mount branch live, and the safe +// direction is to over-list, since an unlisted sentinel costs one noisy warning +// while a wrongly-listed one silently hides a broken lookup. But do not read +// the comment above as describing something that fires today. func TestIsExpectedAbsenceAcceptsFoundNothing(t *testing.T) { for _, err := range []error{ From 71543356849478ec03db9879faab843267daf393 Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Wed, 9 Sep 2026 07:31:55 +0100 Subject: [PATCH 11/21] docs(platforms): record the Windows port and process findings as measured Signed-off-by: Mark Bolwell --- CHANGELOG.md | 8 ++++++++ docs/platforms.md | 22 ++++++++++++++++++++-- docs/windows.md | 10 ++++++++-- 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c10816b..1fb86fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,14 @@ what a registry can read about them without pulling one - docs + - the Windows support matrix now records what was **measured** rather than + what was assumed. `port:` errors with `not implemented yet` on Windows: + gopsutil ships a Windows backend for connection enumeration, so the row had + recorded an assumption nobody had checked. `process: status` reads *not + implemented* rather than *broken*, because that is what the library reports. + `process: user` **works**, and had read *no data*. The page says where each + came from, and notes that the `port:` and `mount:` fixtures are skipped, so + a green Windows suite does not cover them - three documentation pages existed but were unreachable from the documentation index: **goss vs Syver**, **Windows** and **Testing**. They are now listed. Navigation on the published site was unaffected, since it is diff --git a/docs/platforms.md b/docs/platforms.md index ba8b592..30fe856 100644 --- a/docs/platforms.md +++ b/docs/platforms.md @@ -57,6 +57,24 @@ This matrix attempts to track parity across platforms. | {{ n_a }} | Not applicable for this platform | | {{ no_data }} | Not yet tried, no data | +!!! note "Windows `port:`, `process:` and `mount:` are measured, not assumed" + + The Windows cells for these three were verified on Windows Server 2025 on + 2026-09-09, not inferred from reading the code: + + * `port:` errors with `not implemented yet`. gopsutil ships a Windows + backend, so this row previously recorded an assumption nobody had + checked. It is now confirmed. + * `process:` `status` errors the same way, which is why it reads + *not implemented* rather than *broken*: the library says so itself. + * `process:` `user` **works**, and previously read *no data*. + * `mount:` errors with `not supported on this platform`. It used to blame + the mountpoint instead. + + Re-derive rather than trust this note. Build a Windows binary and run the + assertion; the fixtures for `port:` and `mount:` are deliberately skipped + because they would fail, so a green Windows suite does not cover them. + !!! note "About partial support" This is ambiguous. Where you see this, check into the test coverage within `integration-tests/syver/{darwin|windows}/{test}.goss.yaml` for more detail. @@ -130,8 +148,8 @@ This matrix attempts to track parity across platforms. | | ip | {{ fully_supported }} | {{ no_data }} | {{ not_implemented }} | | **process** | | {{ fully_supported }} | {{ work_partially }} | {{ work_partially }} | | | running | {{ fully_supported }} | {{ work_partially }} | {{ work_partially }} | -| | status | {{ fully_supported }} | {{ work_partially }} | {{ broken }} | -| | user | {{ fully_supported }} | {{ work_partially }} | {{ no_data }} | +| | status | {{ fully_supported }} | {{ work_partially }} | {{ not_implemented }} | +| | user | {{ fully_supported }} | {{ work_partially }} | {{ work_partially }} | | **service** | | {{ fully_supported }} | {{ not_implemented }} | {{ work_partially }} | | | enabled | {{ fully_supported }} | {{ not_implemented }} | {{ work_partially }} | | | running | {{ fully_supported }} | {{ not_implemented }} | {{ work_partially }} | diff --git a/docs/windows.md b/docs/windows.md index 73acd3b..bbefe03 100644 --- a/docs/windows.md +++ b/docs/windows.md @@ -61,8 +61,8 @@ The legacy `GOSS_*` names are still honoured, and an exported-but-empty ## What works `file:` (existence, contents, size), `command:`, `http:`, `dns:`, `addr:`, -`process:`, `service:`, `registry:`, and `exists` on `user:`, `group:` and -`interface:`. +`process:` (`running` and `user`, but not `status`), `service:`, `registry:`, +and `exists` on `user:`, `group:` and `interface:`. `registry:` is Windows-only, and is the resource most worth using here. @@ -127,6 +127,12 @@ flag to explicitly set it`. It does not silently answer "not installed", which it did before this release. A backend over the Add/Remove Programs registry hives is planned. +**`port:`.** Every assertion errors with `not implemented yet`. gopsutil ships +a Windows backend for connection enumeration, so this one looked like it might +already work and nobody had checked; measured on Windows Server 2025 on +2026-09-09, it does not. The fixture stays skipped because un-skipping it would +fail rather than reveal anything new. + **`mount:`.** Every assertion errors with "not supported on this platform". It used to report a mountpoint-not-found error instead -- loud, but blaming the operator's path for what was actually a missing implementation. A real From 5462665ed181ed2409823edaaddc529f303547d3 Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Wed, 9 Sep 2026 07:43:17 +0100 Subject: [PATCH 12/21] docs(releases): name both signing keys and say which signs what Signed-off-by: Mark Bolwell --- RELEASES.md | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/RELEASES.md b/RELEASES.md index e6d676f..0379117 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -20,7 +20,29 @@ are this project's own, cut after the fork. Releases are assembled on `devel` and merged to `main` at release time, so from 0.9.1 onward a release carries several branches rather than one. -Every tag from v0.7.0 onward is annotated and GPG-signed with the same key. +Every tag from v0.7.0 onward is annotated and GPG-signed. + +**Two different keys sign two different things, and confusing them makes a good +signature look like a bad one.** + +| What | Signed by | Key | +| --- | --- | --- | +| Git tags | the maintainer's own key | `5154CE6E4F8712D87B9C870DCC071079D4E84F77` | +| Release artifacts: `SHA256SUMS`, the SBOMs | the project signing key, published as [`krameff-syver-key.asc`](krameff-syver-key.asc) | `CD218D529C95DC65A71F18D84C9E5095CABE5092` | + +So importing `krameff-syver-key.asc` and then running `git verify-tag` will +report that it has no public key for the signature. That is expected, not a +problem with the tag. Verify each with the key that signed it: + +```sh +git verify-tag v0.11.2 # maintainer key +gpg --verify syver_0.11.2_SHA256SUMS.sig \ + syver_0.11.2_SHA256SUMS # project key +``` + +This line previously read "signed with the same key", which had no antecedent +and invited exactly that mistake. + "Released" is the date the tag object was created, which is not always the commit date: v0.7.0 was committed on 2026-08-18 and tagged on 2026-08-20. v0.6.0 has no tag in this repository and uses the date its changelog entry From 17a3568ca7a9068ddbaa1261c948a681c4a3a970 Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Wed, 9 Sep 2026 14:26:26 +0100 Subject: [PATCH 13/21] fix(windows): kill the whole process tree when a command times out Signed-off-by: Mark Bolwell --- system/service_windows.go | 16 ++-- util/command.go | 18 ++++ util/command_windows.go | 11 +++ util/procgroup_posix.go | 10 +++ util/procgroup_windows.go | 180 ++++++++++++++++++++++++++++++++++++-- 5 files changed, 220 insertions(+), 15 deletions(-) diff --git a/system/service_windows.go b/system/service_windows.go index 93a9fa0..c1d0640 100644 --- a/system/service_windows.go +++ b/system/service_windows.go @@ -30,8 +30,9 @@ func NewServiceWindows(ctx context.Context, service string, system *System, conf // so cannot go through util.NewCommandContext. Same contract: the caller's // context, a bounded lifetime, and an error only when the context ended the run. // -// Note the process-group caveat in util/procgroup_windows.go -- on Windows the -// started powershell is killed, but a grandchild it spawned can survive. +// The process tree is now bounded: NewCommandForWindowsPowershellContext puts +// the powershell into a Job Object, so cancellation terminates a grandchild it +// spawned rather than leaving it running. See util/procgroup_windows.go. func runHelperPowershell(ctx context.Context, name string, arg ...string) (*util.Command, error) { if ctx == nil { ctx = context.Background() @@ -45,11 +46,12 @@ func runHelperPowershell(ctx context.Context, name string, arg ...string) (*util // once: the bound was added to runHelperCommand and this copy was missed, // leaving the Windows service path with cancellation and no I/O bound. // - // The consequence is worse here than on POSIX rather than merely equal. - // util/procgroup_windows.go is a documented no-op, so there is no process - // group to kill and ANY grandchild survives -- not just one that deliberately - // detached. Every powershell that spawns something outliving it therefore - // takes the wedge path, where on Linux it takes a setsid to get there. + // This used to be worse here than on POSIX: util/procgroup_windows.go was a + // documented no-op, so ANY grandchild survived, not just one that had + // deliberately detached. FEAT-017 replaced that no-op with a Job Object and + // wired this constructor into it, so the tree is killed. The I/O bound below + // is still required -- a job kills processes, it does not unblock a read on + // a pipe handle the parent still holds. // // helperIOGrace, matching runHelperCommand. See that var's comment for why // this path deliberately does not derive the grace from the deadline. diff --git a/util/command.go b/util/command.go index a51cc41..f593b13 100644 --- a/util/command.go +++ b/util/command.go @@ -58,6 +58,17 @@ func (c *Command) Run() error { c.Cmd.Stdout = &c.Stdout c.Cmd.Stderr = &c.Stderr + // FIRST, and before the LookPath early return below: on Windows + // configureProcessGroup has already created a Job Object handle, and every + // path out of this function has to give it back. Releasing is idempotent and + // a no-op when nothing was registered, so this is safe on the paths that + // never start a process and on POSIX, where both hooks do nothing. + // + // kill=false because this is the ORDINARY exit. Terminating the tree is + // cmd.Cancel's job and happens only on a timeout; doing it here would kill a + // background process a successful command deliberately started. + defer releaseProcessGroup(c.Cmd, false) + if _, err := exec.LookPath(c.name); err != nil { c.Err = err return c.Err @@ -68,6 +79,13 @@ func (c *Command) Run() error { return c.Err } + // A no-op unless configureProcessGroup registered something, so a + // context-free NewCommand is unaffected. On Windows this is where the + // process joins its Job Object -- the earliest point at which a process + // exists to assign. An assignment failure is not a check failure: the + // command ran, and cmd.Cancel falls back to killing the direct child. + _ = attachProcessGroup(c.Cmd) + if err := c.Cmd.Wait(); err != nil { c.Err = err if exiterr, ok := err.(*exec.ExitError); ok { diff --git a/util/command_windows.go b/util/command_windows.go index 5e880f5..2705ddc 100644 --- a/util/command_windows.go +++ b/util/command_windows.go @@ -46,6 +46,12 @@ func NewCommandForWindowsCmdContext(ctx context.Context, name string, arg ...str CmdLine: strings.Join(arg, " "), CreationFlags: 0, } + // AFTER SysProcAttr, which is assigned wholesale just above and would + // otherwise discard anything set before it. This constructor is the + // `command:` path on Windows and does not go through NewCommandContext, so + // without this line the Job Object hook never runs on the one path whose + // grandchildren leak. + configureProcessGroup(command.Cmd) return command } @@ -70,6 +76,11 @@ func NewCommandForWindowsPowershellContext(ctx context.Context, name string, arg CmdLine: cmdLine, CreationFlags: 0, } + // Same reason as NewCommandForWindowsCmdContext. system/service_windows.go + // carries a comment saying this path has no process-group protection at + // all; this is the line that makes that comment obsolete, and that comment + // is updated to match. + configureProcessGroup(command.Cmd) return command } diff --git a/util/procgroup_posix.go b/util/procgroup_posix.go index d1756bf..5e961e2 100644 --- a/util/procgroup_posix.go +++ b/util/procgroup_posix.go @@ -45,3 +45,13 @@ func configureProcessGroup(cmd *exec.Cmd) { return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) } } + +// attachProcessGroup is a no-op on POSIX. The group is configured before Start +// via SysProcAttr.Setpgid, so there is nothing to do once the process exists. +// It exists so that util.Command.Run has one shape on every platform; Windows +// cannot assign a process to its Job Object until Start has run. +func attachProcessGroup(cmd *exec.Cmd) error { return nil } + +// releaseProcessGroup is a no-op on POSIX. Killing the group is driven entirely +// by cmd.Cancel above, and there is no handle to release afterwards. +func releaseProcessGroup(cmd *exec.Cmd, kill bool) error { return nil } diff --git a/util/procgroup_windows.go b/util/procgroup_windows.go index ce879d1..82d4331 100644 --- a/util/procgroup_windows.go +++ b/util/procgroup_windows.go @@ -3,13 +3,177 @@ package util -import "os/exec" +import ( + "os/exec" + "sync" + "unsafe" -// configureProcessGroup is a deliberate no-op on Windows. + "golang.org/x/sys/windows" +) + +// jobState is what this file has to remember between configureProcessGroup, +// which runs before Start, and the two hooks that run after it. +// +// attached matters as much as the handle. If assignment failed there is no tree +// to kill, and closing an empty job would kill nothing at all -- which would be +// worse than the behaviour this replaces, because we have already taken over +// exec.CommandContext's default cancel. The fallback path depends on knowing +// which of the two happened. +type jobState struct { + handle windows.Handle + attached bool +} + +// jobs maps a command to its Job Object. +// +// The handle cannot live on exec.Cmd, and it cannot live on util.Command +// either: Command is compiled on every platform, so a windows.Handle field +// would need a build tag on the struct itself. Keying off the *exec.Cmd keeps +// the whole mechanism inside this file, which is the property +// procgroup_posix.go already has. +var jobs sync.Map // map[*exec.Cmd]*jobState + +// configureProcessGroup creates a Job Object for the command and makes context +// cancellation terminate every process in its tree. +// +// Windows has no process group to signal. The equivalent is a Job Object with +// JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE: when the last handle to the job closes, +// the kernel terminates every process still assigned to it. That closes the +// case exec.CommandContext misses, where the started process is killed but a +// grandchild it spawned survives. It is not a hypothetical here -- `command:` +// runs through `cmd /c`, so the process syver starts is the shell and the thing +// that hangs is the shell's own child. // -// There is no process-group kill equivalent; terminating a whole tree needs a -// Job Object, which is a larger change than this fix warrants and cannot be -// tested from here. Windows therefore keeps exec.CommandContext's default -// behaviour: the started process is killed on timeout, but a grandchild it -// spawned can still survive. Windows support is alpha (see docs/platforms.md). -func configureProcessGroup(cmd *exec.Cmd) {} +// This is STRONGER than the POSIX side, and deliberately so. procgroup_posix.go +// records a known limitation: a process that calls setsid leaves the process +// group and is permanently out of reach, measured on this tree. A process +// cannot leave a job unless it was created with CREATE_BREAKAWAY_FROM_JOB and +// the job itself permits breakaway. Neither is set here, so the daemonising +// case POSIX cannot close is closed on Windows. +// +// Assignment happens later, in attachProcessGroup, because there is no process +// to assign until Start has run. +func configureProcessGroup(cmd *exec.Cmd) { + job, err := windows.CreateJobObject(nil, nil) + if err != nil { + // No job means no tree kill, so leave exec.CommandContext's default + // cancel in place: it still kills the direct child. Degrading to the + // old behaviour beats refusing to run the check. + return + } + + info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{ + BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{ + LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + }, + } + if _, err := windows.SetInformationJobObject( + job, + windows.JobObjectExtendedLimitInformation, + uintptr(unsafe.Pointer(&info)), + uint32(unsafe.Sizeof(info)), + ); err != nil { + windows.CloseHandle(job) + return + } + + state := &jobState{handle: job} + jobs.Store(cmd, state) + + cmd.Cancel = func() error { + if !state.attached { + // The job is empty, so closing it terminates nothing. Fall back to + // what exec.CommandContext would have done unaided. + if cmd.Process == nil { + return releaseProcessGroup(cmd, false) + } + err := cmd.Process.Kill() + releaseProcessGroup(cmd, false) + return err + } + // Closing the last handle is the kill. Nothing needs signalling first, + // and the child does not have to be reaped for it to take effect. + return releaseProcessGroup(cmd, true) + } +} + +// attachProcessGroup assigns the started process to its Job Object. It is +// called immediately after Start, which is the earliest moment a process exists +// to assign. +// +// A grandchild spawned in the window between Start returning and this call +// would escape the job. The window cannot be closed without starting the +// process suspended and resuming it by hand, which os/exec gives no way to do. +// It is recorded rather than hidden. +// +// MEASURED, rather than reasoned about, on win11-test (Windows 11 Enterprise +// 10.0.26200) on 2026-09-09, over 200 runs: the window between Start returning +// and the assign completing had a median below the clock's resolution and a +// maximum of 559us, while the earliest a freshly created child reached its own +// first statement was 4.05ms -- and that was a bare .exe launched directly, +// which is the fastest case there is. `command:` goes through `cmd /c`, which +// is slower still. The child lost every one of the 200 races. +// +// That is why there is NO TEST for this window and should not be one. A test +// would have to lose a race it cannot lose, so it would pass unconditionally -- +// including against an implementation that created no job at all, which is the +// definition of a test that proves nothing. +func attachProcessGroup(cmd *exec.Cmd) error { + v, ok := jobs.Load(cmd) + if !ok { + return nil + } + state := v.(*jobState) + + if cmd.Process == nil { + return nil + } + + h, err := windows.OpenProcess( + windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, + false, + uint32(cmd.Process.Pid), + ) + if err != nil { + return err + } + defer windows.CloseHandle(h) + + if err := windows.AssignProcessToJobObject(state.handle, h); err != nil { + return err + } + state.attached = true + return nil +} + +// releaseProcessGroup closes the Job Object handle. It is idempotent: cancel +// and the deferred cleanup in Run both reach it, and only the first does work. +// +// kill decides whether closing the handle is allowed to terminate what is still +// running. On timeout it must (that is the whole point). On NORMAL completion +// it must NOT: a command that deliberately starts a background process and +// exits zero is a legitimate thing for a `command:` check to do, and killing +// its child on the way out would be a new failure this spec never asked for. +// Clearing the limit flag before closing is what separates the two. +func releaseProcessGroup(cmd *exec.Cmd, kill bool) error { + v, ok := jobs.LoadAndDelete(cmd) + if !ok { + return nil + } + state := v.(*jobState) + + if !kill { + info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{} + // Best effort. If clearing the flag fails the close below may terminate + // a survivor, which is the wrong answer but not one worth failing a + // passing check over. + windows.SetInformationJobObject( + state.handle, + windows.JobObjectExtendedLimitInformation, + uintptr(unsafe.Pointer(&info)), + uint32(unsafe.Sizeof(info)), + ) + } + + return windows.CloseHandle(state.handle) +} From 9cb5bce1431b9f057d55f389e0a04e9755370b49 Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Wed, 9 Sep 2026 14:26:51 +0100 Subject: [PATCH 14/21] test(windows): prove a grandchild dies on timeout and survives success Signed-off-by: Mark Bolwell --- util/procgroup_windows_test.go | 128 +++++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 util/procgroup_windows_test.go diff --git a/util/procgroup_windows_test.go b/util/procgroup_windows_test.go new file mode 100644 index 0000000..d8b5210 --- /dev/null +++ b/util/procgroup_windows_test.go @@ -0,0 +1,128 @@ +//go:build windows +// +build windows + +package util + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" +) + +// TestTimeoutKillsGrandchild is the FEAT-017 acceptance criterion, and it is +// the reason the Job Object exists rather than being a tidier way to spell the +// old no-op. +// +// The shape matters. `command:` runs through `cmd /c`, so the process syver +// starts is a shell; exec.CommandContext kills that shell and stops. Anything +// the shell launched with `start /b` is detached from it and, before this +// change, outlived the whole run. That is the FEAT-008 leak, which was closed +// on POSIX and left open here. +// +// Liveness is probed with a marker file rather than by enumerating processes. +// Reading another process's command line on Windows needs WMI or +// NtQueryInformationProcess, and a test that needs its own privileged lookup to +// decide whether it passed is a test that can be wrong in two places. A file +// that does or does not appear cannot be misread. +// +// REVERT-PROOF: with configureProcessGroup restored to its old empty body the +// grandchild survives the timeout, writes the marker, and this fails. Confirm +// that before trusting it. +func TestTimeoutKillsGrandchild(t *testing.T) { + if testing.Short() { + t.Skip("waits on a real timeout and a survival window; not a -short test") + } + + dir := t.TempDir() + started := filepath.Join(dir, "grandchild-started.txt") + survived := filepath.Join(dir, "grandchild-survived.txt") + + // TWO markers, and the first one is what stops this test proving nothing. + // + // Asserting only that "survived" is absent passes just as happily when the + // grandchild was killed as when it NEVER RAN -- a typo in the script, a + // `start /b` that failed, a cmd.exe that is not where we thought. Both look + // identical from the outside: no file. So the grandchild announces itself + // BEFORE it waits, and the test requires that announcement. Absence of + // "survived" only means anything once "started" proves there was something + // to kill. + // + // The parent shell launches it detached and then blocks itself, so the run + // reaches its timeout with both alive. ping is the wait: present on every + // Windows host, and no quoting gymnastics through SysProcAttr.CmdLine. + script := "start /b cmd /c \"echo started > " + started + + " & ping -n 8 127.0.0.1 >nul & echo survived > " + survived + "\"" + + " & ping -n 8 127.0.0.1 >nul" + + ctx, cancel := context.WithTimeout(context.Background(), 1500*time.Millisecond) + defer cancel() + + start := time.Now() + cmd := NewCommandForWindowsCmdContext(ctx, "cmd", "/c", script) + _ = cmd.Run() // a killed command reports an error; that is not what is under test + + if elapsed := time.Since(start); elapsed > 6*time.Second { + t.Fatalf("Run took %v, so the command was not interrupted by the context; "+ + "the test has not exercised the timeout path at all", elapsed) + } + + // Outlive the grandchild's own wait. If it is alive, this is when it writes. + time.Sleep(12 * time.Second) + + // Order matters: establish that there WAS a grandchild before concluding + // anything from the absence of the second marker. + if _, err := os.Stat(started); err != nil { + t.Fatalf("the grandchild never announced itself (%v), so this test has "+ + "proved nothing about killing it -- fix the script rather than "+ + "reading the missing survival marker as success", err) + } + + if _, err := os.Stat(survived); err == nil { + t.Fatalf("grandchild wrote %s after the parent was killed, so it survived "+ + "the timeout: the process tree was not terminated", survived) + } else if !os.IsNotExist(err) { + t.Fatalf("cannot tell whether the grandchild survived: %v", err) + } +} + +// TestNormalExitDoesNotKillSurvivors pins the boundary the kill-on-close flag +// would otherwise cross, and it is the reason releaseProcessGroup takes a kill +// argument at all. +// +// A `command:` that deliberately starts something and exits zero is legitimate. +// Closing the Job Object handle on that path would terminate what it started, +// which is a NEW failure this spec never asked for and which no existing test +// would have caught. Clearing the limit flag before the close is what separates +// "the command timed out" from "the command finished". +// +// REVERT-PROOF: drop the `if !kill` branch in releaseProcessGroup and this +// fails while TestTimeoutKillsGrandchild still passes. The two together are +// what pin the behaviour; either alone permits a wrong implementation. +func TestNormalExitDoesNotKillSurvivors(t *testing.T) { + if testing.Short() { + t.Skip("waits for a background process to outlive its parent") + } + + dir := t.TempDir() + marker := filepath.Join(dir, "background-finished.txt") + + // The shell launches a background task and exits immediately, succeeding. + script := "start /b cmd /c \"ping -n 6 127.0.0.1 >nul & echo finished > " + marker + "\"" + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + cmd := NewCommandForWindowsCmdContext(ctx, "cmd", "/c", script) + if err := cmd.Run(); err != nil { + t.Fatalf("the command should have succeeded: %v", err) + } + + time.Sleep(10 * time.Second) + + if _, err := os.Stat(marker); err != nil { + t.Fatalf("a background process started by a SUCCESSFUL command was killed "+ + "when its handle was released (%v); only a timeout may terminate the tree", err) + } +} From 63a574aca44b492f8e7c65823f9771e28ab6587f Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Wed, 9 Sep 2026 14:27:11 +0100 Subject: [PATCH 15/21] feat(registry): accept regedit and PowerShell hive spellings, add view: Signed-off-by: Mark Bolwell --- resource/registry.go | 27 +++-- system/registry.go | 160 ++++++++++++++++++++++++++++-- system/registry_notwindows.go | 7 ++ system/registry_windows.go | 179 +++++++++++++++++++++++++++++----- 4 files changed, 334 insertions(+), 39 deletions(-) diff --git a/resource/registry.go b/resource/registry.go index f8e9b11..e4952db 100644 --- a/resource/registry.go +++ b/resource/registry.go @@ -10,14 +10,21 @@ import ( type Registry struct { DiscoveryMeta `yaml:",inline" json:",inline"` - Title string `json:"title,omitempty" yaml:"title,omitempty"` - Meta meta `json:"meta,omitempty" yaml:"meta,omitempty"` - id string `json:"-" yaml:"-"` - Name string `json:"name,omitempty" yaml:"name,omitempty"` - Exists matcher `json:"exists" yaml:"exists"` - Value matcher `json:"value,omitempty" yaml:"value,omitempty"` - Type matcher `json:"type,omitempty" yaml:"type,omitempty"` - Skip bool `json:"skip,omitempty" yaml:"skip,omitempty"` + Title string `json:"title,omitempty" yaml:"title,omitempty"` + Meta meta `json:"meta,omitempty" yaml:"meta,omitempty"` + id string `json:"-" yaml:"-"` + Name string `json:"name,omitempty" yaml:"name,omitempty"` + // View selects the WOW64 registry view: "32", "64" or "native". + // + // omitempty is load-bearing twice over. It keeps the attribute out of every + // gossfile that does not use it, so no golden changes and no existing spec + // gains a field it never asked for; and empty parses as native, which is + // exactly the behaviour that existed before this attribute did. + View string `json:"view,omitempty" yaml:"view,omitempty"` + Exists matcher `json:"exists" yaml:"exists"` + Value matcher `json:"value,omitempty" yaml:"value,omitempty"` + Type matcher `json:"type,omitempty" yaml:"type,omitempty"` + Skip bool `json:"skip,omitempty" yaml:"skip,omitempty"` } const ( @@ -66,6 +73,10 @@ func (r *Registry) Validate(ctx context.Context, sys *system.System) []TestResul ctx = withID(ctx, r.ID()) skip := r.Skip sysRegistry := sys.NewRegistry(ctx, r.GetName(), sys, util.Config{}) + // An unusable view is carried by the system object and surfaces from the + // accessors, so a bad `view:` fails the assertions that depended on it + // rather than being reported here and nowhere else. + _ = sysRegistry.SetView(r.View) var results []TestResult results = append(results, ValidateValue(r, "exists", r.Exists, sysRegistry.Exists, skip)) diff --git a/system/registry.go b/system/registry.go index 20ef271..d20f7eb 100644 --- a/system/registry.go +++ b/system/registry.go @@ -2,6 +2,7 @@ package system import ( "errors" + "fmt" "strings" ) @@ -10,10 +11,97 @@ type Registry interface { Exists() (bool, error) Value() (string, error) Type() (string, error) + // SetView selects the WOW64 registry view this resource reads. It is part + // of the interface rather than a constructor argument because the view is + // per-resource spec syntax, while the constructor is shared with every + // other resource type and takes only global config. + // + // An unusable view is recorded and surfaces from Exists, Value and Type + // rather than being returned here, so that a bad `view:` fails the check + // that used it instead of disappearing into a constructor nobody checks. + SetView(view string) error } var ErrRegistryUnsupported = errors.New("registry resource is only supported on Windows") +// RegistryView is the WOW64 view a registry read is performed against. +// +// On 64-bit Windows some keys exist twice: 64-bit programs see one copy and +// 32-bit programs, redirected through WOW64, see another under Wow6432Node. A +// spec that does not say which it means gets whichever the running binary +// happens to be, which for syver is always 64-bit. Compliance work needs to be +// able to assert against either. +type RegistryView int + +const ( + // RegistryViewNative is the default and MUST behave exactly as the code did + // before views existed: no view flag is added to the access mask, so the + // answer is whatever the OS gives a 64-bit process. Any change here is a + // silent change to every existing gossfile. + RegistryViewNative RegistryView = iota + RegistryView32 + RegistryView64 +) + +// ParseRegistryView turns the spec's `view:` string into a RegistryView. +// +// It lives in this untagged file, not in registry_windows.go, so that its +// tests run on Linux. The grammar is a user-facing contract and does not need +// a Windows host to be wrong. +func ParseRegistryView(view string) (RegistryView, error) { + switch strings.ToLower(strings.TrimSpace(view)) { + case "", "native": + return RegistryViewNative, nil + case "32": + return RegistryView32, nil + case "64": + return RegistryView64, nil + default: + return RegistryViewNative, errors.New( + `invalid registry view "` + view + `": want 32, 64 or native`) + } +} + +func (v RegistryView) String() string { + switch v { + case RegistryView32: + return "32" + case RegistryView64: + return "64" + default: + return "native" + } +} + +// registryValueVsKeyWarning is the text of the FEAT-012 W2-5 diagnostic, kept +// in this untagged file so that the parts of it that can be wrong without a +// Windows host are checkable without one. +// +// Three things about this string are load-bearing and none of them are +// reachable from a build-tagged file on Linux: +// +// 1. The "[WARN]" prefix. logs.go filters on that literal through +// logutils.LevelFilter. Drop it, lowercase it or move it after the +// "registry:" label and the line stops being level-filtered at all -- it +// then prints even at --log-level=ERROR, and nothing else in the suite +// would notice. +// 2. %s, not %q, for the suggested path. %q escapes every backslash, so +// HKLM\SOFTWARE\... prints as HKLM\\SOFTWARE\\..., which reads as a +// different path from the one the operator has to type. +// 3. %q, not %s, for the value name. A name that is empty, or that has +// leading or trailing spaces, is invisible unquoted -- and those are +// exactly the names that get an author into this diagnostic. +// +// The FIRING CONDITION -- a value miss with a subkey of that name alongside it +// -- needs a real registry and is tested on Windows in +// registry_windows_diag_test.go. +func registryValueVsKeyWarning(key, valueName string) string { + return fmt.Sprintf( + "[WARN] registry: %s: no value named %q, but a key of that name exists here; "+ + "a trailing backslash asks about the key: %s", + key, valueName, key+`\`) +} + // registryPathParts holds the parsed components of a registry key path. type registryPathParts struct { Hive string @@ -21,18 +109,76 @@ type registryPathParts struct { ValueName string } +// hiveAliases maps every spelling of a hive that syver accepts onto the short +// canonical form. The long names and the PowerShell provider form are here +// because they are what an operator copies: regedit's address bar shows +// HKEY_LOCAL_MACHINE\..., and Get-ItemProperty output shows HKLM:\.... +// Requiring the short form meant hand-editing every path pasted from the tools +// the spec is describing. +var hiveAliases = map[string]string{ + "HKLM": "HKLM", + "HKEY_LOCAL_MACHINE": "HKLM", + "HKCU": "HKCU", + "HKEY_CURRENT_USER": "HKCU", + "HKCR": "HKCR", + "HKEY_CLASSES_ROOT": "HKCR", + "HKU": "HKU", + "HKEY_USERS": "HKU", + "HKCC": "HKCC", + "HKEY_CURRENT_CONFIG": "HKCC", +} + +// normaliseHive maps any accepted spelling onto the canonical short form. +// +// The trailing colon of the PowerShell provider form (HKLM:\...) is stripped +// here rather than in the caller so that there is exactly one place that knows +// which spellings exist. lookupHive then only ever sees canonical names, which +// is what removes the second, duplicated switch that used to validate them +// again on the Windows side. +func normaliseHive(raw string) (string, bool) { + name := strings.ToUpper(strings.TrimSuffix(raw, ":")) + canonical, ok := hiveAliases[name] + return canonical, ok +} + // parseRegistryKey splits a full registry path into hive, subkey, and value name. // -// Two formats are supported: +// Accepted hive spellings: the short forms (HKLM), the long forms +// (HKEY_LOCAL_MACHINE), and either with the PowerShell provider colon +// (HKLM:\...). All normalise to the short form. +// +// Two path formats are supported: // // Standard format: HIVE\subkey\path\ValueName -// The last backslash-separated segment is the value name. A trailing -// backslash indicates the default value. +// The last backslash-separated segment is the value name. // // Explicit format: HIVE\subkey\path::ValueName // Use "::" to explicitly separate the subkey from the value name. This // is required when the value name itself contains backslashes (e.g. // HardenedPaths entries like "\\*\NETLOGON"). +// +// # A TRAILING BACKSLASH ADDRESSES THE KEY, NOT A VALUE +// +// `HKLM\A\B\` parses to subkey `A\B` with an empty value name, and that is a +// deliberate and load-bearing distinction from `HKLM\A\B`, which parses to +// subkey `A` and value name `B`. The two ask different questions of the +// registry and get different answers: +// +// HKLM\...\ProfileList exists -> false (is there a VALUE named ProfileList?) +// HKLM\...\ProfileList\ exists -> true (is there a KEY named ProfileList?) +// +// Both answers are truthful. Measured on Windows Server 2025, 2026-09-05. +// +// This is NOT made optional or auto-detected, and that decision is not up for +// revisiting on the strength of a confused spec: guessing which the author +// meant moves the ambiguity one level down, where it cannot be seen at all. +// What the Windows implementation does instead is say so when the two are +// confusable -- see the diagnostic in registry_windows.go. +// +// Empty value name therefore means "the key itself" to Exists, and "the key's +// default value" to Value and Type, which is what regedit shows as (Default). +// A key can exist while having no default value, so `exists: true` with a +// value lookup that reports not-found is a coherent pair, not a contradiction. func parseRegistryKey(key string) (registryPathParts, error) { if key == "" { return registryPathParts{}, errors.New("empty registry key") @@ -43,10 +189,8 @@ func parseRegistryKey(key string) (registryPathParts, error) { return registryPathParts{}, errors.New("invalid registry key: missing subkey path") } - hive := strings.ToUpper(parts[0]) - switch hive { - case "HKLM", "HKCU", "HKCR", "HKU", "HKCC": - default: + hive, ok := normaliseHive(parts[0]) + if !ok { return registryPathParts{}, errors.New("invalid registry hive: " + parts[0]) } @@ -66,7 +210,7 @@ func parseRegistryKey(key string) (registryPathParts, error) { } // Standard format: split at the last backslash. - // Trailing backslash means default value (empty value name). + // Trailing backslash means the key itself (empty value name). lastSep := strings.LastIndex(rest, `\`) if lastSep < 0 { return registryPathParts{ diff --git a/system/registry_notwindows.go b/system/registry_notwindows.go index ddbfc39..fec9fd0 100644 --- a/system/registry_notwindows.go +++ b/system/registry_notwindows.go @@ -20,3 +20,10 @@ func (r *defRegistry) Key() string { return r.key } func (r *defRegistry) Exists() (bool, error) { return false, ErrRegistryUnsupported } func (r *defRegistry) Value() (string, error) { return "", ErrRegistryUnsupported } func (r *defRegistry) Type() (string, error) { return "", ErrRegistryUnsupported } + +// SetView accepts and discards the view. Every accessor on this type already +// reports ErrRegistryUnsupported, so validating the string here would replace a +// clear "not supported on this platform" with a grammar complaint about an +// attribute that could never have been honoured anyway. The grammar is still +// checked off Windows: ParseRegistryView is untagged and unit-tested on Linux. +func (r *defRegistry) SetView(view string) error { return nil } diff --git a/system/registry_windows.go b/system/registry_windows.go index 4b6be9a..389081c 100644 --- a/system/registry_windows.go +++ b/system/registry_windows.go @@ -7,6 +7,7 @@ import ( "encoding/hex" "errors" "fmt" + "log" "strconv" "strings" @@ -17,6 +18,12 @@ import ( type defRegistryWindows struct { key string + // view and viewErr are set by SetView. An unusable view is carried rather + // than returned so that it surfaces from the accessor that would have used + // it, and therefore fails the check, instead of being swallowed at + // construction time. Same shape as system/mount.go's deferred error. + view RegistryView + viewErr error } func NewDefRegistry(_ context.Context, key string, system *System, config util.Config) Registry { @@ -25,6 +32,32 @@ func NewDefRegistry(_ context.Context, key string, system *System, config util.C func (r *defRegistryWindows) Key() string { return r.key } +// SetView records the requested WOW64 view. See the Registry interface for why +// an invalid value is stored rather than returned. +func (r *defRegistryWindows) SetView(view string) error { + v, err := ParseRegistryView(view) + r.view, r.viewErr = v, err + return nil +} + +// accessMask is the access the registry opens are performed with. +// +// QUERY_VALUE is what every read needs. The WOW64 flags are additive and, for +// RegistryViewNative, nothing is added at all -- so a spec that does not +// mention `view:` produces byte-identical behaviour to the code that existed +// before views did. That property is the whole safety argument for adding the +// attribute, and a test pins it. +func (r *defRegistryWindows) accessMask() uint32 { + mask := uint32(registry.QUERY_VALUE) + switch r.view { + case RegistryView32: + mask |= registry.WOW64_32KEY + case RegistryView64: + mask |= registry.WOW64_64KEY + } + return mask +} + // Exists distinguishes "the key/value genuinely does not exist" // (registry.ErrNotExist -> false, nil) from every other failure, most // importantly ERROR_ACCESS_DENIED (-> false, err). The two used to be folded @@ -38,6 +71,10 @@ func (r *defRegistryWindows) Key() string { return r.key } // so the not-found-vs-access-denied distinction is unit-testable without an // actual unreadable registry key -- see registry_windows_error_test.go. func (r *defRegistryWindows) Exists() (bool, error) { + if r.viewErr != nil { + return false, r.viewErr + } + parts, err := parseRegistryKey(r.key) if err != nil { return false, err @@ -48,7 +85,7 @@ func (r *defRegistryWindows) Exists() (bool, error) { return false, err } - k, openErr := registry.OpenKey(hive, parts.SubKey, registry.QUERY_VALUE) + k, openErr := registry.OpenKey(hive, parts.SubKey, r.accessMask()) if exists, err := classifyRegistryError(openErr, "opening registry key"); !exists || err != nil { return exists, err } @@ -59,7 +96,50 @@ func (r *defRegistryWindows) Exists() (bool, error) { } _, _, getErr := k.GetValue(parts.ValueName, nil) - return classifyRegistryError(getErr, "reading registry value") + exists, err := classifyRegistryError(getErr, "reading registry value") + if err == nil && !exists { + r.warnIfKeyOfThatNameExists(k, parts.ValueName) + } + return exists, err +} + +// warnIfKeyOfThatNameExists is the FEAT-012 W2-5 diagnostic. +// +// A value lookup that misses while a SUBKEY of that name sits under the same +// parent is the one case where a truthful answer reliably answers a different +// question from the one the author asked. `exists: false` then PASSES, which +// makes it the same class of false confidence FEAT-010 existed to remove -- +// worse, because nothing fails and nothing is logged. +// +// THREE DECISIONS, all carried unanswered out of FEAT-011 1.4 and settled here: +// +// 1. Warning, not failure text. Attaching it to failure output only would stay +// silent on the false PASS, which is the case that motivated it. A warning +// fires on both. +// 2. The extra registry open is paid only on a miss that is genuinely +// not-found, never on a hit and never on an access-denied error. A spec +// full of absent-value assertions under `serve` pays one OpenKey per such +// assertion per cycle. That is the accepted cost; the alternative is +// staying quiet about a passing check that is not testing what it says. +// 3. It goes through log, so it is formatter-independent. That sidesteps the +// question of which formatters should carry it: none of them do, and +// --log-level governs it like every other warning. +// +// Deliberately silent when the probe itself fails: this is a hint, and a hint +// that reports its own errors is noise on top of noise. +func (r *defRegistryWindows) warnIfKeyOfThatNameExists(parent registry.Key, name string) { + sub, err := registry.OpenKey(parent, name, r.accessMask()) + if err != nil { + return + } + sub.Close() + // log.Print, not log.Printf: the message is already formatted, and a + // registry path or value name containing a % would otherwise be rendered as + // a verb against no arguments and print as %!x(MISSING). The formatting + // itself lives in registryValueVsKeyWarning, in the untagged file, so the + // parts of it that can be wrong without a Windows host are tested without + // one -- see registry_warning_test.go for what "wrong" means here. + log.Print(registryValueVsKeyWarning(r.key, name)) } // classifyRegistryError turns a raw OpenKey/GetValue error into the @@ -77,25 +157,43 @@ func classifyRegistryError(err error, wrapMsg string) (exists bool, resultErr er return false, fmt.Errorf("%s: %w", wrapMsg, err) } -func (r *defRegistryWindows) Value() (string, error) { +// openForRead is the shared open path for Value and Type. Both previously +// repeated parse, hive lookup and open verbatim, and both would have needed the +// view threading through independently. +func (r *defRegistryWindows) openForRead() (registry.Key, registryPathParts, error) { + if r.viewErr != nil { + return 0, registryPathParts{}, r.viewErr + } + parts, err := parseRegistryKey(r.key) if err != nil { - return "", err + return 0, parts, err } hive, err := lookupHive(parts.Hive) if err != nil { - return "", err + return 0, parts, err + } + + k, err := registry.OpenKey(hive, parts.SubKey, r.accessMask()) + if err != nil { + return 0, parts, fmt.Errorf("opening registry key: %w", err) } + return k, parts, nil +} - k, err := registry.OpenKey(hive, parts.SubKey, registry.QUERY_VALUE) +func (r *defRegistryWindows) Value() (string, error) { + k, parts, err := r.openForRead() if err != nil { - return "", fmt.Errorf("opening registry key: %w", err) + return "", err } defer k.Close() _, valType, err := k.GetValue(parts.ValueName, nil) if err != nil { + if errors.Is(err, registry.ErrNotExist) && parts.ValueName != "" { + r.warnIfKeyOfThatNameExists(k, parts.ValueName) + } return "", fmt.Errorf("reading registry value: %w", err) } @@ -103,30 +201,30 @@ func (r *defRegistryWindows) Value() (string, error) { } func (r *defRegistryWindows) Type() (string, error) { - parts, err := parseRegistryKey(r.key) + k, parts, err := r.openForRead() if err != nil { return "", err } - - hive, err := lookupHive(parts.Hive) - if err != nil { - return "", err - } - - k, err := registry.OpenKey(hive, parts.SubKey, registry.QUERY_VALUE) - if err != nil { - return "", fmt.Errorf("opening registry key: %w", err) - } defer k.Close() _, valType, err := k.GetValue(parts.ValueName, nil) if err != nil { + if errors.Is(err, registry.ErrNotExist) && parts.ValueName != "" { + r.warnIfKeyOfThatNameExists(k, parts.ValueName) + } return "", fmt.Errorf("reading registry value: %w", err) } return typeName(valType), nil } +// lookupHive maps a CANONICAL short hive name onto its root key. +// +// It no longer re-validates spellings: parseRegistryKey normalises every +// accepted form (long, short, PowerShell colon) to the canonical short name +// before this is reached, so the two switches that used to list the same five +// hives are now one list, in hiveAliases. The default branch stays as a guard +// against a caller that skipped the parser, not as a second grammar. func lookupHive(name string) (registry.Key, error) { switch name { case "HKLM": @@ -144,6 +242,14 @@ func lookupHive(name string) (registry.Key, error) { } } +// formatValue renders a value as the string a matcher compares against. +// +// REG_EXPAND_SZ IS COMPARED UNEXPANDED. GetStringValue returns the stored form, +// so a value holding "%SystemRoot%\System32" is matched as that literal text +// and not as "C:\Windows\System32". That is the right default for compliance +// work -- the benchmark states the stored form, and expansion depends on the +// environment of whoever is asking -- but it is not what an operator +// necessarily expects, so it is stated here and in docs/gossfile.md. func formatValue(valType uint32, k registry.Key, name string) (string, error) { switch valType { case registry.SZ, registry.EXPAND_SZ: @@ -158,7 +264,14 @@ func formatValue(valType uint32, k registry.Key, name string) (string, error) { return "", err } return strconv.FormatUint(v, 10), nil - case registry.BINARY: + case registry.BINARY, registry.NONE, registry.DWORD_BIG_ENDIAN, + registry.RESOURCE_LIST, registry.FULL_RESOURCE_DESCRIPTOR, + registry.RESOURCE_REQUIREMENTS_LIST: + // Everything with no more specific rendering is hex. The three + // RESOURCE_* types are hardware descriptors that have no textual form + // worth inventing, and DWORD_BIG_ENDIAN is deliberately NOT folded in + // with DWORD: GetIntegerValue would read it in the wrong byte order and + // return a confidently wrong number. b, _, err := k.GetBinaryValue(name) if err != nil { return "", err @@ -170,25 +283,45 @@ func formatValue(valType uint32, k registry.Key, name string) (string, error) { return "", err } return strings.Join(ss, "\n"), nil + case registry.LINK: + // A symbolic link's target is not readable through the normal value + // API, and pretending otherwise would fabricate a value. + return "", errors.New("REG_LINK values cannot be read as a value; assert on type instead") default: return "", fmt.Errorf("unsupported registry value type: %d", valType) } } +// typeName covers the full REG_* set, so a `type:` assertion works against the +// exotic types rather than reporting UNKNOWN(n) for anything outside the common +// six. The default branch stays: a type this build does not know about should +// say so rather than be silently renamed. func typeName(t uint32) string { switch t { + case registry.NONE: + return "REG_NONE" case registry.SZ: return "REG_SZ" case registry.EXPAND_SZ: return "REG_EXPAND_SZ" - case registry.DWORD: - return "REG_DWORD" - case registry.QWORD: - return "REG_QWORD" case registry.BINARY: return "REG_BINARY" + case registry.DWORD: + return "REG_DWORD" + case registry.DWORD_BIG_ENDIAN: + return "REG_DWORD_BIG_ENDIAN" + case registry.LINK: + return "REG_LINK" case registry.MULTI_SZ: return "REG_MULTI_SZ" + case registry.RESOURCE_LIST: + return "REG_RESOURCE_LIST" + case registry.FULL_RESOURCE_DESCRIPTOR: + return "REG_FULL_RESOURCE_DESCRIPTOR" + case registry.RESOURCE_REQUIREMENTS_LIST: + return "REG_RESOURCE_REQUIREMENTS_LIST" + case registry.QWORD: + return "REG_QWORD" default: return fmt.Sprintf("UNKNOWN(%d)", t) } From 9947cd92113236df9e0d5033d171e6c68059131f Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Wed, 9 Sep 2026 14:27:33 +0100 Subject: [PATCH 16/21] test(registry): pin the grammar, the views, and the value-vs-key warning Signed-off-by: Mark Bolwell --- resource/conformance_fakes_test.go | 1 + resource/sw10_add_path_test.go | 1 + system/registry_test.go | 123 +++++++++++ system/registry_warning_test.go | 63 ++++++ system/registry_windows_diag_test.go | 226 +++++++++++++++++++ system/registry_windows_roundtrip_test.go | 252 ++++++++++++++++++++++ system/registry_windows_view_test.go | 106 +++++++++ 7 files changed, 772 insertions(+) create mode 100644 system/registry_warning_test.go create mode 100644 system/registry_windows_diag_test.go create mode 100644 system/registry_windows_roundtrip_test.go create mode 100644 system/registry_windows_view_test.go diff --git a/resource/conformance_fakes_test.go b/resource/conformance_fakes_test.go index c8b2827..dd93b64 100644 --- a/resource/conformance_fakes_test.go +++ b/resource/conformance_fakes_test.go @@ -135,6 +135,7 @@ func (conformanceRegistry) Key() string { return "registry-fake" } func (conformanceRegistry) Exists() (bool, error) { return true, nil } func (conformanceRegistry) Value() (string, error) { return "", nil } func (conformanceRegistry) Type() (string, error) { return "", nil } +func (conformanceRegistry) SetView(string) error { return nil } type conformancePort struct{} diff --git a/resource/sw10_add_path_test.go b/resource/sw10_add_path_test.go index e1aa956..3080022 100644 --- a/resource/sw10_add_path_test.go +++ b/resource/sw10_add_path_test.go @@ -22,6 +22,7 @@ func (f sw10FakeRegistry) Key() string { return "HKLM\\SW10\\Fake" } func (f sw10FakeRegistry) Exists() (bool, error) { return false, f.existsErr } func (f sw10FakeRegistry) Value() (string, error) { return "", nil } func (f sw10FakeRegistry) Type() (string, error) { return "", nil } +func (f sw10FakeRegistry) SetView(string) error { return nil } func TestNewRegistry_PropagatesExistsError(t *testing.T) { _, err := NewRegistry(sw10FakeRegistry{existsErr: system.ErrRegistryUnsupported}, util.Config{}) diff --git a/system/registry_test.go b/system/registry_test.go index 5feb6f0..e95a3f2 100644 --- a/system/registry_test.go +++ b/system/registry_test.go @@ -150,3 +150,126 @@ func TestParseRegistryKey(t *testing.T) { }) } } + +// FEAT-012 W2-3. The hive grammar used to accept only the five short names, so +// a path copied out of regedit's address bar or out of Get-ItemProperty output +// had to be hand-edited before syver would take it. These are the spellings the +// tools an operator copies from actually produce. +// +// This runs on Linux on purpose: parseRegistryKey is untagged, and a grammar +// contract does not need a Windows host to be wrong. +func TestParseRegistryKeyAcceptsTheSpellingsWindowsToolsShow(t *testing.T) { + for _, tc := range []struct { + key string + wantHive string + }{ + {`HKLM\Software\Syver\Value`, "HKLM"}, + {`HKEY_LOCAL_MACHINE\Software\Syver\Value`, "HKLM"}, + {`HKLM:\Software\Syver\Value`, "HKLM"}, + {`HKEY_LOCAL_MACHINE:\Software\Syver\Value`, "HKLM"}, + {`hkey_local_machine\Software\Syver\Value`, "HKLM"}, + {`HKEY_CURRENT_USER\Console\Value`, "HKCU"}, + {`HKCU:\Console\Value`, "HKCU"}, + {`HKEY_CLASSES_ROOT\.txt\Value`, "HKCR"}, + {`HKEY_USERS\.DEFAULT\Value`, "HKU"}, + {`HKEY_CURRENT_CONFIG\System\Value`, "HKCC"}, + } { + got, err := parseRegistryKey(tc.key) + if err != nil { + t.Errorf("parseRegistryKey(%q) = %v; this is a spelling regedit or PowerShell produces", tc.key, err) + continue + } + if got.Hive != tc.wantHive { + t.Errorf("parseRegistryKey(%q).Hive = %q, want the canonical %q", tc.key, got.Hive, tc.wantHive) + } + if got.ValueName != "Value" { + t.Errorf("parseRegistryKey(%q).ValueName = %q, want %q", tc.key, got.ValueName, "Value") + } + } +} + +// Normalising must not turn everything into a hive. A name that merely looks +// hive-shaped is still an error, or a typo silently reads the wrong hive. +func TestParseRegistryKeyStillRejectsNonHives(t *testing.T) { + for _, key := range []string{ + `HKEY_LOCAL_MACHIN\Software\X`, + `HKLMM\Software\X`, + `HK\Software\X`, + `HKEY_LOCAL_MACHINE_EXTRA\Software\X`, + `:\Software\X`, + `Software\X`, + } { + if _, err := parseRegistryKey(key); err == nil { + t.Errorf("parseRegistryKey(%q) succeeded; a hive that does not exist must not be accepted", key) + } + } +} + +// The trailing backslash is the distinction FEAT-012 W2-5 refuses to guess at. +// These two strings differ by one character and ask different questions, and +// that is the documented contract rather than an accident. +func TestParseRegistryKeyTrailingBackslashAddressesTheKey(t *testing.T) { + withValue, err := parseRegistryKey(`HKLM\Software\Syver\ProfileList`) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if withValue.SubKey != `Software\Syver` || withValue.ValueName != "ProfileList" { + t.Errorf("no trailing backslash should name a VALUE: got subkey %q value %q", + withValue.SubKey, withValue.ValueName) + } + + withoutValue, err := parseRegistryKey(`HKLM\Software\Syver\ProfileList\`) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if withoutValue.SubKey != `Software\Syver\ProfileList` || withoutValue.ValueName != "" { + t.Errorf("a trailing backslash should name a KEY: got subkey %q value %q", + withoutValue.SubKey, withoutValue.ValueName) + } +} + +// FEAT-012 W2-4. The view grammar is user-facing spec syntax, so it is pinned +// on the platform that cannot honour it as well as the one that can. +func TestParseRegistryView(t *testing.T) { + for _, tc := range []struct { + in string + want RegistryView + }{ + {"", RegistryViewNative}, + {"native", RegistryViewNative}, + {"NATIVE", RegistryViewNative}, + {" native ", RegistryViewNative}, + {"32", RegistryView32}, + {"64", RegistryView64}, + } { + got, err := ParseRegistryView(tc.in) + if err != nil { + t.Errorf("ParseRegistryView(%q) = %v, want %v", tc.in, err, tc.want) + continue + } + if got != tc.want { + t.Errorf("ParseRegistryView(%q) = %v, want %v", tc.in, got, tc.want) + } + } + + for _, bad := range []string{"x86", "amd64", "wow64", "32bit", "0", "true"} { + if _, err := ParseRegistryView(bad); err == nil { + t.Errorf("ParseRegistryView(%q) succeeded; an unrecognised view must not "+ + "silently fall back to native, which would read the wrong registry branch", bad) + } + } +} + +// The empty view must be native, and native must be the zero value. If either +// stops holding, every gossfile written before `view:` existed changes meaning +// without its author touching it. +func TestUnsetViewIsNativeAndNativeIsTheZeroValue(t *testing.T) { + var zero RegistryView + if zero != RegistryViewNative { + t.Fatal("RegistryViewNative is not the zero value; an unset view would not be native") + } + got, err := ParseRegistryView("") + if err != nil || got != RegistryViewNative { + t.Fatalf(`ParseRegistryView("") = %v, %v; want native and no error`, got, err) + } +} diff --git a/system/registry_warning_test.go b/system/registry_warning_test.go new file mode 100644 index 0000000..4221332 --- /dev/null +++ b/system/registry_warning_test.go @@ -0,0 +1,63 @@ +package system + +import ( + "strings" + "testing" +) + +// TestValueVsKeyWarningKeepsItsLevelPrefix pins the one property of the +// FEAT-012 W2-5 diagnostic that decides whether it is ever SEEN. +// +// logs.go filters the standard logger through logutils.LevelFilter, which +// classifies a line by the bracketed token it starts with and passes anything +// it cannot classify straight through. So a message that loses its "[WARN]" +// prefix, lowercases it, or puts anything before it does not go quiet -- it +// goes the other way and prints at every log level including --log-level=ERROR. +// Nothing else in the suite would notice, because the line still appears. +func TestValueVsKeyWarningKeepsItsLevelPrefix(t *testing.T) { + got := registryValueVsKeyWarning(`HKLM\SOFTWARE\Example\ProfileList`, "ProfileList") + + if !strings.HasPrefix(got, "[WARN] ") { + t.Fatalf("diagnostic = %q; it must START with the literal \"[WARN] \" or "+ + "logutils cannot classify it and it prints at every log level", got) + } +} + +// TestValueVsKeyWarningPrintsAPathAnOperatorCanType is decision 2 in the +// function's own comment, stated as a check rather than a claim. +// +// The whole point of the message is that it hands the reader the corrected +// path. %q would escape every separator, so HKLM\SOFTWARE\... would print as +// HKLM\\SOFTWARE\\... -- which is not the path, and a reader who pastes it gets +// a different error from the one they started with. +func TestValueVsKeyWarningPrintsAPathAnOperatorCanType(t *testing.T) { + const key = `HKLM\SOFTWARE\Microsoft\Windows NT\CurrentVersion\ProfileList` + + got := registryValueVsKeyWarning(key, "ProfileList") + + if strings.Contains(got, `\\`) { + t.Errorf("diagnostic contains a doubled backslash, so a path was rendered "+ + "with %%q: %s", got) + } + if !strings.Contains(got, key+`\`) { + t.Errorf("diagnostic does not contain the corrected path %q, which is the "+ + "only actionable thing in it: %s", key+`\`, got) + } +} + +// TestValueVsKeyWarningQuotesTheValueName is decision 3, and it is the opposite +// of the rule above on purpose. +// +// The names that land an author in this diagnostic are the ones that do not +// look like anything: empty, or padded with spaces because a YAML key was +// written with a trailing blank. Unquoted, those are invisible and the message +// reads as nonsense. +func TestValueVsKeyWarningQuotesTheValueName(t *testing.T) { + for _, name := range []string{"", " Trailing ", "ProfileList"} { + got := registryValueVsKeyWarning(`HKLM\SOFTWARE\Example`, name) + if !strings.Contains(got, `"`+name+`"`) { + t.Errorf("value name %q is not quoted in the diagnostic, so it cannot be "+ + "seen: %s", name, got) + } + } +} diff --git a/system/registry_windows_diag_test.go b/system/registry_windows_diag_test.go new file mode 100644 index 0000000..f5c22b3 --- /dev/null +++ b/system/registry_windows_diag_test.go @@ -0,0 +1,226 @@ +//go:build windows + +package system + +import ( + "bytes" + "log" + "os" + "strings" + "testing" + + "golang.org/x/sys/windows/registry" +) + +const diagTestSubKey = `Software\SyverDiagRoundtripTest` + +// withDiagKey builds the shape the FEAT-012 W2-5 diagnostic exists for: a +// parent holding a SUBKEY named "Ambiguous" and, separately, a VALUE named +// "Real". Asking for the value "Ambiguous" then misses while something of that +// name is plainly sitting there. +// +// It also creates two shapes the obvious version of this fixture misses. A +// subkey whose name contains a percent sign, because registry names are full of +// those -- everything under Session Manager\Environment -- and they are the +// input that tells log.Print from log.Printf. And "Twin", which exists as BOTH +// a value and a subkey: values and subkeys are separate namespaces, so that is +// legal, common, and the only shape in which the diagnostic could fire on a +// lookup that HIT. +func withDiagKey(t *testing.T) { + t.Helper() + + k, _, err := registry.CreateKey(registry.CURRENT_USER, diagTestSubKey, + registry.SET_VALUE|registry.QUERY_VALUE|registry.CREATE_SUB_KEY) + if err != nil { + t.Fatalf("creating HKCU\\%s: %v", diagTestSubKey, err) + } + for name, val := range map[string]string{ + "Real": "a genuine value", + "Twin": "a value that also has a subkey of the same name", + } { + if err := k.SetStringValue(name, val); err != nil { + t.Fatalf("SetStringValue(%s): %v", name, err) + } + } + for _, sub := range []string{"Ambiguous", "%SystemRoot%", "Twin"} { + sk, _, err := registry.CreateKey(k, sub, registry.QUERY_VALUE) + if err != nil { + t.Fatalf("creating subkey %q: %v", sub, err) + } + sk.Close() + } + + t.Cleanup(func() { + for _, sub := range []string{"Ambiguous", "%SystemRoot%", "Twin"} { + if err := registry.DeleteKey(k, sub); err != nil { + t.Errorf("leaked HKCU\\%s\\%s: %v", diagTestSubKey, sub, err) + } + } + k.Close() + if err := registry.DeleteKey(registry.CURRENT_USER, diagTestSubKey); err != nil { + t.Errorf("leaked HKCU\\%s: %v", diagTestSubKey, err) + } + }) +} + +// captureLog redirects the standard logger for the duration of fn. The +// diagnostic goes through log rather than through a formatter precisely so that +// --log-level governs it, which also makes it observable here. +func captureLog(t *testing.T, fn func()) string { + t.Helper() + + var buf bytes.Buffer + log.SetOutput(&buf) + t.Cleanup(func() { log.SetOutput(os.Stderr) }) + fn() + log.SetOutput(os.Stderr) + return buf.String() +} + +// TestValueMissBesideAKeyOfThatNameWarnsFromEveryAccessor is the firing +// condition, which no test reached before: the diagnostic was a log.Printf with +// nothing observing it, on the one code path whose whole purpose is to say +// something about a check that PASSES. +// +// All three accessors are exercised, because all three have their own call site +// and a fix applied to one of them is exactly the kind of change that leaves +// the other two silent. +// +// REVERT-PROOF: delete the warnIfKeyOfThatNameExists call from Exists (or from +// Value, or from Type) and the corresponding subtest fails while the other two +// still pass. Confirmed on win11-test, 2026-09-09. +func TestValueMissBesideAKeyOfThatNameWarnsFromEveryAccessor(t *testing.T) { + withDiagKey(t) + + const key = `HKCU\` + diagTestSubKey + `\Ambiguous` + + t.Run("Exists", func(t *testing.T) { + r := &defRegistryWindows{key: key} + var exists bool + var err error + out := captureLog(t, func() { exists, err = r.Exists() }) + + // The trap this warns about: the honest answer is false and the check + // PASSES, so nothing in the output tells the author they asked about a + // value when they meant a key. + if err != nil || exists { + t.Fatalf("Exists() = %v, %v; want false, nil -- a subkey is not a value", exists, err) + } + assertWarnsAbout(t, out, key) + }) + + t.Run("Value", func(t *testing.T) { + r := &defRegistryWindows{key: key} + out := captureLog(t, func() { _, _ = r.Value() }) + assertWarnsAbout(t, out, key) + }) + + t.Run("Type", func(t *testing.T) { + r := &defRegistryWindows{key: key} + out := captureLog(t, func() { _, _ = r.Type() }) + assertWarnsAbout(t, out, key) + }) +} + +func assertWarnsAbout(t *testing.T, out, key string) { + t.Helper() + + if !strings.Contains(out, "[WARN]") { + t.Fatalf("no warning was logged for %s; the false PASS this diagnostic "+ + "exists for is silent again. Got: %q", key, out) + } + if !strings.Contains(out, key+`\`) { + t.Errorf("the warning does not hand back the corrected path %q, which is "+ + "the only actionable thing in it. Got: %q", key+`\`, out) + } +} + +// TestTheDiagnosticStaysQuietWhenThereIsNothingToSay pins the other half. +// +// A warning that fires on ordinary absent values would be worse than no warning +// at all: `exists: false` is a completely normal assertion in a hardening spec, +// and a spec full of them would bury the one case that matters under noise it +// caused itself. The extra OpenKey it costs is also only justified on a miss. +// +// TWO SEPARATE GUARDS keep it quiet, and an earlier version of this test +// covered only one of them -- the mutation meant to break it passed. They are +// listed here as the mutations that actually reach them, because "this test is +// about silence" is not enough to tell you what silence depends on: +// +// REVERT-PROOF 1, the guard inside warnIfKeyOfThatNameExists: drop its +// `if err != nil { return }` so it warns even when the probe open failed, and +// the two absent-name cases fail. +// +// REVERT-PROOF 2, the `err == nil && !exists` guard in Exists: replace it with +// `if true` so the probe runs on a HIT too, and the "Twin" case fails -- and +// ONLY the Twin case, because a name that is not also a subkey cannot produce a +// warning however unconditionally the probe is called. Twin is in the table for +// exactly that reason. +// +// Both confirmed on win11-test, 2026-09-09. +func TestTheDiagnosticStaysQuietWhenThereIsNothingToSay(t *testing.T) { + withDiagKey(t) + + for _, tc := range []struct { + name, key string + wantExists bool + }{ + // Nothing of that name exists in any form. An ordinary `exists: false`. + {"absent value with no key beside it", `HKCU\` + diagTestSubKey + `\NoSuchThingAtAll`, false}, + // A value that is really there. The miss branch is never entered, so + // the second OpenKey is never paid. + {"value that exists", `HKCU\` + diagTestSubKey + `\Real`, true}, + // The corrected form the warning itself suggests. Having asked the + // right question, the author must not be told to ask it. + {"the key, addressed with a trailing backslash", `HKCU\` + diagTestSubKey + `\Ambiguous\`, true}, + // A name that is a value AND a subkey. The lookup HITS, so there is + // nothing ambiguous about the answer and nothing to warn about -- but a + // subkey of that name really is sitting there, so the probe inside + // warnIfKeyOfThatNameExists would succeed if it were ever reached. This + // is the only row that depends on Exists guarding the call at all. + {"a name that is both a value and a key", `HKCU\` + diagTestSubKey + `\Twin`, true}, + } { + t.Run(tc.name, func(t *testing.T) { + r := &defRegistryWindows{key: tc.key} + var exists bool + var err error + out := captureLog(t, func() { exists, err = r.Exists() }) + + if err != nil { + t.Fatalf("Exists(): %v", err) + } + if exists != tc.wantExists { + t.Fatalf("Exists() = %v, want %v", exists, tc.wantExists) + } + if strings.Contains(out, "[WARN]") { + t.Errorf("warned about %s, which is not ambiguous: %q", tc.key, out) + } + }) + } +} + +// TestTheDiagnosticSurvivesAPercentInTheName is why the call site uses +// log.Print and not log.Printf. +// +// The message arrives pre-formatted. Handed to Printf with no arguments, a +// registry name containing a percent is read as a format verb and the path +// comes out as %!S(MISSING) -- so the diagnostic would corrupt the one thing it +// exists to hand back, and only for the names most likely to appear under +// Session Manager\Environment. +// +// REVERT-PROOF: change the call site back to +// log.Printf(registryValueVsKeyWarning(...)) and this fails with +// %!S(MISSING) in the output. Confirmed on win11-test, 2026-09-09. +func TestTheDiagnosticSurvivesAPercentInTheName(t *testing.T) { + withDiagKey(t) + + const key = `HKCU\` + diagTestSubKey + `\%SystemRoot%` + + r := &defRegistryWindows{key: key} + out := captureLog(t, func() { _, _ = r.Exists() }) + + if strings.Contains(out, "%!") { + t.Fatalf("the pre-formatted warning was re-interpreted as a format string: %q", out) + } + assertWarnsAbout(t, out, key) +} diff --git a/system/registry_windows_roundtrip_test.go b/system/registry_windows_roundtrip_test.go new file mode 100644 index 0000000..8bd088a --- /dev/null +++ b/system/registry_windows_roundtrip_test.go @@ -0,0 +1,252 @@ +//go:build windows + +package system + +import ( + "errors" + "math" + "strconv" + "testing" + + "golang.org/x/sys/windows/registry" +) + +// The two tests in this file are the only ones in the tree that read a value +// syver itself put into a real registry. Everything else about the registry +// resource is either pure (parsing, view flags, type names) or asserts against +// keys Windows happened to ship, which can prove that a read RESOLVES but +// cannot prove it resolved to the right place. + +const ( + viewTestSubKey = `SOFTWARE\SyverViewRoundtripTest` + viewTestValue = "Marker" + typeTestSubKey = `Software\SyverTypeRoundtripTest` + wow64PhysicalSK = `SOFTWARE\Wow6432Node\SyverViewRoundtripTest` +) + +// createViewKey makes the test's own key in ONE WOW64 view and registers its +// removal. It reports whether the caller can proceed: writing under +// HKLM\SOFTWARE needs elevation, and a key that already exists belongs to +// somebody else and must not be written over or deleted. +func createViewKey(t *testing.T, viewFlag uint32, value string) bool { + t.Helper() + + k, existed, err := registry.CreateKey( + registry.LOCAL_MACHINE, viewTestSubKey, + registry.SET_VALUE|registry.QUERY_VALUE|viewFlag) + if errors.Is(err, registry.ErrNotExist) || err != nil { + t.Skipf("cannot create HKLM\\%s in view %#x (%v); this test needs an "+ + "elevated Windows session and asserts nothing without one", + viewTestSubKey, viewFlag, err) + return false + } + if existed { + k.Close() + t.Skipf("HKLM\\%s already exists; refusing to overwrite and then delete "+ + "a key this test did not create", viewTestSubKey) + return false + } + // Removal is registered HERE, before the write below, and not in the caller. + // The key exists from the CreateKey above onwards; SetStringValue can fail; + // and a caller-side t.Cleanup is not registered until this function has + // already returned. That gap leaked an empty key on the one path nobody + // exercises. Found by re-reading for the question "can this litter a real + // machine", not by it happening. + physical := viewTestSubKey + if viewFlag == registry.WOW64_32KEY { + physical = wow64PhysicalSK + } + t.Cleanup(func() { + if err := registry.DeleteKey(registry.LOCAL_MACHINE, physical); err != nil { + t.Errorf("leaked HKLM\\%s: %v", physical, err) + } + }) + + if err := k.SetStringValue(viewTestValue, value); err != nil { + k.Close() + t.Fatalf("setting %s in view %#x: %v", viewTestValue, viewFlag, err) + } + k.Close() + return true +} + +// TestThe32And64ViewsReadDifferentValuesFromTheSamePath is the assertion the +// `view:` attribute was added for, and until this test existed nothing made it. +// +// TestViewSelectsTheWow64Flag proves the right WOW64 bit reaches the access +// mask. The Windows fixture proves both views RESOLVE. Neither proves the two +// views land on different data, and an implementation that accepted `view:` and +// quietly ignored it would pass both. +// +// The key is created twice, once through each view, with a DIFFERENT value each +// time, and then read back through the production accessor. On 64-bit Windows +// the 32-bit branch is physically HKLM\SOFTWARE\Wow6432Node\..., which is +// asserted here too: if the redirection ever stopped happening the two writes +// would collide on one key and the last one would win. +// +// REVERT-PROOF: make accessMask() return QUERY_VALUE unconditionally -- the +// change that turns `view:` into decoration -- and this fails on the 32-bit +// read, reporting "sixtyfour" where "thirtytwo" was written. Confirmed on +// win11-test, 2026-09-09. +func TestThe32And64ViewsReadDifferentValuesFromTheSamePath(t *testing.T) { + const ( + want64 = "sixtyfour" + want32 = "thirtytwo" + ) + + // Each createViewKey registers its own removal before it can fail, so there + // is deliberately no cleanup wiring out here. + if !createViewKey(t, registry.WOW64_64KEY, want64) { + return + } + if !createViewKey(t, registry.WOW64_32KEY, want32) { + return + } + + full := `HKLM\` + viewTestSubKey + `\` + viewTestValue + + for _, tc := range []struct{ view, want string }{ + {"64", want64}, + {"32", want32}, + // No view at all, and the explicit spelling of no view at all. syver is + // a 64-bit binary, so both must agree with the 64-bit branch. This is + // the compatibility promise in RegistryViewNative's comment, checked + // against data rather than against an access mask. + {"", want64}, + {"native", want64}, + } { + r := &defRegistryWindows{key: full} + if err := r.SetView(tc.view); err != nil { + t.Fatalf("SetView(%q): %v", tc.view, err) + } + + if exists, err := r.Exists(); err != nil || !exists { + t.Fatalf("view %q: Exists() = %v, %v; want true, nil", tc.view, exists, err) + } + got, err := r.Value() + if err != nil { + t.Fatalf("view %q: Value(): %v", tc.view, err) + } + if got != tc.want { + t.Errorf("view %q read %q, want %q: the view did not select the branch "+ + "it names", tc.view, got, tc.want) + } + } + + // The redirection is what makes the two branches distinct in the first + // place. Assert it directly, so that a failure above can be told apart from + // the two writes having landed on one key. + phys := &defRegistryWindows{key: `HKLM\` + wow64PhysicalSK + `\` + viewTestValue} + got, err := phys.Value() + if err != nil { + t.Fatalf("reading the physical Wow6432Node path: %v", err) + } + if got != want32 { + t.Errorf("HKLM\\%s holds %q, want %q: the 32-bit write was not redirected, "+ + "so the two views were never separate", wow64PhysicalSK, got, want32) + } +} + +// TestValueRenderingForTheTypesTheSpecNames closes FEAT-012's REG_MULTI_SZ and +// REG_QWORD acceptance criteria, both of which were claimed rather than +// asserted: typeName is a pure map and proves only that a NAME exists, and +// formatValue had no test against a value of either type at all. +// +// HKCU, not HKLM, and deliberately: registry redirection does not apply to +// HKCU\Software, so nothing here needs elevation and this runs on any Windows +// developer's machine. +// +// REVERT-PROOF, each independently confirmed on win11-test, 2026-09-09: +// - MULTI_SZ joined with " " instead of "\n" -> the multi-line case fails +// - QWORD folded into the SZ branch -> every QWORD case fails +// - QWORD rendered with strconv.FormatInt -> maxUint64 fails, and only +// that case, which is why the boundary value is in the table +func TestValueRenderingForTheTypesTheSpecNames(t *testing.T) { + k, _, err := registry.CreateKey(registry.CURRENT_USER, typeTestSubKey, + registry.SET_VALUE|registry.QUERY_VALUE) + if err != nil { + t.Fatalf("creating HKCU\\%s: %v", typeTestSubKey, err) + } + t.Cleanup(func() { + k.Close() + if err := registry.DeleteKey(registry.CURRENT_USER, typeTestSubKey); err != nil { + t.Errorf("leaked HKCU\\%s: %v", typeTestSubKey, err) + } + }) + + multi := map[string][]string{ + // The ordinary case, and the one docs/gossfile.md describes: the + // elements come back joined with newlines, NOT as a YAML list. An + // author writing `value:` for one of these has to know that. + "MultiOrdinary": {"alpha", "beta", "gamma"}, + // One element must produce NO separator. A join implemented as + // "append a newline after each" passes the case above and fails here. + "MultiSingle": {"only"}, + // An empty element is not the same as no element. Windows stores this + // faithfully and the rendering has to keep the blank line, or two + // different registry contents render identically. + "MultiWithBlank": {"alpha", "", "gamma"}, + } + for name, val := range multi { + if err := k.SetStringsValue(name, val); err != nil { + t.Fatalf("SetStringsValue(%s): %v", name, err) + } + } + + qwords := map[string]uint64{ + "QwordOrdinary": 1234567890123, + // Above 2^32, so a REG_QWORD that was silently read as a REG_DWORD + // would truncate rather than merely be mistyped. + "QwordAbove32Bits": 1 << 40, + // The value that separates FormatUint from FormatInt. FormatInt renders + // this as -1, which is not a number that appears in any registry. + "QwordMax": math.MaxUint64, + } + for name, val := range qwords { + if err := k.SetQWordValue(name, val); err != nil { + t.Fatalf("SetQWordValue(%s): %v", name, err) + } + } + + // The control. REG_DWORD and REG_QWORD share formatValue's integer branch, + // so a DWORD asserted alongside is what shows that the QWORD results are + // not simply the DWORD path answering for everything. + if err := k.SetDWordValue("DwordControl", 42); err != nil { + t.Fatalf("SetDWordValue: %v", err) + } + + cases := []struct{ name, wantType, wantValue string }{ + {"MultiOrdinary", "REG_MULTI_SZ", "alpha\nbeta\ngamma"}, + {"MultiSingle", "REG_MULTI_SZ", "only"}, + {"MultiWithBlank", "REG_MULTI_SZ", "alpha\n\ngamma"}, + {"QwordOrdinary", "REG_QWORD", "1234567890123"}, + {"QwordAbove32Bits", "REG_QWORD", strconv.FormatUint(1<<40, 10)}, + {"QwordMax", "REG_QWORD", strconv.FormatUint(math.MaxUint64, 10)}, + {"DwordControl", "REG_DWORD", "42"}, + } + + for _, tc := range cases { + r := &defRegistryWindows{key: `HKCU\` + typeTestSubKey + `\` + tc.name} + + if exists, err := r.Exists(); err != nil || !exists { + t.Errorf("%s: Exists() = %v, %v; want true, nil", tc.name, exists, err) + continue + } + gotType, err := r.Type() + if err != nil { + t.Errorf("%s: Type(): %v", tc.name, err) + continue + } + if gotType != tc.wantType { + t.Errorf("%s: Type() = %q, want %q", tc.name, gotType, tc.wantType) + } + gotValue, err := r.Value() + if err != nil { + t.Errorf("%s: Value(): %v", tc.name, err) + continue + } + if gotValue != tc.wantValue { + t.Errorf("%s: Value() = %q, want %q", tc.name, gotValue, tc.wantValue) + } + } +} diff --git a/system/registry_windows_view_test.go b/system/registry_windows_view_test.go new file mode 100644 index 0000000..3f18374 --- /dev/null +++ b/system/registry_windows_view_test.go @@ -0,0 +1,106 @@ +//go:build windows + +package system + +import ( + "testing" + + "golang.org/x/sys/windows/registry" +) + +// TestNativeViewAddsNoAccessFlag is the safety property behind the whole +// `view:` attribute, and it is the one worth pinning hardest. +// +// Every gossfile written before this attribute existed has no `view:`, parses +// as native, and MUST produce byte-identical registry access to the code that +// had no concept of views. If native ever starts adding WOW64_64KEY "because +// syver is a 64-bit process anyway", that is a silent behaviour change to every +// existing spec on every Windows host, and nothing else in the suite would +// notice. +func TestNativeViewAddsNoAccessFlag(t *testing.T) { + r := &defRegistryWindows{key: `HKLM\SOFTWARE\Syver\X`} + + if got := r.accessMask(); got != uint32(registry.QUERY_VALUE) { + t.Fatalf("default accessMask = %#x, want exactly QUERY_VALUE (%#x): "+ + "an unset view must not change how the registry is opened", + got, uint32(registry.QUERY_VALUE)) + } + + if err := r.SetView("native"); err != nil { + t.Fatalf("SetView(native): %v", err) + } + if got := r.accessMask(); got != uint32(registry.QUERY_VALUE) { + t.Fatalf("explicit native accessMask = %#x, want exactly QUERY_VALUE (%#x)", + got, uint32(registry.QUERY_VALUE)) + } +} + +func TestViewSelectsTheWow64Flag(t *testing.T) { + for _, tc := range []struct { + view string + want uint32 + }{ + {"32", uint32(registry.QUERY_VALUE) | registry.WOW64_32KEY}, + {"64", uint32(registry.QUERY_VALUE) | registry.WOW64_64KEY}, + } { + r := &defRegistryWindows{key: `HKLM\SOFTWARE\Syver\X`} + if err := r.SetView(tc.view); err != nil { + t.Fatalf("SetView(%q): %v", tc.view, err) + } + if got := r.accessMask(); got != tc.want { + t.Errorf("view %q gave accessMask %#x, want %#x", tc.view, got, tc.want) + } + } +} + +// An unusable view must fail the check that used it, not vanish. SetView +// returns nil by design -- the error is carried and surfaces from the +// accessors, so it lands on the assertion rather than in a constructor whose +// return value nothing inspects. +func TestInvalidViewFailsTheCheckRatherThanDisappearing(t *testing.T) { + r := &defRegistryWindows{key: `HKLM\SOFTWARE\Syver\X`} + if err := r.SetView("x86"); err != nil { + t.Fatalf("SetView is not the place the error surfaces; it returned %v", err) + } + + if _, err := r.Exists(); err == nil { + t.Error("Exists() succeeded with an invalid view; the check would pass or fail " + + "on a reading from a view the author never asked for") + } + if _, err := r.Value(); err == nil { + t.Error("Value() succeeded with an invalid view") + } + if _, err := r.Type(); err == nil { + t.Error("Type() succeeded with an invalid view") + } +} + +// FEAT-012 W2-6. Every REG_* type must have a name, because a `type:` +// assertion cannot be written against UNKNOWN(n). typeName is pure, so this is +// a cheap and complete check rather than a sampled one. +func TestTypeNameCoversEveryRegistryType(t *testing.T) { + for raw, want := range map[uint32]string{ + registry.NONE: "REG_NONE", + registry.SZ: "REG_SZ", + registry.EXPAND_SZ: "REG_EXPAND_SZ", + registry.BINARY: "REG_BINARY", + registry.DWORD: "REG_DWORD", + registry.DWORD_BIG_ENDIAN: "REG_DWORD_BIG_ENDIAN", + registry.LINK: "REG_LINK", + registry.MULTI_SZ: "REG_MULTI_SZ", + registry.RESOURCE_LIST: "REG_RESOURCE_LIST", + registry.FULL_RESOURCE_DESCRIPTOR: "REG_FULL_RESOURCE_DESCRIPTOR", + registry.RESOURCE_REQUIREMENTS_LIST: "REG_RESOURCE_REQUIREMENTS_LIST", + registry.QWORD: "REG_QWORD", + } { + if got := typeName(raw); got != want { + t.Errorf("typeName(%d) = %q, want %q", raw, got, want) + } + } + + // A type outside the documented set must still say so rather than be + // silently renamed to something plausible. + if got := typeName(9999); got == "" || got[:7] != "UNKNOWN" { + t.Errorf("typeName(9999) = %q, want an UNKNOWN(...) form", got) + } +} From 76b72b4c7cdee701bf361c8b92daac82252f51bd Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Wed, 9 Sep 2026 14:28:05 +0100 Subject: [PATCH 17/21] test(windows): assert the new registry grammar and record the new totals Signed-off-by: Mark Bolwell --- integration-tests/run-validate-tests.sh | 24 +++++-- .../syver/windows/tests/registry.goss.yaml | 63 ++++++++++++++++++- 2 files changed, 81 insertions(+), 6 deletions(-) diff --git a/integration-tests/run-validate-tests.sh b/integration-tests/run-validate-tests.sh index 851116d..374e974 100755 --- a/integration-tests/run-validate-tests.sh +++ b/integration-tests/run-validate-tests.sh @@ -62,14 +62,28 @@ fi # degrading to `skip` reads as a clean pass on a suite that got smaller. Over a # third of the fixtures here carry `skip: true`, so that is not hypothetical. # -# WHICH NUMBERS ARE SAFE TO PIN, measured 2026-09-08 rather than assumed: +# WHICH NUMBERS ARE SAFE TO PIN, measured rather than assumed. This comment is +# the ONE place the per-platform totals are written down; anything else that +# needs them should re-run the loop below rather than repeat them. # * `Count` is a property of the FIXTURE, not the host. Running every # platform's fixtures against a locally built linux/amd64 binary reproduced -# CI's totals exactly -- 74 for linux-arm64, 82 for darwin, 122 for windows. -# Safe to pin everywhere, and seedable from any machine. +# CI's totals exactly -- 74 for linux-arm64, 82 for darwin, 122 for windows, +# measured 2026-09-08 on devel at a4d4594. Safe to pin everywhere, and +# seedable from any machine. Re-measured 2026-09-09 on +# feature/windows-registry-and-job-objects, windows is 130: the registry +# fixture went from 12 assertions to 20. Note the windows total counts the +# 13 fixtures gossfile.goss.yaml aggregates twice, once directly and once +# through the aggregate; registry.goss.yaml is NOT one of those 13. # * `Skipped` is host-independent on linux and darwin (32 and 44, both exactly -# matching CI) but NOT on Windows: the same fixtures skip 33 assertions when -# driven from a Linux host and 19 on a real Windows Server runner. So +# matching CI) but NOT on Windows. Driven from a Linux host the windows +# fixtures skipped 33 on devel at a4d4594 and skip 36 on +# feature/windows-registry-and-job-objects, both measured 2026-09-09 by +# the same method as the Count note above -- the windows fixtures run +# against a locally built linux/amd64 binary, which reproduces a4d4594's +# recorded 122/33 exactly, so the method is checkable rather than trusted. +# The real-Windows-host figure was 19 on a4d4594; it is +# NOT 19 any more, because registry.goss.yaml gained two `skip: true` +# entries, and nobody has re-run it on Windows to say what it is. So # Windows fixtures deliberately declare no expect-skipped. Seed it from a # real run on that platform, never by inference from another one. # * `Failed` is host-dependent by design and is NOT pinned here. That is what diff --git a/integration-tests/syver/windows/tests/registry.goss.yaml b/integration-tests/syver/windows/tests/registry.goss.yaml index 0124b3f..e6290a0 100644 --- a/integration-tests/syver/windows/tests/registry.goss.yaml +++ b/integration-tests/syver/windows/tests/registry.goss.yaml @@ -1,4 +1,4 @@ -# expect-count: 12 +# expect-count: 20 # expect-skipped is deliberately absent: these fixtures skip a # different number of assertions on a real Windows host than when # driven from another platform. Seed it from a Windows run only. @@ -57,3 +57,64 @@ registry: HKLM\SOFTWARE\Microsoft\Windows NT\CurrentVersion\ProfileList: exists: false + + # FEAT-012 W2-3. The spellings regedit's address bar and Get-ItemProperty + # actually show. Before this, a path copied from either had to be hand-edited + # into the short form before syver would take it. + HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\ProductName: + exists: true + value: + match-regexp: "Windows.*" + + HKLM:\SOFTWARE\Microsoft\Windows NT\CurrentVersion\ProductName: + exists: true + + # FEAT-012 W2-4. `view: native` must mean exactly what no view at all means, + # so this entry and the first one in this file are the same assertion written + # two ways. If they ever disagree, adding the attribute changed the default. + productname-explicit-native: + name: HKLM\SOFTWARE\Microsoft\Windows NT\CurrentVersion\ProductName + exists: true + view: native + + productname-64bit-view: + name: HKLM\SOFTWARE\Microsoft\Windows NT\CurrentVersion\ProductName + exists: true + view: "64" + + # `view: "32"` must RESOLVE, not merely be accepted. ProductName is present + # in both views (measured on Windows Server 2025, 2026-09-09: + # `reg query "HKLM\SOFTWARE\Microsoft\Windows NT\CurrentVersion" /v ProductName + # /reg:32` and `/reg:64` both succeed), so this asserts the redirected lookup + # works rather than asserting a difference. + productname-32bit-view: + name: HKLM\SOFTWARE\Microsoft\Windows NT\CurrentVersion\ProductName + exists: true + view: "32" + + # The pair where the two views genuinely DISAGREE, which is the whole point of + # the attribute. Skipped for the same reason the HardenedPaths entry above is: + # it depends on the image, not on syver. + # + # MEASURED on Windows Server 2025, 2026-09-09, rather than reasoned: + # reg query "HKLM\SOFTWARE\Microsoft\Windows Defender" /reg:64 -> exists + # reg query "HKLM\SOFTWARE\Microsoft\Windows Defender" /reg:32 -> absent + # Defender writes only to the 64-bit branch. It can be absent or removed on a + # hardened or minimal image, so a CI runner is not guaranteed to match and + # these must not gate the suite. + # + # An earlier version of this pair used HKLM\SOFTWARE\Wow6432Node and asserted + # it was absent from the 32-bit view, reasoning that the view would redirect + # into a nested Wow6432Node. That was WRONG -- measured the same day, it exists + # under BOTH views -- and it would have failed CI. Do not restore it. + defender-64bit-view: + name: HKLM\SOFTWARE\Microsoft\Windows Defender\ + exists: true + view: "64" + skip: true + + defender-32bit-view: + name: HKLM\SOFTWARE\Microsoft\Windows Defender\ + exists: false + view: "32" + skip: true From 3bca090a19ba29b4a03d06062b73beaefc032b80 Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Wed, 9 Sep 2026 14:28:30 +0100 Subject: [PATCH 18/21] docs: view:, the trailing-backslash rule, and Windows process trees Signed-off-by: Mark Bolwell --- CHANGELOG.md | 55 +++++++++++++++++++++++- docs/gossfile.md | 107 ++++++++++++++++++++++++++++++++++++++-------- docs/platforms.md | 28 ++++++++---- docs/schema.yaml | 27 ++++++++++-- docs/testing.md | 12 ++++-- docs/windows.md | 35 ++++++++++++--- 6 files changed, 224 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd815ca..fc3ac2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,59 @@ # Changelog -## 0.11.3 based on krameff/goss v0.6.0 - container package description +## 0.12.0 based on krameff/goss v0.6.0 - Windows registry grammar and process trees + + + +- windows registry + - hive names accept the spellings Windows tools actually print. `regedit`'s + address bar shows `HKEY_LOCAL_MACHINE\...` and `Get-ItemProperty` shows + `HKLM:\...`; both, and the long forms with the PowerShell colon, now parse + alongside the short names, case-insensitively. Previously a path copied out + of either tool had to be hand-edited before syver would take it + - **new attribute `view:`** -- `32`, `64` or `native`, selecting the WOW64 + registry view a check reads. On 64-bit Windows some keys exist twice, and a + spec had no way to say which copy it meant: it got whichever syver's own + architecture saw. `native` is the default and behaves exactly as every + existing gossfile did, which a test pins rather than assumes + - `type:` reports the **full** `REG_*` set. `REG_NONE`, `REG_LINK`, + `REG_DWORD_BIG_ENDIAN` and the three `REG_RESOURCE_*` hardware descriptor + types previously came back as `UNKNOWN(n)`, so a type assertion against + them could not be written. `REG_DWORD_BIG_ENDIAN` is deliberately not read + as an integer: doing so would return a confidently wrong number in the + wrong byte order, so it renders as hex like the other opaque types + - a value lookup that misses while a **key of that name exists in the same + place** now says so. This is the one shape where a truthful answer reliably + answers a different question from the one asked: `HKLM\...\ProfileList` + asks about a value and is false, `HKLM\...\ProfileList\` asks about the + key and is true. Asserting `exists: false` against the first therefore + PASSES while testing nothing the author intended. The trailing backslash is + still not guessed at or made optional -- guessing moves the ambiguity + somewhere you cannot see it -- but the confusable case is no longer silent + - `REG_EXPAND_SZ` is documented as compared **unexpanded**: a value holding + `%SystemRoot%\System32` is matched as that literal text. Unchanged + behaviour, previously unstated + +- windows command timeouts + - a `command:` that timed out killed the process syver started and left + anything that process had spawned running. `command:` runs through + `cmd /c`, so the thing syver starts is a shell and the thing that hangs is + the shell's child -- meaning the leak was the normal case on Windows, not + an edge one. The process now runs inside a **Job Object** whose closure + terminates the whole tree + - this makes Windows **stronger** than Linux and macOS here, which is worth + stating because the documentation said the opposite. A process that calls + `setsid` leaves the POSIX process group and is permanently out of reach; a + process cannot leave a job unless it was created to break away and the job + permits it. The daemonising case that escapes on POSIX does not escape on + Windows + - a command that **succeeds** is untouched. Only a timeout terminates the + tree, so a check that deliberately starts a background process and exits + zero still leaves it running + - the Windows PowerShell probe path used by `service:` had no process-group + protection at all, and now shares the same mechanism - container image - the package page for the published image showed **no description**. The diff --git a/docs/gossfile.md b/docs/gossfile.md index ffd58c2..0d0a8eb 100644 --- a/docs/gossfile.md +++ b/docs/gossfile.md @@ -250,17 +250,24 @@ the hash for backwards compatibility `exec` string carries the same power, since whoever controls the vars file controls part of the command. - On timeout the command is killed along with any child processes it started - (on Linux and macOS; see [platform support](platforms.md) for Windows). - - One exception, and it is deliberate rather than an oversight: a process that - detaches itself into a new session -- `setsid`, `nohup`, most daemons -- has - left the group syver signals, so it survives the timeout and is reparented to - init. Syver has no portable way to find it again. Under `serve` each such - timeout leaves one process behind for as long as it chooses to run, so a spec - that repeatedly times out a daemonising command will accumulate them. If a - check needs to start a daemon, have it start the daemon and exit, rather than - relying on the timeout to clean up. + On timeout the command is killed along with any child processes it started, + on every platform. + + One exception on Linux and macOS, deliberate rather than an oversight: a + process that detaches itself into a new session -- `setsid`, `nohup`, most + daemons -- has left the group syver signals, so it survives the timeout and + is reparented to init. Syver has no portable way to find it again. Under + `serve` each such timeout leaves one process behind for as long as it chooses + to run, so a spec that repeatedly times out a daemonising command will + accumulate them. If a check needs to start a daemon, have it start the daemon + and exit, rather than relying on the timeout to clean up. + + **Windows has no such exception.** The process tree is held in a Job Object, + which a process cannot leave unless it was created with + `CREATE_BREAKAWAY_FROM_JOB` and the job allows it, so a detaching child is + terminated with the rest. A command that succeeds is never touched: only a + timeout terminates the tree, so a check that deliberately starts a background + process and exits zero leaves it running. !!! note "timeout values" @@ -609,11 +616,19 @@ registry: exists: true ``` -Supported hives, as the first path segment: +Supported hives, as the first path segment. Each may be written short, long, +or with the PowerShell provider colon, so a path pasted from `regedit`'s address +bar or from `Get-ItemProperty` output works unedited: -`HKLM` (HKEY_LOCAL_MACHINE), `HKCU` (HKEY_CURRENT_USER), -`HKCR` (HKEY_CLASSES_ROOT), `HKU` (HKEY_USERS), -`HKCC` (HKEY_CURRENT_CONFIG). +| Short | Long | PowerShell | +| :-- | :-- | :-- | +| `HKLM` | `HKEY_LOCAL_MACHINE` | `HKLM:` | +| `HKCU` | `HKEY_CURRENT_USER` | `HKCU:` | +| `HKCR` | `HKEY_CLASSES_ROOT` | `HKCR:` | +| `HKU` | `HKEY_USERS` | `HKU:` | +| `HKCC` | `HKEY_CURRENT_CONFIG` | `HKCC:` | + +Hive names are case-insensitive. Attributes: @@ -622,13 +637,71 @@ Attributes: [matcher](#matchers). Note `REG_MULTI_SZ` is returned as its entries joined by newlines, not as a list, so use `contain-substring` or `match-regexp` rather than list matchers like `contain-element`. -* `type` -- the value's data type, one of `REG_SZ`, `REG_EXPAND_SZ`, - `REG_DWORD`, `REG_QWORD`, `REG_BINARY`, `REG_MULTI_SZ`. +* `type` -- the value's data type. Every `REG_*` type is reported, including + `REG_NONE`, `REG_LINK`, `REG_DWORD_BIG_ENDIAN` and the three `REG_RESOURCE_*` + hardware descriptor types. +* `view` -- which WOW64 registry view to read: `32`, `64`, or `native`. + Defaults to `native`, which is what every spec written before this attribute + existed gets, and means whatever the OS shows a 64-bit process. + +#### Choosing a view + +On 64-bit Windows some keys exist twice. A 32-bit program is redirected under +`Wow6432Node` and sees a different copy from a 64-bit one. `view:` lets a spec +say which copy it means instead of inheriting syver's own architecture: + +```yaml +registry: + # the redirected copy a 32-bit installer wrote, under Wow6432Node + HKLM\SOFTWARE\Vendor\Product\Version: + exists: true + view: "32" + # the native 64-bit copy. Same path, different branch -- so give the two + # entries distinct keys, or use `name:` to point both at one path + HKLM\SOFTWARE\Vendor\Product64\Version: + exists: true + view: "64" +``` + +Quote the value. Syver itself accepts `view: 32` unquoted -- the YAML integer is +coerced to a string on load, and renders back out as `view: "32"` -- but +[`schema.yaml`](schema.yaml) types the attribute as a string, so the unquoted +form fails schema validation against a spec syver would have run. Quoting keeps +the two in step. `view` is matched case-insensitively and surrounding whitespace +is ignored, so `NATIVE` and `native` are the same value. + +#### The trailing backslash, and why it is not guessed By default the last backslash-separated segment of the path is treated as the value name. Use the explicit `::` separator when that guess would be wrong, for example when a value name contains backslashes. +A **trailing backslash addresses the key itself** rather than a value, and the +difference is not cosmetic. These two ask different questions and both answer +truthfully: + +```text +HKLM\...\ProfileList exists -> false is there a VALUE named ProfileList? +HKLM\...\ProfileList\ exists -> true is there a KEY named ProfileList? +``` + +syver does not guess which you meant, because guessing would move the ambiguity +somewhere you cannot see it. Instead, when a value lookup misses and a key of +that name exists in the same place, it logs a warning naming the alternative. +Asserting `exists: false` against the first form therefore passes **and** tells +you it may not be testing what you think. + +A key can exist while having no default value, so `exists: true` on a path +ending in a backslash alongside a `value` lookup that reports not-found is a +coherent pair rather than a contradiction. + +#### REG_EXPAND_SZ is compared unexpanded + +A `REG_EXPAND_SZ` value is matched as stored. A value holding +`%SystemRoot%\System32` is compared as that literal text, not as +`C:\Windows\System32`. This is deliberate: benchmarks state the stored form, +and expansion depends on the environment of whoever is asking. + ### package Validates the state of a package diff --git a/docs/platforms.md b/docs/platforms.md index 49a0a17..4163a35 100644 --- a/docs/platforms.md +++ b/docs/platforms.md @@ -24,13 +24,15 @@ To try out the alpha functionality, you must do one of: * set an environment variable `SYVER_USE_ALPHA=1` (or the legacy `GOSS_USE_ALPHA=1`, which is still honoured; `SYVER_USE_ALPHA` wins when both are set to a non-empty value). -One concrete difference worth knowing before you rely on Windows: syver puts a -timed-out check's process into its own process group and kills the group, and -Windows has no equivalent, so there the started process is killed and anything it -spawned survives. On Linux and macOS only a process that deliberately detaches -into a new session escapes that way. Syver still bounds how long it waits, so a -check fails rather than hanging, but on Windows expect leftover processes after a -timeout more often than the other platforms. +One concrete difference worth knowing, and it now runs the other way. A +timed-out check's process tree is terminated on every platform, but by different +means and with different reach. Linux and macOS put the process into its own +process group and kill the group, which a process that deliberately detaches +into a new session (`setsid`, `nohup`, most daemons) escapes. Windows puts it +into a Job Object, which a process cannot leave unless it was created to break +away and the job permits it, so **the daemonising case that escapes on Linux and +macOS does not escape on Windows.** Syver bounds how long it waits either way, so +a check fails rather than hanging. The macOS and Windows support is community driven; there is no commitment to adding features / fixing bugs for those platforms. @@ -92,7 +94,7 @@ This matrix attempts to track parity across platforms. | | exit-status | {{ fully_supported }} | {{ work_partially }} | {{ work_partially }} | | | stdout | {{ fully_supported }} | {{ work_partially }} | {{ work_partially }} | | | stderr | {{ fully_supported }} | {{ not_automated }} | {{ not_automated }} | -| | timeout | {{ fully_supported }} | {{ not_automated }} | {{ not_automated }} | +| | timeout | {{ fully_supported }} | {{ not_automated }} | {{ work_partially }} | | **dns** | | {{ fully_supported }} | {{ work_partially }} | {{ work_partially }} | | | resolvable | {{ fully_supported }} | {{ work_partially }} | {{ work_partially }} | | | addrs | {{ fully_supported }} | {{ work_partially }} | {{ work_partially }} | @@ -133,6 +135,7 @@ This matrix attempts to track parity across platforms. | | exists | {{ n_a }} | {{ n_a }} | {{ work_partially }} | | | value | {{ n_a }} | {{ n_a }} | {{ work_partially }} | | | type | {{ n_a }} | {{ n_a }} | {{ work_partially }} | +| | view | {{ n_a }} | {{ n_a }} | {{ work_partially }} | | **mount** | | {{ fully_supported }} | {{ not_implemented }} | {{ not_implemented }} | | | exists | {{ fully_supported }} | {{ not_implemented }} | {{ not_implemented }} | | | opts | {{ fully_supported }} | {{ not_implemented }} | {{ n_a }} | @@ -242,6 +245,15 @@ passed before may now fail where it was never actually being checked. | `serve` | {{ fully_supported }} | {{community_supported}} | {{community_supported}} | | `validate` | {{ fully_supported }} | {{ work_partially }} | {{ work_partially }} | +`command:` `timeout` on **Windows** moved from *not automated* to *partially +tested* in this release: `util/procgroup_windows_test.go` asserts both that a +timed-out command's grandchild is terminated and that a succeeding command's +background child is not, and the `windows-latest` leg of `golangci.yaml` runs +`make test`, which passes no `-short`, so both execute in CI. The macOS cell is +unchanged: nothing there tests timeout expiry. No fixture on any platform +asserts what happens when a budget runs out, which is why this is *partially* +tested rather than fully. + The macOS and Windows cells above are measured from CI, not estimated. Every `add`, `help`, `serve` and `validate` cell describes a lane that runs on every push and passes. `autoadd` is genuinely untested on both: its fixture carries diff --git a/docs/schema.yaml b/docs/schema.yaml index 236bbd5..05a9bd2 100644 --- a/docs/schema.yaml +++ b/docs/schema.yaml @@ -477,15 +477,36 @@ definitions: returned as its entries joined by newlines, not as a list, so use substring or regexp matchers rather than list matchers. type: - description: the value's data type + description: >- + the value's data type. REG_EXPAND_SZ values are compared as stored, + unexpanded, so %SystemRoot% is matched literally. type: string enum: + - REG_NONE - REG_SZ - REG_EXPAND_SZ - - REG_DWORD - - REG_QWORD - REG_BINARY + - REG_DWORD + - REG_DWORD_BIG_ENDIAN + - REG_LINK - REG_MULTI_SZ + - REG_RESOURCE_LIST + - REG_FULL_RESOURCE_DESCRIPTOR + - REG_RESOURCE_REQUIREMENTS_LIST + - REG_QWORD + view: + description: >- + which WOW64 registry view to read. Defaults to native, which is what + the OS shows a 64-bit process and what every spec written before this + attribute existed gets. Quote it: this schema types the attribute as a + string, so an unquoted 32 or 64 fails validation here even though + syver itself coerces the YAML integer and accepts it. + type: string + enum: + - "32" + - "64" + - native + default: native skip: type: boolean default: false diff --git a/docs/testing.md b/docs/testing.md index e08ee2f..7c55060 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -175,10 +175,14 @@ exit code is for. `Count` is a property of the fixture and is pinned everywhere. worth knowing because it is not obvious. Skips are not purely declarative: a resource whose existence check fails has its remaining attributes reported as *skipped* rather than failed, so one missing file turns five further assertions -into skips. The Windows fixtures therefore skip 33 assertions when driven from a -Linux host and 19 on a real Windows host, where the files and registry keys -actually exist. Seed that value from a run on the platform itself, never by -inference from another one. +into skips. The Windows fixtures therefore skip materially more assertions when +driven from a Linux host than on a real Windows host, where the files and +registry keys actually exist and the dependent attributes run instead of +cascading. Seed that value from a run on the platform itself, never by inference +from another one -- and note that a real Windows figure goes stale the moment a +fixture gains a `skip: true` entry, which is not visible from any other +platform. `run-validate-tests.sh` prints both totals on every run and its header +comment records the last measured pair, with the date and commit. That cascade is also what makes `expect-skipped` worth pinning at all: a resource that quietly stops being present raises the skip count without failing diff --git a/docs/windows.md b/docs/windows.md index bbefe03..d5d8439 100644 --- a/docs/windows.md +++ b/docs/windows.md @@ -66,6 +66,13 @@ and `exists` on `user:`, `group:` and `interface:`. `registry:` is Windows-only, and is the resource most worth using here. +`command:` timeouts are also **stronger** here than on Linux and macOS. A +timed-out command's whole process tree is terminated through a Job Object, which +a process cannot leave unless it was built to break away. On POSIX a process that +calls `setsid` escapes the process group and survives. A command that *succeeds* +is never touched, so a check that deliberately starts a background process and +exits zero leaves it running. + **Mind the trailing backslash.** The last path segment is read as a *value* name, so a key check needs a trailing `\`: @@ -78,9 +85,19 @@ registry: exists: true # a VALUE named HardenedPaths exists. Different check. ``` -Omitting the backslash does not error, it quietly asks a different question and -answers it correctly, so a key check written that way reports `false` against a -key that plainly exists. See [gossfile](gossfile.md#registry) for the full path +Omitting the backslash does not error. It asks a different question and answers +it correctly, so a key check written that way reports `false` against a key that +plainly exists -- and because `exists: false` then *passes*, nothing fails to +draw your attention to it. It is no longer silent: when a value lookup misses +while a key of that name exists in the same place, syver logs a warning naming +the alternative path. The grammar itself is deliberately not guessed at, because +guessing moves the ambiguity somewhere you cannot see it. + +Hive names may be written short (`HKLM`), long (`HKEY_LOCAL_MACHINE`), or with +the PowerShell provider colon (`HKLM:`), so a path pasted from `regedit`'s +address bar or from `Get-ItemProperty` works unedited. A per-entry +`view: 32|64|native` selects the WOW64 registry view. +See [gossfile](gossfile.md#registry) for the full path grammar, including `::` for value names that themselves contain a backslash. It distinguishes three outcomes rather than two: a key that is absent, a key that exists, and a key that exists but could not be read. That last case used to be @@ -154,7 +171,7 @@ that four fixtures assert nothing at all. | --- | --- | --- | --- | | `gossfile` | 13 of 13 | 51 | aggregate of the others | | `command` | 6 of 6 | 18 | | -| `registry` | 7 of 8 | 12 | | +| `registry` | 12 of 15 | 20 | 3 skipped: one GPO-delivered, two Defender view-difference | | `file` | 2 of 2 | 7 | includes an absent-file case | | `http` | 1 of 1 | 3 | | | `group` | 3 of 3 | 3 | includes an absent-account case | @@ -174,9 +191,13 @@ that four fixtures assert nothing at all. **"Assertions" is not the same as "assertions that ran."** A resource whose existence check fails has its remaining attributes reported as *skipped* rather than failed, so one missing file turns five further assertions into skips. That -cascade is why the same fixtures skip 33 assertions when driven from a Linux -host and 19 on a real Windows Server host: on Windows the files and registry -keys are actually there, so the dependent attributes run instead of cascading. +cascade is why the same fixtures skip substantially more assertions when driven +from a Linux host than on a real Windows Server host: on Windows the files and +registry keys are actually there, so the dependent attributes run instead of +cascading. `integration-tests/run-validate-tests.sh` prints both totals on every +run and its header comment records the last measured pair with the date and +commit; do not restate them here, because the Windows figure can only be +re-measured on Windows and this page is edited far more often than that happens. It also means a resource quietly disappearing shows up as a rise in skips, not as a failure. From 242905ace3a0b62400f2982d882c9ae11a6ff0a7 Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Thu, 10 Sep 2026 12:01:51 +0100 Subject: [PATCH 19/21] Updated ready for 0.12.0 release Signed-off-by: Mark Bolwell --- CHANGELOG.md | 5 ----- RELEASES.md | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc3ac2f..36f7907 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,11 +2,6 @@ ## 0.12.0 based on krameff/goss v0.6.0 - Windows registry grammar and process trees - - - windows registry - hive names accept the spellings Windows tools actually print. `regedit`'s address bar shows `HKEY_LOCAL_MACHINE\...` and `Get-ItemProperty` shows diff --git a/RELEASES.md b/RELEASES.md index 0379117..0c98afc 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -59,6 +59,8 @@ artifacts and signatures always correspond to the tag as it now stands. ## Contents +* [v0.12.0 - Windows registry grammar and process trees](#v0120---windows-registry-grammar-and-process-trees) + -- **assembled, not released** * [v0.11.2 - Signed SBOMs and a patched base image](#v0112---signed-sboms-and-a-patched-base-image) * [v0.11.1 - Documentation site corrections](#v0111---documentation-site-corrections) * [v0.11.0 - Windows: stop returning confident wrong answers](#v0110---windows-stop-returning-confident-wrong-answers) @@ -75,6 +77,43 @@ artifacts and signatures always correspond to the tag as it now stands. --- +## v0.12.0 - Windows registry grammar and process trees + +**NOT RELEASED. Assembled on two feature branches, neither merged, nothing +tagged.** This entry exists so the work is traceable before it ships; replace +the pending fields at tag time rather than writing them now. + +| Field | Value | +| --- | --- | +| Released | pending | +| Tag | pending | +| Commit | pending | +| Base | krameff/goss v0.6.0 | +| Integration branch | `feature/windows-depth-wave2` (FEAT-013) and `feature/windows-registry-and-job-objects` (FEAT-012, FEAT-017), the second branched from the first. Both still need a PR; pushes do not build feature branches, so neither has been through CI | +| Scope | measure at tag time: `git diff --stat devel..feature/windows-registry-and-job-objects` | +| Changelog | [0.12.0](CHANGELOG.md#0120-based-on-krameffgoss-v060---windows-registry-grammar-and-process-trees) | + +**Why this is a minor and not a patch.** `registry:` gains a `view:` attribute. +The resource layer is deliberately cross-platform, so a new attribute appears in +`docs/schema.yaml` on every platform, not just the one that honours it. That is +user-facing spec syntax and cannot ship under a patch. + +It also carries the container-package-description work that had been assembled +on `devel` under an unreleased `0.11.3` heading. That heading no longer exists; +the work rides here. + +**Validation, stated because it is unusually good for Windows work and unusually +uneven.** FEAT-012 and FEAT-017 were exercised on two independent Windows images +-- a Windows Server 2025 guest and a Windows 11 Enterprise host -- with the unit +tests passing on both and the registry fixture producing an identical +`Count: 20, Failed: 0, Skipped: 3`. The `view:` attribute was proven to read +genuinely different data by creating one key through each WOW64 view with +different values and reading both back through the shipped binary. +**FEAT-013, on the older branch, has never been run on Windows at all.** Do not +let the strength of the first two imply anything about the third. + +--- + ## v0.11.2 - Signed SBOMs and a patched base image | Field | Value | From 129cfcf19a0eb5df0b086489a74a94e3b80ba221 Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Thu, 10 Sep 2026 12:04:49 +0100 Subject: [PATCH 20/21] Updated file Signed-off-by: Mark Bolwell --- RELEASES.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/RELEASES.md b/RELEASES.md index 0c98afc..5bb5c21 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -109,8 +109,13 @@ tests passing on both and the registry fixture producing an identical `Count: 20, Failed: 0, Skipped: 3`. The `view:` attribute was proven to read genuinely different data by creating one key through each WOW64 view with different values and reading both back through the shipped binary. -**FEAT-013, on the older branch, has never been run on Windows at all.** Do not -let the strength of the first two imply anything about the third. +FEAT-013 was the gap in this record and it is now closed: on 2026-09-10 its +tests were run explicitly on Windows 11 from a binary built at `3bca090` and all +passed, including `TestMountReportsUnsupportedNotNotFound` (the mount fix +itself), `TestProcessNeverReportsNothingSuccessfully`, the four +`TestCollectPerProcess` cases and the `RealPath` set. Two mount tests skip by +design, being POSIX-only. All three of FEAT-012, FEAT-013 and FEAT-017 have now +been exercised on a real Windows host. --- From 3df9949326d17681e8dc2d4c0b82fd983d6f4c67 Mon Sep 17 00:00:00 2001 From: Mark Bolwell Date: Thu, 10 Sep 2026 12:18:15 +0100 Subject: [PATCH 21/21] fix(windows): make the Job Object attach flag atomic Signed-off-by: Mark Bolwell --- util/procgroup_windows.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/util/procgroup_windows.go b/util/procgroup_windows.go index 82d4331..7b3dfd4 100644 --- a/util/procgroup_windows.go +++ b/util/procgroup_windows.go @@ -6,6 +6,7 @@ package util import ( "os/exec" "sync" + "sync/atomic" "unsafe" "golang.org/x/sys/windows" @@ -19,9 +20,17 @@ import ( // worse than the behaviour this replaces, because we have already taken over // exec.CommandContext's default cancel. The fallback path depends on knowing // which of the two happened. +// +// attached MUST be atomic. It is written by attachProcessGroup on the goroutine +// running Run, and read by the cmd.Cancel closure, which os/exec invokes from +// its own watchCtx goroutine the moment the context expires. Those two race by +// construction whenever a command times out -- the only path this file exists +// to serve. A plain bool passed every test on two Windows hosts and was caught +// by `-race` on the CI runner: neither host had a C compiler, so cgo was +// unavailable and the detector never ran locally. type jobState struct { handle windows.Handle - attached bool + attached atomic.Bool } // jobs maps a command to its Job Object. @@ -81,7 +90,7 @@ func configureProcessGroup(cmd *exec.Cmd) { jobs.Store(cmd, state) cmd.Cancel = func() error { - if !state.attached { + if !state.attached.Load() { // The job is empty, so closing it terminates nothing. Fall back to // what exec.CommandContext would have done unaided. if cmd.Process == nil { @@ -142,7 +151,7 @@ func attachProcessGroup(cmd *exec.Cmd) error { if err := windows.AssignProcessToJobObject(state.handle, h); err != nil { return err } - state.attached = true + state.attached.Store(true) return nil }