MilpacsService — three adjacent error-handling cleanups (out of scope of #83)

[SyniRon]

Three small inconsistencies surfaced during scoping of the
"every error becomes 404" fix (PR #83); none of them were that bug, so
they were flagged out-of-scope in the spec. Unblocked now that #83 has
merged. Bundle into a single chore PR.

1. Roster handlers use plain errors.New instead of status.Errorf(codes.InvalidArgument, ...).
   GetRoster / GetLiteRoster / GetS1UniformsRoster all do
   return nil, errors.New("cannot request null roster type") on
   RosterType_ROSTER_TYPE_UNSPECIFIED. grpc-gateway wraps that as
   codes.Unknown → HTTP 500, when it should be
   codes.InvalidArgument → HTTP 400 (consumer sent a bad enum). Three
   one-line swaps.

2. Empty Keycloak / Discord ID warn-but-continue.
   GetUserViaKeycloakId and GetUserViaDiscordId log a warning then
   fall through to the lookup with the empty string. Should early-return
   status.Errorf(codes.InvalidArgument, ...) like GetGamertagProfile
   already does. Two roughly-symmetric blocks.

3. processProfiles-error wrap inconsistency.
   FindProfileByKeycloakID and FindProfileByDiscordID return
   fmt.Errorf("error generating profile") — no %w, no underlying err
   in the message. The other three single-row finders (FindProfilesById,
   FindProfilesByUsername, FindProfileByGamertag) all use
   fmt.Errorf("error generating profile: %w", err). The S1-uniforms
   inline loop (FindS1UniformsRosterByType) has the same bare-string
   outlier. Make all of them match. No observable consumer change today
   (handler maps all to codes.Internal), but the inconsistency will
   break a future errors.Is check the moment one is added.

Bundled because they all touch the same files and split cleanly per
commit.

[SyniRon]

This was generated by AI during triage.

Agent Brief

Category: chore
Summary: Three adjacent error-handling cleanups in MilpacsService that were flagged out-of-scope of PR #83. Bundle into a single PR with one commit per cleanup.

Current behavior:

1. Roster handlers leak codes.Unknown. The three roster handlers (GetRoster, GetLiteRoster, GetS1UniformsRoster) reject the ROSTER_TYPE_UNSPECIFIED enum with a bare errors.New(...). grpc-gateway wraps that as codes.Unknown and the HTTP gateway maps it to 500 — even though the consumer just sent an invalid enum, which is the canonical 400 case.

2. Empty-ID handlers warn-but-continue. GetUserViaKeycloakId, GetUserViaDiscordId, and GetGamertagProfile all check for an empty input ID, log Warn.Println(...), and then fall through to the datastore call with the empty string anyway. There's no early return; the datastore is asked to look up the empty string and either returns gorm.ErrRecordNotFound (→ 404) or hits a query error (→ 500). Either is the wrong shape — the consumer sent an invalid argument and should get 400 immediately.

3. processProfiles error-wrap inconsistency. FindProfileByKeycloakID, FindProfileByDiscordID, and the S1-uniforms inline loop in FindS1UniformsRosterByType wrap profile-generation errors with fmt.Errorf("error generating profile") — no %w, no underlying error in the message. The other single-row finders (FindProfilesById, FindProfilesByUsername, FindProfileByGamertag) all use fmt.Errorf("error generating profile: %w", err). No observable consumer change today (handler maps all to codes.Internal) but the bare-string outliers will break any future errors.Is check the moment one is added.

Desired behavior:

1. Each of the three roster handlers returns status.Errorf(codes.InvalidArgument, "cannot request null roster type") instead of the bare errors.New. Through the HTTP gateway this becomes a 400.

2. Each of the three empty-ID handlers (GetUserViaKeycloakId, GetUserViaDiscordId, GetGamertagProfile) early-returns status.Errorf(codes.InvalidArgument, "<id-name> required") (or equivalent) when the request's ID is empty. No datastore call, no Warn.Println — the 400 to the consumer is the only correct response.

3. All five error generating profile-style wraps emit fmt.Errorf("error generating profile: %w", err) so errors.Is works across the board.

No proto / OpenAPI / interface changes. No release cut needed.

Key interfaces:

• The three roster handlers in MilpacsService that gate on ROSTER_TYPE_UNSPECIFIED.
• The three empty-ID handlers in MilpacsService that today only Warn.Println. Note: GetGamertagProfile is included even though the parent issue body only named the two ID handlers — it's the same shape and should be fixed for consistency.
• The three Mysql finders that wrap profile-generation errors without %w. Mirror the pattern already used by the five sibling finders that have it right.
• The fakeDatastore harness in servers/grpc/fake_datastore_test.go is the right place to assert the new mappings per RPC.

Acceptance criteria:

• Three new unit tests assert that each of GetRoster / GetLiteRoster / GetS1UniformsRoster returns codes.InvalidArgument for ROSTER_TYPE_UNSPECIFIED.
• Three new unit tests assert that each of GetUserViaKeycloakId / GetUserViaDiscordId / GetGamertagProfile returns codes.InvalidArgument for an empty input ID without calling the datastore (assertable via the fake's call-count or by not arming a return value).
• One new unit test (or one extension of an existing test) asserts that errors.Is(err, <some-sentinel>) works through the wrap from FindProfileByKeycloakID (or one of the three patched call sites) — proves the %w change is observable.
• All five error generating profile wraps emit : %w consistently. Easy to verify with a grep — the bare-string variants should be zero after the change.
• Existing tests stay green; go build ./... clean; make lint clean.
• PR is split into three commits, one per cleanup, so reviewer can read them independently.

Out of scope:

• Any change to which scopes the handlers gate on (RequireScope(ctx, "read") stays).
• Touching TicketsService — its error-routing is already correct (see PR fix(milpacs): route errors to NotFound vs Internal #83 context).
• Changing gorm.ErrRecordNotFound handling — PR fix(milpacs): route errors to NotFound vs Internal #83 already wired this correctly across the board.
• Adding new errors.Is(err, gorm.ErrRecordNotFound) checks beyond what PR fix(milpacs): route errors to NotFound vs Internal #83 already added.

Interaction note (no dependency wired):

The Keycloak half of cleanup #2 and one of the FindProfileByKeycloakID lines in cleanup #3 will both evaporate when #100 (Keycloak removal Phase 2) lands. Fixing them symmetrically today is still right — small diff now, deleted symmetrically later. No need to coordinate the merge order with #97 or #100; they don't conflict.