scoped-review

name	scoped-review
description	Scoped code review — determines the change scope (explicit, discussion, or git), picks a source of truth (discussion, new docs, or code), then reviews the changes for correctness, doc agreement, doc coverage, convention adherence, duplicated shapes that want a shared abstraction, hand-rolled complex logic that wants a named/shared function, and unit-test coverage of complex logic.
disable-model-invocation	true
context	fork
agent	general-purpose

Scoped Code Review

A focused review of a specific set of changes — not the whole codebase. This skill reviews a bounded diff against a chosen source of truth.

Precision over recall. A short report of real problems beats a long one padded with speculation. Every finding the user dismisses as "not actually a problem" is a tax on the review. If you are not confident a finding is a real problem right now, drop it. Be skeptical of the code — and equally skeptical of your own findings before they go in.

Step 1: Determine scope

What changes are under review. Resolve in this order; stop at the first that applies:

1. Explicit ask. If the user named a scope (a path, a commit range, a PR, "the wear sync changes", "since the last release"), that is the scope. Use it verbatim.
2. Discussion context. If this conversation has been working on a specific change, that work is the scope — even if not restated as a review target.
3. Git, in this order:
   • On a feature branch (not main): scope is the branch — git diff main...HEAD plus any uncommitted changes.
   • Otherwise, if there are uncommitted changes (git status --porcelain non-empty): scope is the working tree — git diff HEAD (staged + unstaged), including untracked files.
   • Otherwise: scope is the last commit — git show HEAD / git diff HEAD~1..HEAD.

State the resolved scope in one line at the top of the report, with the exact diff command, so the user can confirm you reviewed what they meant. If scope is genuinely ambiguous (e.g. branch and a big pile of unrelated uncommitted work), ask before reviewing rather than guess.

Step 2: Determine the source of truth

What the changes are measured against. The review's job is to check the change agrees with its source of truth — so picking the right one matters. Resolve in this order:

1. Discussion context. If the conversation laid out what the change should do (a spec, an agreed design, acceptance criteria), that is the source of truth. Code and docs are judged against it.
2. New / changed documentation that clearly defines a feature. If the diff includes documentation that specifies new behaviour — a new docs/ plan, a diary/ design note, a CLAUDE.md section describing intent — treat that doc as the spec. The code must implement what the doc promises.
3. Code, with docs judged against it. If the diff is mostly code with only scattered, incidental doc edits (not a coherent spec), the code is the source of truth and the documentation is reviewed against the code — stale, contradictory, or missing doc updates are findings, not the spec.

State the chosen source of truth in one line in the report. When it's docs or discussion, the bar is "does the code do what was promised". When it's code, the bar is "do the docs (and conventions, and tests) keep up with what the code now does".

Load the context needed to judge against the source of truth:

• CLAUDE.md — commands, module layout, architecture, source layout quirk, platform priorities, privacy principles.
• README.md — outward feature summary, version compatibility notes.
• diary/ — setup/design notes for the touched area (e.g. Database.md, State and navigation.md), when relevant.
• Whatever docs/spec the source-of-truth decision points at.

Step 3: Review the changes

Read the full diff for the scope. For non-trivial changes, also open the surrounding code at each change site — a diff read in isolation hides bugs that are obvious in context. Check each of the following.

3a. Correctness — the changes work, no bugs introduced

• Logic does what it intends; edge cases handled (empty drink list, zero alcohol content, zero body weight / unset profile, negative or overlapping time ranges, error paths, off-by-one).
• BAC / unit math in bac/ and drinks/ stays dimensionally sound — units (grams of alcohol, per-mille, litres, drink units) convert through the existing helpers, not ad-hoc constants; rounding/formatting via existing util/ number helpers.
• Time handling uses kotlinx-datetime types across boundaries; be suspicious of timezone/DST assumptions and of mixing platform time APIs into common code. The "day" boundary for drink tracking follows DrinkTimeService, not raw midnight.
• expect/actual pairs complete. A new or changed expect in commonMain has matching actuals in androidMain and iosMain (iOS may be a no-op/reduced implementation, but it must exist and compile). Koin platformModule() actuals both updated when DI changes.
• SQLDelight: schema changes come with a matching .sqm migration (numbered next in migrations/), and ./gradlew verifySqlDelightMigration passes. SQL stays compatible with SQLite 3.38 (iOS 18 floor) — no features newer than the pinned dialect. Foreign-key implications considered (enforcement is on for every connection via the platform driver configs).
• Privacy: no analytics, tracking, ads, or network calls introduced. Only allowed exceptions: Google Account device backup, user-initiated manual export/import, and Wear OS data-layer sync.
• No regressions in adjacent behaviour the change touches.

3b. Agreement with the source of truth

• Code matches what the spec / docs / discussion promised. Flag every divergence — both "code does less than promised" and "code does something the spec didn't ask for" (scope creep is a finding too).

3c. All relevant docs updated

For the behaviour the change introduces or alters, check that every doc that should reflect it does. A user-visible or structural change with no matching doc update is a finding. Relevant docs, when applicable:

• README.md — outward feature summary, screenshots, version notes.
• CLAUDE.md — commands, module layout, architecture, conventions (when the change alters any of these).
• diary/ — when the change resolves or invalidates a documented workaround (especially Troubleshooting.md) or changes a documented setup step.
• docs/ — when the change implements (or diverges from) a documented plan, the plan's status should reflect it.
• gradle/libs.versions.toml — app-version-code / app-version-name bumped when the change is part of a release.

Judge appropriateness — not every change touches every doc. A pure internal refactor may need no doc changes at all; a schema migration or user-visible feature usually needs some. Flag the docs that genuinely should have changed and didn't.

3d. Conventions adhered to

Against CLAUDE.md and codebase norms:

• Source layout quirk: new files under composeApp/src/*/kotlin/ omit the fi/tuska/beerclock directory prefix but keep the full package declaration (package fi.tuska.beerclock.bac in commonMain/kotlin/bac/).
• Localization: every new user-visible string is added to the Strings interface and both EnStrings and FiStrings. No hard-coded UI text in composables.
• DI: services registered in commonModule() (or the platform modules) and resolved via KoinComponent — matching how neighbouring services do it.
• Screens: Voyager pattern — *Screen + *ViewModel pair under screens/<feature>/, using the custom ViewModel base with rememberWithDispose, not androidx ViewModels.
• Parcelize: CommonParcelize (and friends in util/Serialization.kt), never @Parcelize directly in common code.
• Platform priorities: full Android support expected; iOS actual may be reduced/no-op but basic functionality keeps working; Wear-only features stay in wearOs / Android actuals.
• General: naming, module placement, and idiom match the surrounding code; platform code goes through the existing expect/actual homes (util/, logging/, backup/, wear/) rather than new Platform.* checks in common code.

3e. Duplicated shapes — refactor signal (important)

Check whether the new functionality introduces code that duplicates a shape already present elsewhere in the repo. This is the key signal that a common shape should be lifted into a generic implementation.

• Search the repo for the same logic / structure the change adds — exact copies and near-copies that drifted slightly (the dangerous kind: they diverge into bugs).
• If the change copy-pastes a SQLDelight query shape, a screen/ViewModel scaffold, a dialog composable, a unit-conversion routine, or a helper that already exists, list every site and propose the unifying abstraction (shared util in util/, shared composable in ui/composables/, shared service in the owning feature package).
• Watch specifically for logic duplicated between composeApp and wearOs — the Wear app is a separate module and can't see common code, so drift there is easy; flag it and note what can be shared.
• Weigh against cognitive load: three short copies a reader holds in their head can beat one abstraction chased across four files. Make that call explicitly — but a genuine duplicated shape with real divergence risk is a finding, not a style nit.
• Check for existing rationale comments explaining why sites are kept separate; if the rationale still holds, drop the finding silently.

3f. Hand-rolled complex logic — name it / share it (important)

Check that complex calculation, parsing, unit conversion, date arithmetic, or similar fiddly logic is not inlined raw.

• Even at a single use site, this kind of logic deserves a named function at minimum — both for readability and so it can be unit-tested in isolation.
• If the functionality is generic (an alcohol-amount conversion, a BAC interpolation, a graph-data bucketing routine, a day-boundary computation) and there are other sites in the repo that do the same thing — or the logic is general enough to belong there — it should be promoted to util/ or the owning domain package (bac/, drinks/), not left inline in a composable or ViewModel.
• Flag inline blocks of multi-step arithmetic, manual string parsing, or date math with no name and no test — especially anything inside a @Composable body.

3g. Test coverage of complex logic

• Every complex calculation / piece of fiddly logic the change adds must be covered by unit tests, so regressions are caught. Unit-testable logic belongs in commonMain (pure Kotlin, no platform deps) where composeApp/src/commonTest reaches it via ./gradlew check.
• Check the diff (or adjacent test files) for tests exercising the new logic's edge cases — not just a happy-path smoke test. BAC/unit math needs zero/rounding/boundary cases; time logic needs day-boundary and timezone cases.
• Schema changes need the migration verified (./gradlew verifySqlDelightMigration is part of check).
• Untested complex logic is a finding even if the code looks correct; "looks correct" is exactly what regresses silently.

Step 4: Self-validate before reporting

Run every candidate finding through this filter; drop anything that fails. Do not pad the report.

• Read the surrounding context at the cited lines. Many "issues" evaporate once the local context is clear.
• Look for explanatory comments / commit messages / diary notes. If the code is shaped this way on purpose (intentional duplication, a documented Compose Multiplatform workaround, a deliberate iOS reduction), assess whether the rationale still holds. If it does, drop the finding. If not, the finding must say why it's stale.
• Confirm the problem exists today. "Could be a problem if X" is not a finding unless X is real and you confirmed it.
• Check the fix is actually better. If a proposed refactor trades one complexity for an equal one, drop it.
• No style-only nits dressed as findings. Renames / reorderings / "I'd write it differently" are not findings.

Step 5: Report

Open with two lines: the resolved scope (with the exact diff command) and the chosen source of truth. Then lead with what's most worth fixing, grouped:

• High — bugs, correctness issues, privacy violations (any new network communication, analytics, tracking), broken or missing expect/actual pairs, missing/incorrect SQLDelight migrations, SQLite-compat breakage, code that disagrees with the source of truth, missing localization for user-visible text, untested complex logic.
• Medium — duplicated shapes that should be unified, hand-rolled complex logic that should be named/shared, convention violations, stale docs that contradict the code, iOS left broken (not merely reduced).
• Low — minor doc gaps, naming, small cleanups.

Number every finding with a severity-class ID so it's easy to refer to later: H / M / L + a sequential number within that class (H1, H2, M1, L1, …), numbered from 1 per class in report order. The IDs are local to this report — a handle for the user, not stable tracker IDs.

Each finding: **H1** path:line — what's wrong, why it matters now, concrete fix. For duplication / extraction findings, list every site and sketch the target shape. For source-of-truth divergences, cite both the promise and the code.

Rules:

• No praise. The report is a list of things to change.
• No quotas. A short sharp report beats a long mixed one. Omit a severity group when it's genuinely empty.
• This skill reports; it does not edit. The user (or a follow-up pass) makes the changes.