chore: land the remaining low-severity items from the whole-project review
Phase 11 of PROJECT_REVIEW_PLAN.md: the themed cleanup pass over every low-severity finding still open (2.2-2.3, 3.4-3.6, 4.3-4.7, 6.4-6.7, 7.1-7.3, 8.2-8.3, 9.1-9.4, and the under-documented decisions in §10/§11). Behavioral fixes: - Reassign duplicate job IDs in a hand-edited jobs.json instead of letting two jobs share one runtime, schedule entry, and SeedStats bucket. - Disambiguate run-log file names that collide within the same second. - Compute AvgDurationMS as DurationSumMS/TimedRunCount instead of an incremental integer mean, so it always matches the seeded-from-logs average instead of drifting from truncation error. - Clean absolute paths in ResolveConfiguredPath so two spellings of the same jobs file do not trigger a spurious adoption. - Report InstallDesktopIcon failures through ErrorOccurred instead of discarding them silently. - Move settingsView's blocking AutostartStatus (PowerShell on Windows) off the UI thread. - Give notify-timing.tsv its own extension so CleanupLogs no longer manages it as a run log. - Replace the settingsView Save handler's second copy of validateConfig's rules with a bare parse, letting the Service's own error surface. Cleanups: - Delete collectActivity, the dead yaml tags on RunRecord, and the logArguments/LogArguments alias. - Fold the two systemTrayRegistered/mainWindowHidden globals into one trayState instance Run owns and threads through Settings and the single-instance reveal path. - Fix stale comments/docs: the frozen window-size restore claim, a reference to a renamed recordRun, README's "Pause all" and notification wording, the PowerShell quoting note for TESTS.md's coverage command, and scripts/test.bat's UTF-8 checkmarks under a non-UTF-8 code page. - Document the single-instance fallback's consequence and the unauthenticated instance-channel port in STANDARDS.md; record the config-shim retirement plan in ROADMAP.md. 3.5, 7.3, and 9.4 turned out to already be fixed by earlier phases; no change needed for those three. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -237,8 +237,9 @@ applies to them — and so measure launch latency only.
|
||||
| `RunCount` | total runs recorded |
|
||||
| `FailCount` | runs that exited non-zero |
|
||||
| `LastDurationMS` | wall-clock time of the most recent run (launch latency for `StartOnly`) |
|
||||
| `AvgDurationMS` | mean over all runs with a recorded duration |
|
||||
| `AvgDurationMS` | mean over all runs with a recorded duration, computed as `DurationSumMS / TimedRunCount` on every update rather than folded incrementally, so it never disagrees with the exact sum/count average `runner.aggregateLogStats` computes when seeding from logs |
|
||||
| `MaxDurationMS` | longest recorded run |
|
||||
| `DurationSumMS` | running total of every timed run's duration; the source `AvgDurationMS` is divided from |
|
||||
|
||||
`runner.RunJob` measures the wall-clock start→finish and sets `DurationMS` on
|
||||
the returned `RunRecord`. `runner/logfile.go` writes a `duration` line into the
|
||||
|
||||
+22
-2
@@ -18,15 +18,35 @@ machine):** average **773 ms** per toast (695–874 ms), dominated by PowerShell
|
||||
cold start. Re-run the script when comparing after a native toast implementation.
|
||||
|
||||
**App-side timing:** each failure notification appends one line to
|
||||
`logs/notify-timing.log` (`ms_after_run`, `ms_fyne_do`, `ms_send`,
|
||||
`logs/notify-timing.tsv` (`ms_after_run`, `ms_fyne_do`, `ms_send`,
|
||||
`ms_app_total`). These columns end when Fyne returns from `SendNotification`; OS
|
||||
toast latency is not included.
|
||||
toast latency is not included. The `.tsv` extension keeps it out of
|
||||
`runner.CleanupLogs`, which only manages `.log` files — this file is
|
||||
diagnostic instrumentation for this item, not job output, and should be
|
||||
removed (or unified with the run-log retention policy under its own knob) once
|
||||
the native-toast direction below lands and the timing data is no longer
|
||||
needed.
|
||||
|
||||
**Direction:** add `src/platform/notify/` with a native Windows toast (WinRT or
|
||||
a maintained Go wrapper), used for failure notifications on Windows. Keep Fyne
|
||||
`SendNotification` on Linux (DBus / xdg-desktop-portal) unless profiling shows it
|
||||
needs the same treatment.
|
||||
|
||||
### Retire the config compatibility shims
|
||||
|
||||
Two read-only shims in `storage.loadOrCreateConfig` rewrite an old file into
|
||||
the current shape on the next save, so each becomes dead the moment a user's
|
||||
config has been saved once by a build that has it:
|
||||
|
||||
- `Config.JobsDir` (pre-0.15, superseded by `Config.JobsFile`).
|
||||
- `Theme == "default"` (pre-1.0.1, superseded by `ThemeSystem`).
|
||||
|
||||
Neither has an expiry. Remove both — the field, the migration branch, and
|
||||
`TestLoadOrCreateConfigMigratesJobsDir` /
|
||||
`TestLoadOrCreateConfigMigratesLegacyThemeDefault` — once a release has shipped
|
||||
long enough that a config file still carrying either old shape is not a
|
||||
realistic upgrade path GoSentry needs to support.
|
||||
|
||||
### Dynamic tray icon toggle
|
||||
|
||||
Fyne exposes `SetSystemTrayIcon` and related APIs only at application startup.
|
||||
|
||||
@@ -121,6 +121,22 @@ change to their shape has to stay compatible on its own.
|
||||
an occurrence that fired before the pause/disable. The details pane appends
|
||||
", N queued" to the statistics line via `DisplayStats` whenever the count is
|
||||
non-zero.
|
||||
- **Single-instance arbitration falls back to "start anyway" when the port is
|
||||
held by something else.** `acquireSingleInstance` (`singleinstance.go`)
|
||||
binds `127.0.0.1:37653`; if that fails and a dial to the same address does
|
||||
not answer as GoSentry either, startup continues rather than refusing to
|
||||
open because of an unrelated local listener. The consequence is deliberate
|
||||
but worth spelling out: two GoSentry processes can then run two schedulers
|
||||
against the same `jobs.json` and the same logs directory, each overwriting
|
||||
the other's saves. Atomic writes (`writeFileAtomic`) prevent a *torn* file
|
||||
from a concurrent write, but not one process's save clobbering the other's.
|
||||
- **The single-instance channel is an unauthenticated localhost TCP port.**
|
||||
Port 37653 accepts one command, `"show"`, from any local process — including
|
||||
one running as a different user on a shared machine. This is a deliberate
|
||||
scope choice, not an oversight: the command only raises the existing window,
|
||||
so the impact of an unwelcome sender is a window popping up, not data
|
||||
exposure or control. Anything with a larger blast radius on that channel
|
||||
would need real authentication.
|
||||
|
||||
## Out of scope
|
||||
|
||||
|
||||
+31
-3
@@ -26,6 +26,12 @@ The GUI tests build the Fyne desktop backend, so CGO must be enabled; on Windows
|
||||
that means the MSYS2 UCRT64 toolchain described in
|
||||
[DEVELOPMENT.md](DEVELOPMENT.md).
|
||||
|
||||
`src/ui` dominates `go test -race ./...`'s wall time — around 229s in the
|
||||
2026-08-05 whole-project review, against under 8s for every other package
|
||||
combined. Budget iteration accordingly: a change confined to `domain`,
|
||||
`storage`, `runner`, `scheduler`, or `app` gets a fast feedback loop; a `ui`
|
||||
change does not.
|
||||
|
||||
### Manual test commands
|
||||
|
||||
Run all tests:
|
||||
@@ -63,6 +69,15 @@ covered by the `app` tests. Measure the engine packages together instead:
|
||||
go test -coverpkg=./src/domain,./src/storage,./src/runner,./src/scheduler,./src/app ./src/domain ./src/storage ./src/runner ./src/scheduler ./src/app
|
||||
```
|
||||
|
||||
In the PowerShell environment DEVELOPMENT.md prescribes on Windows, PowerShell
|
||||
splits the comma-separated `-coverpkg` list on its own and the command fails
|
||||
with `directory not found`. Use the stop-parsing token, or quote the whole
|
||||
flag:
|
||||
|
||||
```powershell
|
||||
go test --% -coverpkg=./src/domain,./src/storage,./src/runner,./src/scheduler,./src/app ./src/domain ./src/storage ./src/runner ./src/scheduler ./src/app
|
||||
```
|
||||
|
||||
That figure was 84.4% at the 2026-08-04 review, which is the number to compare
|
||||
against before concluding that coverage has slipped.
|
||||
|
||||
@@ -245,6 +260,8 @@ Tests JSON round-tripping, default generation, and backward compatibility.
|
||||
| `TestJobsRoundTrip` | Verifies that jobs saved to JSON are reloaded with identical field values. |
|
||||
| `TestConfigRoundTrip` | Verifies that settings saved to JSON are reloaded with identical field values. |
|
||||
| `TestNormalizeJobsFillsDefaults` | Verifies that `normalizeJobs` assigns sequential IDs and sets default name, schedule, and command for jobs missing those fields. |
|
||||
| `TestNormalizeJobsReassignsDuplicateIDs` | Verifies that a hand-edited `jobs.json` with two entries sharing one ID gets the later duplicates reassigned instead of colliding on one runtime. |
|
||||
| `TestResolveConfiguredPathCleansAbsolutePaths` | Verifies (Windows only) that forward-slash and backslash spellings of the same absolute path resolve to the same string. |
|
||||
| `TestLoadOrCreateConfigCreatesDefaultsOnFirstRun` | Verifies that a missing config file is created with sane defaults. |
|
||||
| `TestLoadOrCreateJobsSeedsSampleJobsOnFirstRun` | Verifies that a missing jobs file is created with the sample jobs from `defaultJobs`. |
|
||||
| `TestLoadOrCreateConfigKeepsZeroTimeoutOnReload` | Verifies that `default_timeout_seconds: 0` survives a reload rather than being normalized away — 0 is a value, not a missing field. |
|
||||
@@ -376,6 +393,19 @@ Tests log-file cleanup by age and by count.
|
||||
|
||||
---
|
||||
|
||||
### src/runner/logfile_test.go
|
||||
|
||||
**Package:** `runner`
|
||||
|
||||
Tests the disambiguating suffix `writeRunLog` applies when two runs land on
|
||||
the same second.
|
||||
|
||||
| Test | Purpose |
|
||||
|------|---------|
|
||||
| `TestUniqueLogPathAvoidsCollision` | Verifies repeated calls for the same file name return distinct paths instead of silently overwriting an existing log. |
|
||||
|
||||
---
|
||||
|
||||
### src/platform/autostart/autostart_windows_test.go
|
||||
|
||||
**Location:** `src/platform/autostart/autostart_windows_test.go`
|
||||
@@ -508,8 +538,6 @@ column-width behaviour of the assembled table.
|
||||
|
||||
| Test | Purpose |
|
||||
|------|---------|
|
||||
| `TestCollectActivityMergesAndSorts` | Verifies per-job logs are merged and sorted by time. |
|
||||
| `TestCollectActivitySkipsMissingRuntimes` | Verifies missing runtime entries are skipped safely. |
|
||||
| `TestHistoryCellText` | Verifies table cell text for all columns; empty trigger → `Unknown`. |
|
||||
| `TestLogFileName` | Verifies log path basename extraction on Windows and Unix paths. |
|
||||
| `TestNewEventUsesConsistentTimestampShape` | Verifies UI events use the same timestamp layout as run records. |
|
||||
@@ -593,7 +621,7 @@ 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. |
|
||||
| `TestAppendNotificationTimingLogWritesHeaderAndRow` | Verifies `appendNotificationTimingLog` creates `notify-timing.tsv` with its header on first write and appends a row containing the job name. |
|
||||
|
||||
---
|
||||
|
||||
|
||||
Reference in New Issue
Block a user