From 1242b22e4fa3624b5e65670d22fd98d40466288a Mon Sep 17 00:00:00 2001 From: Mikhail Yenuchenko Date: Thu, 6 Aug 2026 16:53:59 +0300 Subject: [PATCH] fix: Windows command quoting, atomic JSON/log writes, restore dropped test Implements items 1-3 of the whole-project review's suggested order (docs/PROJECT_REVIEW_PLAN.md): - Restore TestJobListViewIsCompact, accidentally dropped by 5b0e6fe; drop the redundant TestDefaultConfigUsesDetailedJobList row from TESTS.md and document the two other doc gaps the review found. - Fix quoteLeadingWindowsProgramPath to find the earliest file-extension match at a word boundary instead of the first extension in list order, so a .bat/.cmd command whose argument ends in .exe no longer has its whole command line mistaken for the program path. - Write gosentry.json, jobs.json, and run log files atomically (temp file + rename) so a crash or power loss mid-write can no longer leave a truncated file. Wire Service.Stop() into the app shutdown path so it actually runs, cancelling the run context for in-flight runs. Co-Authored-By: Claude Sonnet 5 --- docs/CHANGELOG.md | 11 +++++++ docs/TESTS.md | 16 +++++++++-- src/domain/config_test.go | 21 ++++++++++++++ src/runner/invocation_windows.go | 47 +++++++++++++++++++++++------- src/runner/logfile.go | 40 +++++++++++++++++++++++++- src/runner/runner_windows_test.go | 41 ++++++++++++++++++++++++++ src/storage/store.go | 48 +++++++++++++++++++++++++++---- src/storage/store_test.go | 41 ++++++++++++++++++++++++++ src/ui/run.go | 6 ++++ 9 files changed, 253 insertions(+), 18 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 4db557b..0098655 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -21,6 +21,17 @@ the app icon (experimental).** - On Windows, failure toasts can show the app icon: after `NewWindow`, `AppMetadata.Icon` is registered so Fyne picks up artwork without calling `SetIcon`, which would override the PE multi-size window/taskbar icon. +- Fixed a Windows quoting bug where a job whose **Command** field held a whole + command line (a `.bat`/`.cmd` wrapper followed by an argument that itself + ended in `.exe`) had its entire command line mistaken for the program path, + so the run failed with an unmappable shell error. The program path is now + found by the earliest file-extension match at a word boundary, not the first + extension in list order. +- `gosentry.json` and `jobs.json` (and run log files) are now written + atomically — to a temp file, then renamed into place — so a crash or power + loss mid-write can no longer leave a truncated or empty file. `Service.Stop()` + is now called when the app quits, which also makes the run context + cancellation reach in-flight runs on shutdown. **Jobs:** diff --git a/docs/TESTS.md b/docs/TESTS.md index fc4dfd6..f23842c 100644 --- a/docs/TESTS.md +++ b/docs/TESTS.md @@ -100,7 +100,6 @@ Tests autostart argument helpers and the jobs-list density normalization rule. | `TestAutostartArguments` | Verifies `AutostartArguments` returns `--start-in-tray` when the tray is enabled and an empty string when it is off. | | `TestResolveStartHidden` | Verifies hidden autostart requires both the CLI flag and `KeepRunningInTray`. | | `TestJobListViewIsCompact` | Verifies only the exact `"compact"` value selects one-line rows: empty, differently-cased, and unrecognised values all read as detailed. | -| `TestDefaultConfigUsesDetailedJobList` | Verifies `DefaultConfig` selects the detailed job list. | --- @@ -554,6 +553,19 @@ Tests main view construction with an injected `*app.Service`. --- +### src/ui/notify_timing_test.go + +**Package:** `ui` + +Tests the failure-notification timing diagnostics added in 1.0.2. + +| Test | Purpose | +|------|---------| +| `TestNotificationTimingFormatLine` | Verifies `notificationTiming.formatLine` renders the job name and the three millisecond deltas (`ms_after_run`, `ms_fyne_do`, `ms_send`) plus their sum (`ms_app_total`). | +| `TestAppendNotificationTimingLogWritesHeaderAndRow` | Verifies `appendNotificationTimingLog` creates `notify-timing.log` with its header on first write and appends a row containing the job name. | + +--- + ## Test Design Principles 1. **Isolation** — Tests use `t.TempDir()` for file operations and `t.Setenv()` for environment variables to avoid affecting system state. @@ -603,6 +615,6 @@ A coverage run over the non-UI packages reports these as uncovered. All are intentional; none is an oversight to be "fixed" with a test. - The real `Clock` — a fake is injected everywhere it is used. -- `storage.OpenStore`, `storage.ResolvePaths`, `app.Service.Start`, `app.Service.Open` — process entry points, exercised by running the app. +- `storage.OpenStore`, `storage.ResolvePaths`, `storage.PeekKeepRunningInTray`, `app.Service.Start`, `app.Service.Open` — process entry points, exercised by running the app. - The autostart and desktop-icon wrappers — OS integration, driven only on a real desktop. - `app.Service.ShouldNotifyOnFailure` — a getter under the mutex. diff --git a/src/domain/config_test.go b/src/domain/config_test.go index ee782fc..0e15510 100644 --- a/src/domain/config_test.go +++ b/src/domain/config_test.go @@ -26,3 +26,24 @@ func TestResolveStartHidden(t *testing.T) { } } } + +// TestJobListViewIsCompact pins the normalization rule: only the exact +// "compact" value selects the one-line rows, so empty and unrecognised values +// (including configs written before the field existed) keep the detailed look. +func TestJobListViewIsCompact(t *testing.T) { + cases := []struct { + view JobListView + want bool + }{ + {JobListViewCompact, true}, + {JobListViewDetailed, false}, + {"", false}, + {"Compact", false}, + {"tiny", false}, + } + for _, tc := range cases { + if got := tc.view.IsCompact(); got != tc.want { + t.Errorf("JobListView(%q).IsCompact() = %v, want %v", tc.view, got, tc.want) + } + } +} diff --git a/src/runner/invocation_windows.go b/src/runner/invocation_windows.go index 9447509..d16c3d5 100644 --- a/src/runner/invocation_windows.go +++ b/src/runner/invocation_windows.go @@ -6,6 +6,7 @@ import ( "strings" "syscall" "unicode" + "unicode/utf8" ) func shellCommand(ctx context.Context, command string) *exec.Cmd { @@ -32,19 +33,45 @@ func quoteLeadingWindowsProgramPath(command string) string { } lower := strings.ToLower(trimmed) + pathEnd := -1 for _, extension := range []string{".exe", ".cmd", ".bat", ".com"} { - index := strings.Index(lower, extension) - if index < 0 { - continue + end := earliestBoundedExtensionEnd(lower, extension) + if end >= 0 && (pathEnd < 0 || end < pathEnd) { + pathEnd = end } - pathEnd := index + len(extension) - programPath := trimmed[:pathEnd] - if !strings.ContainsFunc(programPath, unicode.IsSpace) { - return command - } - return leadingWhitespace + `"` + programPath + `"` + trimmed[pathEnd:] } - return command + if pathEnd < 0 { + return command + } + programPath := trimmed[:pathEnd] + if !strings.ContainsFunc(programPath, unicode.IsSpace) { + return command + } + return leadingWhitespace + `"` + programPath + `"` + trimmed[pathEnd:] +} + +// earliestBoundedExtensionEnd returns the offset just past the first +// occurrence of extension in s that ends at a token boundary (end of string +// or whitespace), or -1 if none does. Scanning left to right and rejecting +// unbounded matches keeps a trailing "...\App.exe" inside an argument, such +// as "run.bat C:\tool.exe", from being mistaken for the program path. +func earliestBoundedExtensionEnd(s, extension string) int { + offset := 0 + for { + index := strings.Index(s[offset:], extension) + if index < 0 { + return -1 + } + end := offset + index + len(extension) + if end == len(s) { + return end + } + r, _ := utf8.DecodeRuneInString(s[end:]) + if unicode.IsSpace(r) { + return end + } + offset += index + 1 + } } func startsWithWindowsRootedPath(command string) bool { diff --git a/src/runner/logfile.go b/src/runner/logfile.go index 470ceec..7d10410 100644 --- a/src/runner/logfile.go +++ b/src/runner/logfile.go @@ -26,12 +26,50 @@ func writeRunLog(logsDir string, job domain.Job, trigger string, state string, d path := filepath.Join(logsDir, fileName) content := fmt.Sprintf("time: %s\njob_id: %d\njob_name: %s\ntrigger: %s\nstate: %s\ndetail: %s\nduration: %d\ncommand: %s\narguments: %s\nstart_only: %t\n\n%s\n", started.Format("2006-01-02 15:04:05"), job.ID, job.Name, trigger, state, detail, durationMS, job.Command, logArguments(job.Arguments), job.StartOnly, output) - if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + if err := writeFileAtomic(logsDir, path, []byte(content), 0o644); err != nil { return "", fmt.Errorf("write log file: %w", err) } return path, nil } +// writeFileAtomic writes data to a temp file in dir, then renames it over +// path. Rename is atomic within a volume on both supported platforms, so a +// crash or a killed process mid-write can never leave path holding a +// truncated log file the way a direct os.WriteFile could. +func writeFileAtomic(dir, path string, data []byte, perm os.FileMode) error { + tmp, err := os.CreateTemp(dir, filepath.Base(path)+".tmp*") + if err != nil { + return err + } + tmpPath := tmp.Name() + success := false + defer func() { + if !success { + os.Remove(tmpPath) + } + }() + + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return err + } + if err := tmp.Sync(); err != nil { + tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + if err := os.Chmod(tmpPath, perm); err != nil { + return err + } + if err := os.Rename(tmpPath, path); err != nil { + return err + } + success = true + return nil +} + func sanitizeFileName(name string) string { name = strings.TrimSpace(name) if name == "" { diff --git a/src/runner/runner_windows_test.go b/src/runner/runner_windows_test.go index 6ec730d..e3bcb32 100644 --- a/src/runner/runner_windows_test.go +++ b/src/runner/runner_windows_test.go @@ -51,3 +51,44 @@ func TestWindowsShellCommandLineQuotesUnquotedProgramPath(t *testing.T) { t.Fatalf("expected command line %q, got %q", want, got) } } + +// TestQuoteLeadingWindowsProgramPathPicksEarliestBoundedExtension pins the +// fix for the quoting bug found in the whole-project review: the program +// path must end at the *earliest* extension match that sits at a token +// boundary, not the first extension in the .exe/.cmd/.bat/.com list order, +// and not a substring match inside another word. +func TestQuoteLeadingWindowsProgramPathPicksEarliestBoundedExtension(t *testing.T) { + cases := []struct { + name string + command string + want string + }{ + { + name: "bat with unquoted argument", + command: `C:\My Tools\run.bat D:\in.txt`, + want: `"C:\My Tools\run.bat" D:\in.txt`, + }, + { + name: "bat program with exe argument", + command: `C:\My Tools\run.bat C:\Windows\System32\notepad.exe`, + want: `"C:\My Tools\run.bat" C:\Windows\System32\notepad.exe`, + }, + { + name: "cmd program with exe argument", + command: `C:\Program Files\App\deploy.cmd D:\stage\setup.exe`, + want: `"C:\Program Files\App\deploy.cmd" D:\stage\setup.exe`, + }, + { + name: "exe substring inside directory name", + command: `C:\dir.exexample\My Tool\run.bat`, + want: `"C:\dir.exexample\My Tool\run.bat"`, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := quoteLeadingWindowsProgramPath(tc.command); got != tc.want { + t.Fatalf("quoteLeadingWindowsProgramPath(%q) = %q, want %q", tc.command, got, tc.want) + } + }) + } +} diff --git a/src/storage/store.go b/src/storage/store.go index c73ce60..dcf54b3 100644 --- a/src/storage/store.go +++ b/src/storage/store.go @@ -230,7 +230,8 @@ func (s *Store) applyConfigPaths() { } func writeJSON(path string, value any) error { - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0o755); err != nil { return err } data, err := json.MarshalIndent(value, "", " ") @@ -240,10 +241,47 @@ func writeJSON(path string, value any) error { // A trailing newline keeps the file friendly to editors and diff tools that // expect text files to end with one. data = append(data, '\n') - // WriteFile replaces the full file instead of patching it in place. For small - // JSON files this is simpler and prevents stale keys from older versions from - // lingering after the schema changes. - return os.WriteFile(path, data, 0o644) + return writeFileAtomic(dir, path, data, 0o644) +} + +// writeFileAtomic writes data to a temp file in dir, syncs it, then renames it +// over path. Rename is atomic within a volume on both supported platforms, so +// a crash, a power loss, or the process being killed mid-write can never leave +// path holding a truncated or empty file the way a direct os.WriteFile could. +func writeFileAtomic(dir, path string, data []byte, perm os.FileMode) error { + tmp, err := os.CreateTemp(dir, filepath.Base(path)+".tmp*") + if err != nil { + return err + } + tmpPath := tmp.Name() + // Any failure past this point must remove the temp file rather than leave + // it behind for the next write to trip over. + success := false + defer func() { + if !success { + os.Remove(tmpPath) + } + }() + + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return err + } + if err := tmp.Sync(); err != nil { + tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + if err := os.Chmod(tmpPath, perm); err != nil { + return err + } + if err := os.Rename(tmpPath, path); err != nil { + return err + } + success = true + return nil } func defaultJobs() []domain.Job { diff --git a/src/storage/store_test.go b/src/storage/store_test.go index 4ad188b..07ddcae 100644 --- a/src/storage/store_test.go +++ b/src/storage/store_test.go @@ -430,3 +430,44 @@ func TestJobsJSONDoesNotPersistRuntimeNoise(t *testing.T) { } } } + +// TestWriteJSONReplacesFileAtomically pins the durability fix: writeJSON must +// never truncate the destination in place. It writes through a temp file and +// renames over the target, so a reader can never observe a partially written +// file, and an existing file survives untouched if the marshal fails first. +func TestWriteJSONReplacesFileAtomically(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "gosentry.json") + + original := domain.DefaultConfig() + original.LogsDir = "logs-original" + if err := writeJSON(path, original); err != nil { + t.Fatal(err) + } + + updated := domain.DefaultConfig() + updated.LogsDir = "logs-updated" + if err := writeJSON(path, updated); err != nil { + t.Fatal(err) + } + + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + var got domain.Config + if err := json.Unmarshal(data, &got); err != nil { + t.Fatal(err) + } + if got.LogsDir != "logs-updated" { + t.Fatalf("LogsDir = %q, want %q", got.LogsDir, "logs-updated") + } + + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + if len(entries) != 1 { + t.Fatalf("expected only the final file in %s, got %v", dir, entries) + } +} diff --git a/src/ui/run.go b/src/ui/run.go index 0d0885b..a57668d 100644 --- a/src/ui/run.go +++ b/src/ui/run.go @@ -85,6 +85,7 @@ func Run(startInTray bool) { // instead of forcing one timing definition onto two different UX flows. recordStartup(time.Since(started), false) a.Run() + svc.Stop() return } // Show the window before recording startup time. Measuring earlier, during @@ -94,6 +95,11 @@ func Run(startInTray bool) { w.Show() recordStartup(time.Since(started), true) a.Run() + // a.Run() blocks until the tray's Quit item or a window close calls a.Quit(). + // Stopping here — rather than not at all — cancels the run context so an + // in-flight run's os/exec call sees ctx.Done() instead of being orphaned, and + // stops the scheduler goroutine before the process exits. + svc.Stop() } // setWindowsNotificationIcon supplies App.Icon for Fyne desktop notifications