From a90755a59fda7474278265047c91b81c15cbcc50 Mon Sep 17 00:00:00 2001 From: Zoltan Csizmadia Date: Sun, 20 Sep 2026 08:36:17 -0500 Subject: [PATCH] fix: autostart works on a profile with no Run key Closes #444. Found by the nightly acceptance run, two days after #426 wired it up. skrog: applying config: opening the Run key: The system cannot find the file specified. HKCU\Software\Microsoft\Windows\CurrentVersion\Run is created by Windows ON DEMAND. A profile that has never registered a logon entry does not have one -- the normal state of a fresh hosted runner, and of a new or freshly-imaged user profile. All three entry points used OpenKey, which fails with ERROR_FILE_NOT_FOUND when the key is absent: Enable could not register autostart at all Status returned an error, so `skrog status` and `doctor` failed Disable returned an error, contradicting its own doc comment -- "Removing an entry that does not exist is success: the user asked for a state, not an action." It handled a missing VALUE and not a missing KEY, which is the same answer one level up. The message compounded it: "cannot find the file specified" reads as a missing FILE and sends the user looking for skrogw.exe. Enable now uses CreateKey, the standard idiom for the Run key -- it opens an existing key unchanged, so it costs nothing where one is already there. Status reports "not registered" and Disable reports success, because an absent key is an answer rather than a failure. Why the tests did not catch it ------------------------------ From the existing helper: // The scratch key must exist for OpenKey(SET_VALUE) to succeed. k, _, err := registry.CreateKey(...) The test created the precondition the product assumed. That comment was the bug report, sitting in the file the whole time. useAbsentScratchKey is its opposite: it points at a key that does not exist and asserts it stays that way until the code under test creates it. Three tests use it, one per entry point. Verification ------------ Each fix controlled separately, and both reproduce the nightly failure verbatim: Enable back to OpenKey -> "Enable with no Run key: opening the Run key: The system cannot find the file specified." Status guard removed -> "Status with no Run key returned an error: opening the Run key: The system cannot find the file specified." Severity is real but narrow: most Windows profiles have the key. It bites a fresh profile, a hardened or imaged machine, and every clean CI runner -- which is exactly the headless-CI case the README sells. --- internal/autostart/autostart_windows.go | 24 +++++- internal/autostart/autostart_windows_test.go | 82 ++++++++++++++++++++ 2 files changed, 105 insertions(+), 1 deletion(-) diff --git a/internal/autostart/autostart_windows.go b/internal/autostart/autostart_windows.go index 20ac6c7..d621c16 100644 --- a/internal/autostart/autostart_windows.go +++ b/internal/autostart/autostart_windows.go @@ -13,7 +13,9 @@ package autostart import ( + "errors" "fmt" + "io/fs" "os" "path/filepath" "strings" @@ -42,7 +44,13 @@ func Enable(skrogExe string) error { "(a console binary at logon would flash a console window): %w", err) } - k, err := registry.OpenKey(registry.CURRENT_USER, runKeyPath, registry.SET_VALUE) + // CreateKey, not OpenKey: the Run key is created on demand by Windows and + // is NOT guaranteed to exist (#444). A profile that has never had a logon + // entry has no key, and OpenKey then fails with "The system cannot find + // the file specified" — which reads as a missing FILE and sends the user + // looking for skrogw.exe. CreateKey opens an existing key unchanged, so + // this costs nothing on the machines that already have one. + k, _, err := registry.CreateKey(registry.CURRENT_USER, runKeyPath, registry.SET_VALUE) if err != nil { return fmt.Errorf("opening the Run key: %w", err) } @@ -59,6 +67,13 @@ func Enable(skrogExe string) error { // success: the user asked for a state, not an action. func Disable() error { k, err := registry.OpenKey(registry.CURRENT_USER, runKeyPath, registry.SET_VALUE) + if errors.Is(err, registry.ErrNotExist) || errors.Is(err, fs.ErrNotExist) { + // No Run key at all, so nothing is registered — which is the state + // the caller asked for. The comment above already said a missing + // VALUE is success; a missing KEY is the same answer one level up, + // and failing here contradicted it (#444). + return nil + } if err != nil { return fmt.Errorf("opening the Run key: %w", err) } @@ -106,6 +121,13 @@ func DisableIfOwned(installDir string) (bool, error) { // Status returns whether autostart is registered, and the command if so. func Status() (enabled bool, command string, err error) { k, err := registry.OpenKey(registry.CURRENT_USER, runKeyPath, registry.QUERY_VALUE) + if errors.Is(err, registry.ErrNotExist) || errors.Is(err, fs.ErrNotExist) { + // No key means nothing is registered, which is an answer and not a + // failure (#444). Reporting an error here made `skrog status` and + // `doctor` fail on a profile that had simply never autostarted + // anything. + return false, "", nil + } if err != nil { return false, "", fmt.Errorf("opening the Run key: %w", err) } diff --git a/internal/autostart/autostart_windows_test.go b/internal/autostart/autostart_windows_test.go index efbb24a..9837bb6 100644 --- a/internal/autostart/autostart_windows_test.go +++ b/internal/autostart/autostart_windows_test.go @@ -194,3 +194,85 @@ func TestDisableIfOwnedMatchesAResolvedDir(t *testing.T) { t.Fatal("the owner could not remove its own entry when passing a resolved directory") } } + +// useAbsentScratchKey points the package at a key that does NOT exist, and +// makes sure it stays that way until the code under test creates it. +// +// This is the case the existing helper cannot cover, because it creates the +// key first — with a comment saying it must exist for OpenKey(SET_VALUE) to +// succeed. That comment is the bug report: the product assumed a precondition +// the test then supplied for it (#444). +func useAbsentScratchKey(t *testing.T) { + t.Helper() + orig := runKeyPath + runKeyPath = `Software\SkrogTest\AbsentRun` + + // Make sure a previous run did not leave it behind. + registry.DeleteKey(registry.CURRENT_USER, runKeyPath) + if k, err := registry.OpenKey(registry.CURRENT_USER, runKeyPath, registry.QUERY_VALUE); err == nil { + k.Close() + t.Fatalf("%s exists; this test is about the case where it does not", runKeyPath) + } + + t.Cleanup(func() { + registry.DeleteKey(registry.CURRENT_USER, runKeyPath) + registry.DeleteKey(registry.CURRENT_USER, `Software\SkrogTest`) + runKeyPath = orig + }) +} + +// A profile with no Run key at all must still be able to enable autostart. +// +// Windows creates HKCU\...\CurrentVersion\Run on demand, so a profile that has +// never registered a logon entry does not have one. OpenKey then fails with +// "The system cannot find the file specified", which `skrog install --config` +// surfaced as: +// +// skrog: applying config: opening the Run key: The system cannot find the file specified. +// +// — a message that reads as a missing FILE and sends the user looking for +// skrogw.exe. Found by the nightly acceptance run on a clean hosted runner. +func TestEnableCreatesTheRunKeyWhenAbsent(t *testing.T) { + useAbsentScratchKey(t) + exe := fakeInstall(t, true) + + if err := Enable(exe); err != nil { + t.Fatalf("Enable with no Run key: %v", err) + } + enabled, cmd, err := Status() + if err != nil { + t.Fatalf("Status: %v", err) + } + if !enabled { + t.Error("autostart not registered after Enable created the key") + } + if !strings.Contains(cmd, "skrogw.exe") { + t.Errorf("Run entry = %q, want the skrogw launcher", cmd) + } +} + +// Status on a profile with no Run key is "not enabled", not an error. It used +// to fail, which broke `skrog status` and `doctor` on such a profile. +func TestStatusWithNoRunKeyIsNotAnError(t *testing.T) { + useAbsentScratchKey(t) + + enabled, cmd, err := Status() + if err != nil { + t.Fatalf("Status with no Run key returned an error: %v", err) + } + if enabled || cmd != "" { + t.Errorf("Status = (%v, %q), want (false, \"\")", enabled, cmd) + } +} + +// Disable's own doc comment says removing an entry that does not exist is +// success — "the user asked for a state, not an action". That held for a +// missing VALUE and not for a missing KEY, which is the same answer one level +// up. +func TestDisableWithNoRunKeyIsSuccess(t *testing.T) { + useAbsentScratchKey(t) + + if err := Disable(); err != nil { + t.Errorf("Disable with no Run key: %v", err) + } +}