diff --git a/.cursor/rules/commit-and-review.md b/.cursor/rules/commit-and-review.md index f0c5cae..7bc7b84 100644 --- a/.cursor/rules/commit-and-review.md +++ b/.cursor/rules/commit-and-review.md @@ -35,6 +35,15 @@ When the user asks to **open**, **create**, or **file** a pull request (or clear 7. **Upstream** — If **`create_pull_request`** fails because **`head`** is not on **`origin`**, push first (**only** as allowed under **Never push** above—i.e. the same user message asked to open the PR or explicitly to push). 8. **Fallback** — If the GitHub MCP is unavailable or the call fails after push, give **`https://github.com/{owner}/{repo}/pull/new/{head}`** and tell the user to set the PR title to **`NEO-123:`** … . Do not switch to `gh` as an agent fallback. +### Ticket scope vs mechanical churn (reviewability) + +When a story would otherwise mix **ticket-scoped gameplay/API changes** with **broad, cross-suite edits that do not change runtime behavior** (for example: Arrange/Act/Assert layout-only updates, mass snippet or rule doc tweaks), **prefer** one of: + +- A **separate branch or pull request** for the mechanical churn (often `chore:`), based on or merged separately from the feature branch; or +- **Separate commits** on the story branch so history and blame separate behavioral changes from test-only or doc-only batches. + +That keeps review and bisect focused on product risk. + ## Commit message format when a Linear issue applies - Any commit that is **part of implementing or delivering a Linear issue or task** must put the **Linear issue id first** in the subject line, then **`:`**, then the summary (e.g. `NEO-8: persist position state in PostgreSQL`). diff --git a/docs/reviews/2026-04-27-NEO-28.md b/docs/reviews/2026-04-27-NEO-28.md new file mode 100644 index 0000000..aed7734 --- /dev/null +++ b/docs/reviews/2026-04-27-NEO-28.md @@ -0,0 +1,43 @@ +# Code review — NEO-28 + +- **Date:** 2026-04-27 +- **Scope:** Branch `NEO-28-combat-accept-deny-reason-ux` (`origin/main...HEAD`). +- **Base:** `origin/main` + +## Verdict + +Approve with nits. + +## Summary + +This branch lands the NEO-28 cast accept/deny integration end to end: server-side target authority checks (`invalid_target`, `out_of_range`), client-side cast result handling, HUD feedback, and documentation/manual-QA updates. The server path remains authoritative and aligned with existing targeting/position rules, and targeted API tests pass for the expanded deny matrix. The branch also includes broad AAA-layout cleanup and rule updates across existing tests, which is useful but increases review surface area relative to the feature change. + +## Documentation checked + +- `docs/plans/NEO-28-implementation-plan.md` — **matches** (scope, acceptance criteria, and file touchpoints align with implementation). +- `docs/decomposition/modules/module_dependency_register.md` — **matches** (E1.M4 remains In Progress; note references NEO-28). +- `docs/decomposition/modules/E1_M4_AbilityInputScaffold.md` — **matches** (implementation snapshot and Linear row reflect NEO-28 behavior). +- `docs/decomposition/modules/E5_M1_CombatRulesEngine.md` — **matches** (prototype cast accept/deny staging documented as pre-engine behavior). +- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **matches** (E1.M4 row includes NEO-28 landed state and remaining scope). +- `docs/decomposition/modules/client_server_authority.md` — **N/A for changes** (no conflict observed with server-authoritative cast validation). +- `docs/decomposition/modules/contracts.md` — **N/A for changes** (existing prototype JSON contract approach remains consistent). + +Implementation tracking table/register updates are already included in this branch; no additional status correction appears required for merge. + +## Blocking issues + +None. + +## Suggestions + +1. ~~Consider splitting broad AAA/rule-only churn from ticket-scoped gameplay/API changes in future branches so reviewers can isolate behavior risk more quickly.~~ **Done.** Guideline added under [commit-and-review.md](../../.cursor/rules/commit-and-review.md) (“Ticket scope vs mechanical churn”). + +## Nits + +- ~~Nit: The branch diff is large for a single story because it mixes feature work with cross-suite formatting/rule edits; this is acceptable here but raises cognitive load for future audits and blame.~~ **Done.** Same guideline documents the preferred split for future work; acknowledged for this merge. + +## Verification + +- `dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~AbilityCastApiTests"` — passed (14/14). +- `git diff --check origin/main...HEAD` — passed. +- Not run: headless Godot/GdUnit suite and manual checklist in `docs/manual-qa/NEO-28.md`.