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>
Phase 8 (PROJECT_REVIEW_PLAN.md 8.1): 0 in MaxLogFiles/MaxLogAgeDays now
means "keep everything" end to end. runner.CleanupLogs already treated
<= 0 as disabled; validateConfig, the Settings form, and
loadOrCreateConfig's backfill were the only things making that state
unreachable.
Phase 9 (1.1, rolling up 1.2, 1.3, 7.3): added Service.Config() and
Service.Paths(), copying under mu, and converted every UI site that read
Service state through the raw *storage.Store returned by Store() (now
removed). jobs_view's pause control is now driven by refreshView reading
svc.Config().Paused on every event instead of only mirroring its own tap
handler, which makes it an actual consumer of SchedulerStateChanged.
mainwindow's event listener is a real type switch, and events.go's doc
comment no longer claims a compiler exhaustiveness check Go doesn't have.
Unexported the redundant SetAutostart/AutostartStatus package functions
in platform/autostart now that only the Manager methods are used outside
the package.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Phase 7 of the whole-project review (findings 3.2 and 3.3).
Service.mu is the lock the Fyne main thread takes on every Jobs() and
Runtime() call, so anything blocking inside it makes a UI refresh wait on
the disk. Three things did:
- Every SaveJobs/SaveConfig was a marshal, fsync, and rename under mu.
Writes are now prepared under the lock (Store.PrepareSaveJobs /
PrepareSaveConfig snapshot the payload and target path) and run after
it is released. deferSaveLocked takes saveMu while mu is still held, so
writes still reach the file in the order their snapshots were taken and
an older snapshot can never land on top of a newer one.
- executeRun ran runner.CleanupLogs under mu after every run. It needs
only the values already snapshotted into runEnv, so it now runs after
the unlock — including when the job is gone, since the run still wrote
a log file that retention covers.
- adoptJobsLocked ran runner.SeedStats under mu, reached from
UpdateSettings on the UI thread. Seeding moved out into
applySeededStatsLocked; UpdateSettings now reads the new jobs file and
seeds its statistics before taking the lock, and re-checks the
"no jobs-file switch while running" guard once it has it.
SeedStats also opened every log file twice — once to find the job, again
to read the result. readLogSummary reads job_id, state, and duration in
one pass, so each log is opened once.
StartOnly runs were built with exec.CommandContext on the app's lifecycle
context. os/exec keeps a watcher goroutine alive until Wait returns or the
context is done, and StartOnly never calls Wait, so one goroutine leaked
per run and would then try to kill a process whose handle startJobOnly had
already released. The invocation now uses context.Background(), whose nil
Done channel means no watcher is started at all.
Regression tests: TestRunJobStartOnlyLeavesNoContextWatcher (fails with 5
leaked goroutines on the old code), TestConcurrentJobOperationsLeaveTheFileMatchingMemory,
and TestUpdateSettingsSeedsAdoptedJobsFromLogs. STANDARDS gains the
no-I/O-under-mu rule and the "a StartOnly process outlives GoSentry" entry.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
History was appended to on every recorded run and never trimmed, and every
event re-sorted the whole slice and re-measured the Job, Detail and Log
columns across every row. The per-run cost therefore grew with the number of
rows, in exactly the mode the app is designed for: left in the tray for days.
The session History now keeps the newest maxHistoryRows (1000) records, the
way maxJobLogs caps a job's own activity list, and drops the oldest from the
front, zeroing the tail so a dropped record's full captured output is not
kept alive by the backing array. Column widths move into a historyLog value
that folds each new record into the current maxima instead of rescanning.
Widths only grow within a theme, so a column never narrows when a record ages
out; a theme change is the one case that still rescans, because every stored
width was measured at the old text size.
Measured with a throwaway benchmark over 5000 accumulated records: one
refresh went from 15.8 ms to 0.9 ms. At the new cap the full width rescan
alone costs 1.5 ms, so both halves of the fix carry weight.
Plan item 6 of docs/PROJECT_REVIEW_PLAN.md (finding 3.1).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Implements items 4-5 of the whole-project review's suggested order
(docs/PROJECT_REVIEW_PLAN.md):
- Drop the three SaveJobs calls in the run lifecycle (startRunLocked,
executeRun, SetGlobalPause): none of them change a durable Job field,
everything they touch lives on JobRuntime, which is never persisted.
Retire TestStartRunLockedRollbackOnSaveFailure with the rollback it
guarded, since a run can no longer fail to start this way.
- Clear PendingRuns (the "queue" overlap policy's backlog) when a job is
disabled or the scheduler is globally paused, so resuming or
re-enabling a job no longer replays a deferred run left over from
before the pause/disable. Cap it at maxPendingRuns (10) so a job whose
runs take longer than its own interval stops accumulating an unbounded
backlog. Surface the queued count in the details pane via DisplayStats.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Autostart entries pass --start-in-tray only when the tray is enabled; Settings warns that the notification icon needs a restart (Fyne limitation).
Co-authored-by: Cursor <cursoragent@cursor.com>
Every item in TEST_REVIEW_PLAN.md is done or decided, so the working
document goes as its own header instructed. What outlives it moves to
where a later reviewer will actually look:
- TESTS.md gains the -coverpkg command and the 84.4% baseline (per-package
figures understate the suite), design principle 9 (redundancy is judged
by comparing coverage profiles, and identical coverage alone is not
grounds for deletion), a table of the look-alike tests that are kept
with the reason each survives, and the list of functions deliberately
at 0%.
- STANDARDS.md's "Intentional behavior" section points at both lists, so
the mechanism REVIEW.md describes still reaches them.
Dropped as spent: the per-item checklists, the suggested order, and the
model-selection table.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four documents asserted things the code contradicts.
ARCHITECTURE's component diagram had the UI calling the autostart Manager
directly. It does not, and must not: src/ui holds no reference to the
package at all — Settings reads svc.AutostartStatus(), like everything else
it reads. The edge is folded into the existing ui→Service one, so the
diagram no longer draws the exception to the project's own rule.
platform/desktop was described in both ARCHITECTURE and DEVELOPMENT as a
"display-scale helper". It installs the .desktop entry and icon under XDG
data home; there is no scale helper in it.
The ~250-line file guideline was written as though the jobs_view and
settings_view splits had settled it. Both files are over it again and
history_view.go has never been split, so the guideline is now stated as
the target it is, with the current state named rather than implied.
STANDARDS pointed at a "CI coverage gate" item that ROADMAP does not have,
while omitting the two it does.
README's gosentry.json sample was three keys short of what the app writes
on first run — default_timeout_seconds, theme and job_list_view — which
made the one file the user is invited to hand-edit the least accurate
thing in the document. The sample is now the real default (verified by
marshalling DefaultConfig), with the keys explained, including why a zero
timeout is written out and an unset one is not. The per-job overrides for
overlap policy and timeout were undocumented despite being in the job
dialog, and the feature list had not caught up with the timeout, the theme,
or the compact job list.
Version numbers in example output paths are now <version>, matching how the
CI section already wrote them, so they cannot go stale again.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Stage 9 of the GUI layout plan: the roadmap item the review was raised
under is closed, so the plan and the findings document go with it — what
they established now lives in STANDARDS and the CHANGELOG.
STANDARDS gains the rule the review produced: a size that must follow the
theme is measured at build time, not written as a pixel constant, because
a hand-tuned number is only correct for the theme it was tuned against.
rowOverlap, captionColumnWidth, textColumnWidth, activityRowsHeight and
initialSplitOffset are the worked examples.
The CHANGELOG entry keeps to what the user can see: the window opens at
the size it asks for and drags smaller, the Jobs divider is draggable,
History columns hold their content on a scaled UI, and the Settings
button row and block spacing are as their layouts intended.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Jobs directory row named a folder and assumed the file inside it was
called jobs.json. It is now a Jobs file row: Browse opens a file picker
filtered to .json, the field stays editable so a file that does not exist yet
can be typed, and the job list can live under any name.
Config.JobsDir/jobs_dir becomes Config.JobsFile/jobs_file, holding the whole
path; Paths.JobsDir is derived from it so saves still create the folder. An
older gosentry.json is migrated on load by joining its jobs_dir with
jobs.json — the exact file that version used — and the retired key is dropped
when the config is rewritten. The default clears before unmarshalling, or a
file that omits jobs_file and a file that sets it would be indistinguishable
and the migration would never run.
Saving used to write the current job list over whatever was at the new path,
which made switching to an existing jobs file impossible: its contents were
destroyed. An existing file now wins. Its jobs are loaded, normalized, and
adopted, with runtimes, schedule cache, next-run times and log-seeded
statistics rebuilt around them by adoptJobsLocked — the same helper NewService
now uses, so construction and adoption cannot drift. A path with no file
behind it still receives the current jobs, which is how the file is renamed or
relocated. The new file is read before anything is written, so an unparsable
one leaves both the config and the jobs untouched.
Adoption drops every runtime, and a run finishing afterwards would write its
result onto whichever job inherited its ID, so the switch is refused while a
job is running. Unrelated settings still save during a run. Because the
replacement happens without a prompt, the Service emits JobsLoaded with the
path and count, and History carries the receipt.
A path that names only a folder (trailing separator, a dot, or two dots) is
rejected with a validation error instead of failing later with an opaque OS
error.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reviewing the project used to mean re-stating what to look at every time.
docs/REVIEW.md now holds that agenda once — nine areas, each anchored to this
codebase — and both entry points point at it rather than copying it: the
/review-project command in .claude/commands, and a section in CLAUDE.md so a
plain-language review request lands in the same place.
STANDARDS.md gains a "Config file compatibility" section. The project has
applied the same rule three times (Theme, JobListView, TimeoutSeconds) without
ever writing it down: a new Config field is omitempty and its zero value means
the previous behavior, a meaningful zero is never backfilled on load, and an
unrecognised enum value reads as the default through one shared helper. With no
migration step and hand-editable files, that is what keeps older configs working.
Also removes docs/PLAN-compact-job-list.md, implemented in edabc57 — everything
but the version bump, which now waits for the release along with the rest of
the Unreleased section. .claude/settings.local.json is ignored so the shared
command can be tracked without per-developer permissions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A per-job timeout now has three distinct states: unset inherits the global
default, an explicit 0 means no timeout and does not inherit, and a positive
value is the per-job limit. Job.TimeoutSeconds became *int so unset and 0 stay
distinguishable in jobs.json.
Also fixes the global default, which could not persist a 0. loadOrCreateConfig
normalized DefaultTimeoutSeconds <= 0 back to 30 on every read of an existing
gosentry.json, so "no timeout" only held until the next restart. The field is
now written unconditionally (no omitempty) and read back as-is.
Existing jobs and configs are unaffected: a job with no timeout_seconds still
inherits, and a saved global default of 30 stays 30.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DefaultTimeoutSeconds now means "no timeout" when 0/empty, and that is
the new default, rather than an invalid config forcing a positive
value. runner.RunJob avoids context.WithTimeout with a zero duration
(which would expire immediately) and instead runs on a plain
cancelable context when no timeout is configured. Per-job
TimeoutSeconds inherit semantics are unchanged.
Add an optional per-job run timeout following the overlap_policy inherit
pattern: Job.TimeoutSeconds (0 = inherit) resolves against a new
Config.DefaultTimeoutSeconds (default 30s), replacing the hard-coded 30s
guard in runner.RunJob.
- domain/storage: new fields, default 30, load-time normalization
- runner: RunJob takes an explicit timeout; StartOnly stays untimed so it
keeps measuring launch latency only
- app: effectiveTimeout resolves under mu into runEnv, threaded to runJob;
seam signature and validation updated; DisplayTimeout helper
- ui: Timeout entry in the job dialog, Default timeout in Settings, and a
Timeout row in the details panel
- tests + docs (ARCHITECTURE, STANDARDS, ROADMAP, CHANGELOG) updated;
version bumped to 0.12.0
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mix
mixeme