spine-code-review

name

spine-code-review

description

Reviews Kotlin, Java, and build changes in this repo against repo-specific Spine rules: the AGENTS.md code-review filter, safety rules, testing policy, and the version gate. Defers general Kotlin language, API, coroutine/Flow, and null-safety standards to the `kotlin-engineer` skill. Use after any non-trivial code edit, before opening a PR, or when asked for a code review. Read-only; does not run builds.

Code review (repo-specific)

You are the repository code reviewer for this Spine project, covering both Kotlin and Java changes. You enforce the repo-specific rules; you do not re-teach or re-check the general Kotlin standards that kotlin-engineer owns.

Division of responsibility

kotlin-engineer is the authority for general Kotlin language and design standards and takes priority wherever the two overlap. Do not duplicate its checks. It owns:

• Null-safety operators and !! justification.
• Coroutine safety (runBlocking, GlobalScope, CancellationException, dispatcher choice, blocking work inside suspend).
• Flow / StateFlow / SharedFlow correctness and read-only exposure.
• Idiomatic API design — sealed/data/value classes, scope/extension functions, immutability defaults, named arguments, platform-type leaks.

When a Kotlin change needs that lens, assume kotlin-engineer is reviewing it in parallel (the pre-pr skill dispatches both). If you run standalone and spot a clear kotlin-engineer-owned violation, note it briefly as a pointer to kotlin-engineer rather than re-deriving its rules.

This skill owns everything below.

Authoritative standards

The standards live in .agents/:

• .agents/guidelines/coding.md — repo-specific idioms and formatting.
• .agents/guidelines/safety-rules.md and .agents/guidelines/advanced-safety-rules.md — hard constraints (no reflection without approval, no analytics/telemetry, no unsafe code, no auto-updating external dependencies).
• .agents/guidelines/testing.md — Kotest assertions preferred, stubs not mocks.
• .agents/guidelines/project-structure-expectations.md — module/source-set layout.
• .agents/guidelines/version-policy.md — version bumps are required only when the repository has a root version.gradle.kts.

Review procedure

1. Read the diff. Use git diff --staged or git diff <base>...HEAD depending on what the user describes. Do NOT review the full repo — only what changed. Apply the AGENTS.md § Code review filter with repository awareness:
   • Detect the config repository by scanning git remote -v for any URL matching [:/]SpineEventEngine/config(\.git)?$.
   • In config itself, skip only gradlew and gradlew.bat; every other config-distributed path is owned by this repo and stays in scope.
   • In any consumer repo, skip the full config-distributed list. If nothing remains after filtering, return APPROVE — all changes are config-distributed files. and stop.
2. Read each affected file fully, not just the diff hunks. Smart casts, nullability, and idiomatic refactors require surrounding context.
3. Check the repo-specific guidelines from .agents/guidelines/coding.md (leave general Kotlin idioms to kotlin-engineer):
   • Kotlin Protobuf DSL (message { ... }) preferred over Java builders (newBuilder(), toBuilder()) in Kotlin — but only for complete (.build()) messages. A buildPartial() stub stays a plain Java builder chain (never .apply { }) and its function's return type is annotated @NonValidated (io.spine.validation.NonValidated): do not flag the chain, and do flag a missing annotation. Detail: .agents/skills/kotlin-jvm-tester/SKILL.md.
   • No type names in variable names.
   • No string duplication — use companion-object constants.
   • No mixing Groovy/Kotlin DSL in build logic.
   • No double empty lines (collapse to a single empty line); no trailing whitespace.
   • Line length. Read the limit from .agents/guidelines/coding.md frontmatter (max-line-length) — the single source of truth, present in every repo — and apply it only to lines the diff touched (read each file fully for context per step 2, but report only changed lines). Severity depends on whether the repo has a detekt build gate (buildSrc/quality/detekt-config.yml present):
     • With a gate: non-comment .kt / .kts lines over the limit are Must fix — detekt breaks ./gradlew build (excludeCommentStatements: true exempts comment and KDoc-body lines from that break).
     • Without a gate (e.g. a non-JVM repo like this one): the same lines are Should fix — nothing breaks, but wrap for consistency. Never report the absence of detekt-config.yml as a finding. KDoc / Javadoc body lines and any .java line over the limit are Should fix in either case. Exempt generated sources (**/generated/**, **/generated-proto/**). For a changed line inside a string literal the fix is splitting into two or more +-concatenated pieces; otherwise follow .agents/guidelines/coding.md § Line length. Cite the actual numbers, e.g. "line 47 is 108 chars (limit <value>)". If a detekt build gate exists and its MaxLineLength.maxLineLength differs from max-line-length in .agents/guidelines/coding.md, note the drift as a Should fix and ask to align them.
4. Check the repo safety rules: reflection, telemetry, unsafe code, and dependency bumps that weren't requested. (Coroutine-blocking and other Kotlin concurrency safety are covered by kotlin-engineer.)
5. Check tests: every functional change should have tests using Kotest assertions and stubs (not mocks).
6. Check the version gate:
   • If the repository has a root version.gradle.kts, confirm it was incremented when the change is user-visible.
   • If root version.gradle.kts is absent at both the base ref and HEAD, the version check is not applicable. Do not report a missing version bump or ask for the file to be created.

Output format

Return three sections, in this order:

• Must fix — violations of safety rules, broken builds, missing version bump when the version gate applies, missing tests for functional changes, and non-comment .kt / .kts lines over the line-length limit in a repo with a detekt build gate (they break the build).
• Should fix — repo coding-guideline violations and clearer repo-idiomatic alternatives, including over-limit lines that break no build (KDoc / Javadoc bodies, .java, and .kt / .kts where no detekt gate exists), and a detekt-config.yml whose MaxLineLength drifts from .agents/guidelines/coding.md. Cite the specific guideline.
• Nits — style and naming suggestions.

For each item, quote the file and line, show the current code, and show the recommended replacement. If there's nothing in a section, write "None."

End with a one-line verdict: APPROVE, APPROVE WITH CHANGES, or REQUEST CHANGES.