reznok/gmcabilitysystem/gmas-review

Behavior below was verified against GMC 2.3.x and GMAS 1.4. Confirm signatures in this project's own headers (paths are relative to each plugin's root, e.g. Source/GMCCore/Public/Components/GMCReplicationComponent.h); GMC's source is not part of this plugin.

This skill is a review procedure, not a reference. The rules live in gmas:gmas-rules (where state lives, binding order, queue types, server operations), gmas:gmc-prediction (the move cycle, replays, combined moves) and the authoring skills (gmas:gmas-ability, gmas:gmas-effect, gmas:gmas-attribute, gmas:gmas-task); this skill says what to verify, in which order, and how to report it. Inputs: the request (task text, plan or issue), the whole diff (git diff --stat first, then every hunk, new and deleted files included), the tests with their run output, and the implementer's reported deviations. Read the request before the diff, and review the code rather than its description: when a commit message says "guarded against replay", find the guard. GMAS paths below are relative to Source/GMCAbilitySystem/, GMC paths to the GMC plugin root.

Two passes

Pass 1, spec compliance. Does the change do what was asked, all of it and nothing else?

• Every requested item is present: list them from the request and tick each against a hunk. An item delivered differently from the request is either a reported deviation or a finding, never silently accepted.
• Nothing extra: no unrequested features, unrelated refactors or renames, reformatting of untouched lines, or stray files (scratch, logs, generated content the project's generator owns, editor-saved assets the request did not name).
• Tests exist with the names and assertions the request stated, in the module it named; a requested test that was "deferred" is a missing item.
• Each reported deviation is named, has a reason, and is acceptable against the request; an unreported deviation you find is a finding.
• Documentation the request named (README, plan status, provenance notes) is updated.

Pass 2, quality. Correctness, networking, hygiene and tests, with the checklists below; each finding ranked Critical, Important or Minor (definitions under Report format).

Never start pass 2 while pass 1 has open issues: reviewing the quality of code that may be rewritten to meet the spec is wasted, and it tempts the author to fix the quality items and leave the spec gap. When spec fails, the Quality section of the report reads "not started (spec open)".

Networking checklist

Trace every hunk that runs inside a GMC move or touches replicated state on three machines: the owning client, the server (dedicated, and a listen host whose own pawn is never predicted or corrected) and a simulated proxy. The guards and roles are in gmas:gmc-prediction; the GMAS placement rules in gmas:gmas-rules.

• Authority. Outcomes (damage, hits, kills, pickups, score, server operations) are decided only where HasAuthority() holds (AActor, or the ASC's inline role check in Public/Components/GMCAbilityComponent.h); a client branch predicts cosmetics and bound state, never results. Role branches use GMC's queries (IsAutonomousProxy(), IsSimulatedProxy(), IsLocallyControlledServerPawn(), IsRemotelyControlledServerPawn() in Source/GMCCore/Public/Components/GMCReplicationComponent.h), and both sides of every branch were read: the server branch assumes no local player or viewport (dedicated server), the client branch no authority, and whatever one side queues the other side drains.
• Replay guards. Every one-shot inside predicted code (spawn, FX, sound, RPC, score, damage) runs only on a live move: !IsReplayingForGMASLogic() && !IsSimulatedMove() on the owner (1.4+; CL_IsReplaying() on the movement component for non-GMAS code or older trees), !IsSimulatedMove() on the server (IsSimulatedMove() in Source/GMCCore/Public/Components/GMCMovementUtilityComponent.h). A guard on one path and not its twin (the server half, an effect's EndEffectEvent, a task's Completed handler, an OnEffectApplied / OnEffectRemoved handler: those broadcast inside the move, replays included) is a finding.
• Combined moves. A one-shot raised from bound state (a cooldown reaching zero, a queued teleport, a shot) is followed by CL_DoNotCombineNextMove() or driven by an input bound CombineIfUnchanged; otherwise the next combined re-run of the same move fires it again.
• Simulated proxies. Nothing gameplay-relevant in GenSimulationTick; proxy cosmetics come from GenSimulationTick, from bound values with a SimulationMode other than None, or from change delegates. Data a proxy needs at spawn also replicates as an ordinary initial-only property (PostSpawnSmoothingPause).
• Pure predicted logic. GenPredictionTick, ability TickEvent, effect hooks and task Tick read only bound state, ActionTimer, the move's DeltaTime and GetMoveTimestamp(). No GetWorld()->GetTimeSeconds(), FPlatformTime, FDateTime, frame delta, timers, random numbers without a bound seed, anything that differs per machine (cursor, viewport, local player, frame rate, editor-only state), other pawns' displayed transforms, or unbound members written in one move and read in the next. Server-only bookkeeping is not polled from the prediction tick.
• Bound vs replicated. Every new UPROPERTY, attribute row and member has exactly one home from the "where does this state live" table in gmas:gmas-rules. Grep the diff for DOREPLIFETIME, Replicated and bGMCBound = false: none of them is read by predicted logic. Grep for new Bind* calls: none of them is read only by the HUD or cosmetics.
• Binding. BindReplicationData_Implementation calls Super first, binds the game's values in a fixed order with no condition around any bind, sets the ASC's GMCMovementComponent (1.4.1+ resolves a null pointer from the owner and binds nothing, with an Error, when there is none; 1.4.0 dereferenced it unchecked) and ends with the ASC's BindReplicationData(). No Bind* call anywhere else; the same bindings on every machine.
• Clamps. Every new or edited attribute row sets Clamp.Max or MaxAttributeTag, or unticks bClampMax (Public/Attributes/GMCAttributeClamp.h); the default clamp pins the value at 0 (1.4+). Same for the floor.
• Cooldowns. A cooldown the prediction tick reads is a bound float on the movement component; CooldownTime (Public/Ability/GMCAbility.h) and the ASC's ActiveCooldowns (Public/Components/GMCAbilityComponent.h) only where no predicted logic inspects it, because replays do not rewind them, and only with an AbilityTag set (an empty tag never cools down).
• Presentation on every machine. Predicted cosmetics use the ASC helpers with bIsClientPredicted = true on the live move; server-decided cues use false from the server; receivers skip when they have authority or are the predicted owner (the helpers do this; a hand-written multicast must); bDelayByGMCSmoothing where the cue must land on a proxy's displayed position; montages through GMC's PlayMontage_Blocking, never a multicast. The table is in gmas:gmas-ability.
• Server operations. ApplyAbilityEffect(..., ServerAuth), AddImpulse, FireCustomEvent and activations on server-controlled pawns are called on the target's ASC, on the server, and nothing at the call site assumes the operation applied inline: it lands in the target's next move (or next ancillary tick for AI, 1.4+). The first granted candidate passes its gates on both sides.
• Impulses. AddImpulse(FVector Impulse, bool bVelChange) on the target's ASC from the server; "set velocity" is Desired − Current with bVelChange = true; nobody writes another pawn's velocity directly. A pawn's own dash writes its velocity inside its own prediction tick on both sides.
• Activation. Input and AI call QueueAbility from outside the pawn's move (input handler, AI tick, timer), never from GenPredictionTick or an ability tick of the same pawn; TryActivateAbility* appears only in server-only code or tests.
• Time. Code that compares move timestamps or GetTime() across machines has exactly one AGMC_WorldTimeReplicator spawned (Source/GMCCore/Public/Replication/WorldTime.h).

GMAS-specific checks