docs: add FUTURE_WORK quality standard and close polish gaps
Replace CODE_REVIEW.md with a living maturity checklist, document session-only History, inject Service into newMainView for testability, add UI and scheduler regression tests, and fix RunNow error surfacing plus empty jobs view handling. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
mixeme
@@ -97,6 +97,9 @@ flowchart LR
|
||||
When a run goroutine completes, `Service` updates the job's runtime
|
||||
(including the statistics aggregate), saves JSON, triggers log cleanup, and
|
||||
emits `RunRecorded`. The UI observer appends the record to the History tab.
|
||||
History rows exist only for the current process session; restarting the app
|
||||
clears the table (aggregate stats in the details panel are still seeded from
|
||||
log files).
|
||||
|
||||
7. Autostart:
|
||||
`UpdateSettings` in the Service calls `autostart.Manager.Set`. The Manager
|
||||
@@ -150,6 +153,12 @@ accumulated run history immediately after a restart.
|
||||
Older log files that pre-date the `duration` header are tolerated: the run is
|
||||
counted but the timing is skipped.
|
||||
|
||||
`JobRuntime.Logs` (per-run `RunRecord` entries shown in the History tab) is
|
||||
**session-only**: it is not written to `jobs.json` and is not rebuilt from
|
||||
`.log` files on startup. Log files on disk feed aggregate counters via
|
||||
`SeedStats` only. See [FUTURE_WORK.md](FUTURE_WORK.md) for the intentional
|
||||
trade-off.
|
||||
|
||||
### Persisted global pause
|
||||
|
||||
`domain.Config` carries a `Paused bool` field (`json:"paused,omitempty"`).
|
||||
|
||||
@@ -1,53 +0,0 @@
|
||||
# GoSentry — Code Review (2026-06-29)
|
||||
|
||||
Версия на момент ревью: **0.11.2** (исправления вошли в **0.11.3**)
|
||||
|
||||
## Итог
|
||||
|
||||
| Критерий | Оценка |
|
||||
|----------|--------|
|
||||
| Архитектура | 9/10 |
|
||||
| Сложность vs масштаб | 8/10 |
|
||||
| Качество кода | 8/10 |
|
||||
| Поддерживаемость | 8/10 |
|
||||
| Логические ошибки | 9/10 (после исправлений) |
|
||||
|
||||
Проект зрелый и поддерживаемый для десктопного планировщика (~59 `.go`-файлов). Архитектура слоистая, core-логика хорошо протестирована.
|
||||
|
||||
## Сильные стороны
|
||||
|
||||
- Single-writer `app.Service` с явным locking contract
|
||||
- Разделение `domain.Job` (durable) и `domain.JobRuntime` (transient)
|
||||
- Event-driven UI без обратных вызовов в Fyne под lock
|
||||
- Portable storage от `os.Executable()`
|
||||
- Инъекция `runJob` и `scheduler.Clock` в тестах
|
||||
- Подробная документация (`ARCHITECTURE.md`, inline comments)
|
||||
|
||||
## Найденные проблемы и статус исправлений
|
||||
|
||||
| # | Проблема | Серьёзность | Статус |
|
||||
|---|----------|-------------|--------|
|
||||
| 1 | Data race: `store.Paths` в `executeRun` без lock | Высокая | Исправлено |
|
||||
| 2 | Run стартует при ошибке `SaveJobs` | Средняя | Исправлено |
|
||||
| 3 | CRUD эмитит events при failed save | Средняя | Исправлено |
|
||||
| 4 | Overlap queue — только один `Pending` | Средняя | Исправлено (`PendingRuns`) |
|
||||
| 5 | `time.Now()` vs scheduler clock в `startRunLocked` | Низкая | Исправлено |
|
||||
| 6 | Silent log write failures | Низкая | Исправлено |
|
||||
| 7 | Невалидный per-job `overlap_policy` | Низкая | Исправлено |
|
||||
| 8 | Docs drift (YAML, RunNow/pause) | Низкая | Исправлено |
|
||||
| 9 | `StartOnly` игнорировал cancel context | Низкая | Исправлено |
|
||||
| 10 | `SeedStats` коллизия sanitized имён | Низкая | Исправлено (match по `job_id`) |
|
||||
| 11 | `AvgDurationMS` seed vs live расходились | Низкая | Исправлено (`TimedRunCount`) |
|
||||
| 12 | Legacy ticket-ссылки в комментариях | Низкая | Исправлено |
|
||||
|
||||
## Намеренное поведение (не баги)
|
||||
|
||||
- `RunNow` разрешён при global pause и для disabled jobs
|
||||
- Sequential mode — FIFO по порядку в `jobs.json`
|
||||
- Scheduler tick 1s — sub-second `@every` не поддерживается
|
||||
- Command timeout 30s — глобальный лимит
|
||||
|
||||
## Рекомендации на будущее
|
||||
|
||||
- UI widget tests или smoke E2E
|
||||
- Per-job command timeout в конфиге
|
||||
@@ -0,0 +1,112 @@
|
||||
# GoSentry — Future Work
|
||||
|
||||
> Временный документ: цель проекта, стандарт качества и открытые задачи.
|
||||
> После завершения шлифовки переименуем (например, в `STANDARDS.md`).
|
||||
|
||||
## Зачем этот документ
|
||||
|
||||
**Образцовый проект** — тот, на который можно сослаться как на эталон: архитектура
|
||||
понятна с первого прочтения, границы пакетов соблюдаются, намеренные компромиссы
|
||||
задокументированы, поведение воспроизводимо тестами, новый контрибьютор знает
|
||||
*как* и *почему* писать код здесь.
|
||||
|
||||
Оценки зрелости ниже — **зеркало**, не KPI. Поднимать балл ради балла не имеет
|
||||
смысла; имеет смысл закрывать пункты чеклиста.
|
||||
|
||||
## Базовая оценка (внутреннее ревью, 2026-06-29)
|
||||
|
||||
| Критерий | Оценка |
|
||||
|----------|--------|
|
||||
| Архитектура | 9/10 |
|
||||
| Сложность vs масштаб | 8/10 |
|
||||
| Качество кода | 8/10 |
|
||||
| Поддерживаемость | 8/10 |
|
||||
| Логические ошибки | 9/10 |
|
||||
|
||||
Ревью проводилось на **0.11.2**; исправления вошли в **0.11.3–0.11.4**.
|
||||
Текущая версия: `src/app/version.go`.
|
||||
|
||||
## Архитектурные сильные стороны
|
||||
|
||||
- Single-writer `app.Service` с явным locking contract
|
||||
- Разделение `domain.Job` (durable) и `domain.JobRuntime` (transient)
|
||||
- Event-driven UI без обратных вызовов в Fyne под lock
|
||||
- Portable storage от `os.Executable()`
|
||||
- Инъекция `runJob` и `scheduler.Clock` в тестах
|
||||
- Подробная документация (`ARCHITECTURE.md`, inline comments)
|
||||
|
||||
## Стандарт качества (обязателен для нового кода)
|
||||
|
||||
- Контракты пакетов — [ARCHITECTURE.md](ARCHITECTURE.md)
|
||||
- User-facing error → `dialog.ShowError` или History event, не silent `return`
|
||||
- Pure helpers → unit-тест в том же пакете
|
||||
- Фикс severity ≥ medium → regression-тест
|
||||
- Намеренное поведение → §«Намеренное поведение» ниже, не backlog-баг
|
||||
- UI view-конструкторы принимают `*app.Service`, не вызывают `app.Open()` внутри
|
||||
|
||||
## Чеклист зрелости
|
||||
|
||||
| # | Критерий | Статус |
|
||||
|---|----------|--------|
|
||||
| 1 | Контракты пакетов задокументированы и соблюдаются | ✓ |
|
||||
| 2 | Намеренные trade-off'ы явно записаны | ✓ |
|
||||
| 3 | Нет silent failures в user-facing путях | ✓ |
|
||||
| 4 | Pure logic вынесена и покрыта unit-тестами | ✓ |
|
||||
| 5 | Regression-тест на серьёзные фиксы ревью | ✓ |
|
||||
| 6 | DI на границе UI↔Service | ✓ |
|
||||
| 7 | Документация = код | ✓ |
|
||||
| 8 | Platform-код тестируется по одному образцу | ✓ |
|
||||
| 9 | Единый стиль ошибок в UI | ✓ |
|
||||
|
||||
## Намеренное поведение (не баги)
|
||||
|
||||
- `RunNow` разрешён при global pause и для disabled jobs
|
||||
- Sequential mode — FIFO по порядку в `jobs.json`
|
||||
- Scheduler tick 1s — sub-second `@every` не поддерживается
|
||||
- Command timeout 30s — глобальный лимит
|
||||
- **History tab — session-only.** `JobRuntime.Logs` живёт только в памяти
|
||||
текущей сессии. Файлы в `logs_dir` используются для aggregate stats
|
||||
(`SeedStats`), не для таблицы History. Подробнее — [ARCHITECTURE.md](ARCHITECTURE.md).
|
||||
|
||||
## Закрытые находки ревью (2026-06-29)
|
||||
|
||||
Историческая справка; не открывать повторно без новых данных.
|
||||
|
||||
| # | Проблема | Серьёзность | Статус |
|
||||
|---|----------|-------------|--------|
|
||||
| 1 | Data race: `store.Paths` в `executeRun` без lock | Высокая | Исправлено |
|
||||
| 2 | Run стартует при ошибке `SaveJobs` | Средняя | Исправлено |
|
||||
| 3 | CRUD эмитит events при failed save | Средняя | Исправлено |
|
||||
| 4 | Overlap queue — только один `Pending` | Средняя | Исправлено (`PendingRuns`) |
|
||||
| 5 | `time.Now()` vs scheduler clock в `startRunLocked` | Низкая | Исправлено |
|
||||
| 6 | Silent log write failures | Низкая | Исправлено |
|
||||
| 7 | Невалидный per-job `overlap_policy` | Низкая | Исправлено |
|
||||
| 8 | Docs drift (YAML, RunNow/pause) | Низкая | Исправлено |
|
||||
| 9 | `StartOnly` игнорировал cancel context | Низкая | Исправлено |
|
||||
| 10 | `SeedStats` коллизия sanitized имён | Низкая | Исправлено (match по `job_id`) |
|
||||
| 11 | `AvgDurationMS` seed vs live расходились | Низкая | Исправлено (`TimedRunCount`) |
|
||||
| 12 | Legacy ticket-ссылки в комментариях | Низкая | Исправлено |
|
||||
|
||||
## Вне scope (осознанные trade-off'ы)
|
||||
|
||||
| Item | Где зафиксировано |
|
||||
|------|-------------------|
|
||||
| History из `.log` | Session-only by design (§выше) |
|
||||
| Per-job command timeout | [ROADMAP.md](ROADMAP.md) |
|
||||
| Window size persistence | [ROADMAP.md](ROADMAP.md) (frozen) |
|
||||
| Column filters в History | [ROADMAP.md](ROADMAP.md) |
|
||||
| CI coverage gate | [ROADMAP.md](ROADMAP.md) (будущее) |
|
||||
|
||||
## Честно про 10/10
|
||||
|
||||
Единые **10/10** по всем критериям — неразумная цель для Fyne desktop: CGO,
|
||||
platform stubs и headless-лимиты GUI объективно добавляют сложность.
|
||||
**Образцовость ≠ идеальный балл.** Целевое состояние: чеклист зрелости ✓,
|
||||
проект как reference implementation layered Go desktop app.
|
||||
|
||||
## Связанные документы
|
||||
|
||||
- [ARCHITECTURE.md](ARCHITECTURE.md) — контракты пакетов
|
||||
- [TESTS.md](TESTS.md) — как и что тестировать
|
||||
- [ROADMAP.md](ROADMAP.md) — крупные фичи и platform blockers
|
||||
- [DEVELOPMENT.md](DEVELOPMENT.md) — сборка и layout
|
||||
+78
-4
@@ -117,7 +117,7 @@ Tests all mutating operations on the Service, scheduler integration, and setting
|
||||
| `TestRunNowUsesRunnerAndRecords` | Verifies that `RunNow` invokes the runner, records a `RunRecord`, and emits `RunRecorded`. |
|
||||
| `TestRunNowNotFound` | Verifies that `RunNow` returns an error for an unknown job ID. |
|
||||
| `TestRunNowRefusedWhileAlreadyRunning` | Verifies that a second concurrent `RunNow` on the same job is rejected while the first is in progress. |
|
||||
| `TestRunNowRefusedWhilePaused` | Verifies that `RunNow` is rejected when the global pause flag is set. |
|
||||
| `TestRunNowAllowedWhilePaused` | Verifies that `RunNow` is allowed when the global pause flag is set (pause stops scheduled runs only). |
|
||||
| `TestRunDueStartsDueJob` | Verifies that `RunDue` launches a job whose next-run time has passed. |
|
||||
| `TestRunDueSkipsJobNotYetDue` | Verifies that `RunDue` does not launch a job that is not yet due. |
|
||||
| `TestRunDueSkipsJobInRunningState` | Verifies that `RunDue` does not start a second concurrent run for an already-running job. |
|
||||
@@ -134,6 +134,30 @@ Tests all mutating operations on the Service, scheduler integration, and setting
|
||||
|
||||
---
|
||||
|
||||
### src/app/run_test.go
|
||||
|
||||
**Package:** `app`
|
||||
|
||||
Tests overlap policy, sequential execution, run statistics, and scheduler edge cases using injected `runJob` and `primeDue`.
|
||||
|
||||
| Test | Purpose |
|
||||
|------|---------|
|
||||
| `TestUpdateStats` | Verifies aggregate duration math on `JobRuntime`. |
|
||||
| `TestUpdateStatsSkipsZeroDuration` | Verifies zero-duration runs are excluded from averages. |
|
||||
| `TestRunDueParallelStartsAllDueJobs` | Parallel mode: both due jobs enter the runner before either completes. |
|
||||
| `TestRunDueSequentialSerializes` | Sequential mode: job 2 waits until job 1 finishes. |
|
||||
| `TestRunDueSkipDropsOverlap` | Global skip: no second concurrent run, `PendingRuns` stays 0. |
|
||||
| `TestRunDueQueueRerunsAfterFinish` | Queue: one deferred run after an in-flight finish. |
|
||||
| `TestRunDueQueueDrainsMultipleOverlaps` | Queue: multiple missed ticks drain as separate runs. |
|
||||
| `TestRunDuePerJobQueueOverridesGlobalSkip` | Per-job `queue` beats global `skip`. |
|
||||
| `TestRunDuePerJobSkipOverridesGlobalQueue` | Per-job `skip` beats global `queue`. |
|
||||
| `TestRunDueEmptyOverlapInheritsGlobal` | Empty per-job policy inherits the global default. |
|
||||
| `TestRunNowSequentialGuard` | Manual run refused while another job runs in sequential mode. |
|
||||
| `TestStartRunLockedRollbackOnSaveFailure` | Regression: run does not start when `SaveJobs` fails. |
|
||||
| `TestRunDueQueueDrainSkippedWhenPaused` | Queued overlaps are not drained while the scheduler is paused. |
|
||||
|
||||
---
|
||||
|
||||
### src/app/events_test.go
|
||||
|
||||
**Package:** `app`
|
||||
@@ -162,7 +186,9 @@ Tests display-formatting helpers used by the UI.
|
||||
| `TestDisplayArguments` | Verifies that an empty arguments string shows "None". |
|
||||
| `TestDisplayRunMode` | Verifies run-mode labels for normal and start-only modes. |
|
||||
| `TestDisplayInvocation` | Verifies that the full invocation display string combines command and arguments with spacing. |
|
||||
| `TestDisplayIndex` | Verifies that the display index is one-based (job slice index + 1). |
|
||||
| `TestDisplayIndex` | Verifies the list position of a job index in a filtered index slice. |
|
||||
| `TestDisplayStats` | Verifies statistics line formatting for the details panel. |
|
||||
| `TestDisplayOverlapPolicy` | Verifies per-job vs inherited global overlap policy labels. |
|
||||
|
||||
---
|
||||
|
||||
@@ -302,6 +328,20 @@ Tests Linux autostart via XDG Desktop Entry files.
|
||||
|
||||
---
|
||||
|
||||
### src/platform/desktop/desktop_linux_test.go
|
||||
|
||||
**Location:** `src/platform/desktop/desktop_linux_test.go`
|
||||
**Build Tags:** `//go:build linux`
|
||||
|
||||
Tests Linux desktop integration (`.desktop` file and icon under XDG data home).
|
||||
|
||||
| Test | Purpose |
|
||||
|------|---------|
|
||||
| `TestInstallDesktopIntegrationWritesDesktopAndIcon` | Verifies `.desktop` and PNG icon files are written under `$XDG_DATA_HOME`. |
|
||||
| `TestQuoteDesktopExecQuotesPath` | Verifies `Exec=` paths with spaces are shell-quoted. |
|
||||
|
||||
---
|
||||
|
||||
### src/ui/jobs_view_test.go
|
||||
|
||||
**Package:** `ui`
|
||||
@@ -317,6 +357,37 @@ Tests pure helper functions in the jobs view (no Fyne widget construction).
|
||||
| `TestFilteredJobIndexesByNamedFolder` | Verifies that filtering by a named folder returns only jobs in that folder. |
|
||||
| `TestFilteredJobIndexesNoFolder` | Verifies that the "No folder" filter returns only jobs with an empty folder field. |
|
||||
| `TestFilteredJobIndexesEmptySlice` | Verifies that filtering an empty job slice returns an empty index list. |
|
||||
| `TestLastJobLogsCapsAndCopies` | Verifies activity panel cap and defensive copy semantics. |
|
||||
| `TestLastJobLogsEmpty` | Verifies nil/empty log input returns an empty slice. |
|
||||
| `TestIndexOfID` | Verifies job lookup by ID returns `-1` when not found. |
|
||||
|
||||
---
|
||||
|
||||
### src/ui/history_view_test.go
|
||||
|
||||
**Package:** `ui`
|
||||
|
||||
Tests pure History tab helpers (no Fyne widget construction).
|
||||
|
||||
| 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. |
|
||||
|
||||
---
|
||||
|
||||
### src/ui/mainwindow_test.go
|
||||
|
||||
**Package:** `ui`
|
||||
|
||||
Smoke test for main view construction with an injected `*app.Service`.
|
||||
|
||||
| Test | Purpose |
|
||||
|------|---------|
|
||||
| `TestMainViewBuilds` | Verifies `newMainView` assembles tabs without panic using `fyne.io/fyne/v2/test`. |
|
||||
|
||||
---
|
||||
|
||||
@@ -334,9 +405,12 @@ Tests pure helper functions in the jobs view (no Fyne widget construction).
|
||||
|
||||
6. **Start-Only Mode** — Special handling for long-running processes that should be launched but not waited on, tested separately from normal execution flow.
|
||||
|
||||
7. **Regression on serious fixes** — Any fix from an internal review with severity ≥ medium gets a targeted regression test (see `run_test.go` for examples).
|
||||
|
||||
---
|
||||
|
||||
## Remaining Test Coverage Gaps
|
||||
|
||||
- GUI integration tests — Fyne widget interaction is not yet tested end-to-end
|
||||
- Job history on-disk retrieval — RunRecord file reading is not covered
|
||||
- Full GUI E2E — tab navigation, dialog flows, and native file pickers are not exercised end-to-end
|
||||
- History is session-only by design — `.log` files seed aggregate stats only, not the History table (see [FUTURE_WORK.md](FUTURE_WORK.md))
|
||||
- `layout.go` custom layouts — optional Fyne `test.NewApp()` coverage when CGO is available in CI
|
||||
|
||||
Reference in New Issue
Block a user