diff --git a/.claude/commands/review-project.md b/.claude/commands/review-project.md deleted file mode 100644 index e96a748..0000000 --- a/.claude/commands/review-project.md +++ /dev/null @@ -1,26 +0,0 @@ ---- -description: Review the project as a whole against the agenda in docs/REVIEW.md ---- - -Perform a whole-project review of GoSentry. - -Read [docs/REVIEW.md](../../docs/REVIEW.md) first — it is the agenda, and its -nine sections are the areas to cover. Read [docs/STANDARDS.md](../../docs/STANDARDS.md) -and [docs/ARCHITECTURE.md](../../docs/ARCHITECTURE.md) for the rules and -contracts the code is checked against. - -$ARGUMENTS narrows the review when given — a package path, a file, or the name -of an agenda section. With no arguments, sweep the whole `src/` tree. - -Rules for the report: - -- Anything listed under "Intentional behavior" in STANDARDS.md is not a finding. - If you believe such an entry is now wrong, say so explicitly as a challenge to - the decision rather than reporting it as a bug. -- Verify before reporting. Read the surrounding code and, where cheap, confirm - the behavior with a test rather than reasoning about it alone. -- Group findings by agenda section, most severe first, each with the file and - line and what would actually go wrong. -- Report honestly that a section is clean rather than inventing something for it. -- Do not fix anything during the review. Report first; apply fixes only when - asked, following "What happens to the findings" in REVIEW.md. diff --git a/CLAUDE.md b/CLAUDE.md index 18e8e99..31c4d17 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -13,12 +13,6 @@ application service, scheduler, storage, and command runner in one binary. - [docs/TESTS.md](docs/TESTS.md) — test layout and conventions. - [docs/ROADMAP.md](docs/ROADMAP.md) — deliberately out of scope. -## Reviewing the project - -When the user asks for a review of the project (rather than of a specific -diff), follow [docs/REVIEW.md](docs/REVIEW.md) — it is the agenda, and the -`/review-project` command runs the same thing. Do not improvise a checklist. - ## Key rules (full list in STANDARDS.md) - `src/app.Service` is the sole owner of job and runtime state; the UI reads it diff --git a/README.md b/README.md index b7b29af..a33095d 100644 --- a/README.md +++ b/README.md @@ -51,7 +51,6 @@ GoSentry is built and tested on **Windows** and **Linux**: - [Roadmap](docs/ROADMAP.md) — planned work larger than a single bug fix - [Architecture](docs/ARCHITECTURE.md) — component interaction model - [Standards](docs/STANDARDS.md) — quality rules and intentional behavior -- [Review](docs/REVIEW.md) — what a whole-project review looks at - [Development](docs/DEVELOPMENT.md) — build instructions, project layout, dependencies - [Tests](docs/TESTS.md) — test suite layout and how to run it - [Performance](docs/PERFORMANCE.md) — measured performance findings diff --git a/docs/PROJECT_REVIEW_PLAN.md b/docs/PROJECT_REVIEW_PLAN.md index 2cd309a..1be6c1e 100644 --- a/docs/PROJECT_REVIEW_PLAN.md +++ b/docs/PROJECT_REVIEW_PLAN.md @@ -1,9 +1,9 @@ # Whole-project review — action plan -Working document for the findings of the 2026-08-05 whole-project review, run -against the agenda in [REVIEW.md](REVIEW.md). It is not part of the permanent -doc set: delete it once every item below is either done or moved to -[ROADMAP.md](ROADMAP.md), the way `TEST_REVIEW_PLAN.md` was retired. +Working document for the findings of the 2026-08-05 whole-project review. It is +not part of the permanent doc set: delete it once every item below is either +done or moved to [ROADMAP.md](ROADMAP.md), the way `TEST_REVIEW_PLAN.md` was +retired. The rules the findings are judged against live in [STANDARDS.md](STANDARDS.md) and [ARCHITECTURE.md](ARCHITECTURE.md). Anything listed under "Intentional @@ -41,8 +41,8 @@ The findings below are therefore mostly about **the paths that only show up after the app has been running for a while** (§3.1, §6.3), **durability of the JSON files** (§6.2), and **one confirmed Windows quoting bug** (§6.1). -Severity is used as REVIEW.md uses it: *medium* means it gets a regression test -with the fix. +Severity follows the whole-project review convention: *medium* means it gets a +regression test with the fix. --- @@ -717,7 +717,7 @@ entries. These four are not. ## Suggested order Grouped so that each commit is independently reviewable and each medium finding -lands with its regression test, per REVIEW.md's "What happens to the findings". +lands with its regression test. 1. **4.1 — restore `TestJobListViewIsCompact`, retire the other row.** Smallest, and it restores an enforcement STANDARDS calls mandatory. Do it first so the diff --git a/docs/REVIEW.md b/docs/REVIEW.md deleted file mode 100644 index d5ad5cb..0000000 --- a/docs/REVIEW.md +++ /dev/null @@ -1,86 +0,0 @@ -# GoSentry — Review Agenda - -What to look at when reviewing the project as a whole, as opposed to a single -diff. This is the agenda; the rules a review checks against live in -[STANDARDS.md](STANDARDS.md) and [ARCHITECTURE.md](ARCHITECTURE.md). - -Scope note: a normal pull-request review checks the change. This agenda is for -a periodic sweep of the whole codebase, so a pass may legitimately end with -"nothing to report" on most items. - -## 1. Architecture and project structure - -Does the code still match the package map and the event flow in -[ARCHITECTURE.md](ARCHITECTURE.md)? Watch for the boundaries that matter here: -`app.Service` as the sole owner of job and runtime state, the UI reading it -through typed events, `domain` staying free of I/O, and platform-specific code -staying behind the `platform/*` interfaces. - -## 2. Complexity against the size of the project - -GoSentry is a single-process desktop app with two direct dependencies. Flag -abstraction that is not paying for itself: interfaces with one implementation -and no test seam, indirection added for a use case nobody has asked for, a new -dependency where thirty lines of standard library would do. Also check the -opposite direction — files that have grown past the size guideline in -ARCHITECTURE and should be split the way `jobs_view.go` was. - -## 3. Code quality - -The checkable rules are in [STANDARDS.md](STANDARDS.md) — error handling, unit -tests for pure helpers, regression tests for fixes, `fyne.Do` for updates off -the main thread. Beyond them: concurrency around `Service.mu`, goroutines whose -lifetime is not obvious, and error paths that report something less useful than -what they caught. - -## 4. Documentation and comments - -Does every documented behavior still exist, and does every non-obvious behavior -get documented? Check the doc set against the code: README (user-facing -behavior and config keys), ARCHITECTURE (packages and flows), STANDARDS -(rules and intentional behavior), DEVELOPMENT (build), TESTS, PERFORMANCE, -CHANGELOG (an entry per notable change). For comments, the bar is *why*, not -*what* — a comment restating the line below it is noise; an unexplained -workaround is a finding. - -## 5. Readability and maintainability - -Read a package as someone who has not seen it before. Can the next change be -made without reverse-engineering? Naming that matches the domain vocabulary, -functions that do one thing, and control flow that does not need a diagram. - -## 6. Logical errors - -Correctness independent of style: scheduling and timing edge cases (overlap -policy, sequential mode, pause interactions), off-by-one and boundary handling, -zero values that mean something (see the timeout rules in STANDARDS), state -that can be observed mid-update, and error paths that leave state inconsistent. - -## 7. Legacy code and migrations - -The app has no database, so migration means file compatibility: `gosentry.json` -and `jobs.json` written by an older version must keep working. Check that new -`Config` fields are backward compatible, that normalization happens in one -place, and that values which are meaningful zeros are not normalized away. Also -look for code kept alive only for a case that no longer exists. - -## 8. Undocumented or under-documented contentious decisions - -Any decision a future reader would question needs its reasoning recorded where -it lives: a comment at the code, an entry in the "Intentional behavior" section -of [STANDARDS.md](STANDARDS.md), or — when the work is deferred rather than -decided — a note in [ROADMAP.md](ROADMAP.md), which is where the frozen -window-size work keeps its rationale. - -## 9. Other improvement proposals - -Anything that does not fit above: build and release ergonomics, test coverage -gaps, dependency health, UX rough edges. - -## What happens to the findings - -- A defect → fix it, with a regression test when severity is medium or higher. -- Behavior that turns out to be deliberate → record it under "Intentional - behavior" in [STANDARDS.md](STANDARDS.md) so it is not re-reported. -- Work larger than a single fix → [ROADMAP.md](ROADMAP.md), with the reasoning. -- A new rule the review establishes → [STANDARDS.md](STANDARDS.md). diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 658f1ef..2b86d45 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -136,7 +136,7 @@ were already split once: | `src/storage/store.go` | 265 | This is deliberately deferred to the next whole-project review rather than done -piecemeal: [REVIEW.md](REVIEW.md) already asks item 2 to look for exactly this, +piecemeal: a future review already asks item 2 to look for exactly this, a split touches every reader of the file, and doing all six in one pass keeps the seams consistent instead of settling them six different ways. Splitting is also the kind of change that reads as pure movement while quietly dropping a diff --git a/docs/STANDARDS.md b/docs/STANDARDS.md index 4dc7bee..96c628b 100644 --- a/docs/STANDARDS.md +++ b/docs/STANDARDS.md @@ -1,8 +1,7 @@ # GoSentry — Standards Quality rules and intentional behavior for contributors. Package contracts live -in [ARCHITECTURE.md](ARCHITECTURE.md); test conventions in [TESTS.md](TESTS.md); -what a whole-project review looks at, in [REVIEW.md](REVIEW.md). +in [ARCHITECTURE.md](ARCHITECTURE.md); test conventions in [TESTS.md](TESTS.md). ## Code quality