Skip to content

Conversation

@tudddorrr
Copy link
Collaborator

No description provided.

@tudddorrr tudddorrr added the release This pull request contains a release label Nov 20, 2025
@claude
Copy link

claude bot commented Nov 20, 2025

🔴 Code Quality and Best Practices

Policy file not updated to use new scopes

The PR introduces READ_PLAYER_PRESENCE and WRITE_PLAYER_PRESENCE scopes, but src/policies/api/player-presence-api.policy.ts still uses the old generic scopes:

// Current (incorrect)
async get(): Promise<PolicyResponse> {
  return this.hasScope(APIKeyScope.READ_PLAYERS)
}

async put(): Promise<PolicyResponse> {
  return this.hasScope(APIKeyScope.WRITE_PLAYERS)
}

Should be:

async get(): Promise<PolicyResponse> {
  return this.hasScope(APIKeyScope.READ_PLAYER_PRESENCE)
}

async put(): Promise<PolicyResponse> {
  return this.hasScope(APIKeyScope.WRITE_PLAYER_PRESENCE)
}

This undermines the intent of adding granular scopes - API keys would still need broad READ_PLAYERS/WRITE_PLAYERS access instead of the more limited presence-specific scopes.


🔴 Test Coverage

Tests use wrong scopes

The test files need updating:

  • tests/services/_api/player-presence-api/get.test.ts:9 - Uses READ_PLAYERS instead of READ_PLAYER_PRESENCE
  • tests/services/_api/player-presence-api/get.test.ts:53 - Same issue
  • tests/services/_api/player-presence-api/put.test.ts:10 - Uses WRITE_PLAYERS instead of WRITE_PLAYER_PRESENCE
  • tests/services/_api/player-presence-api/put.test.ts:41 - Same issue
  • tests/services/_api/player-presence-api/put.test.ts:85 - Same issue

The tests pass with the old scopes, masking the policy bug.


🔵 Minor - Documentation

The API docs in src/docs/player-presence-api.docs.ts should mention the required scopes (READ_PLAYER_PRESENCE for GET, WRITE_PLAYER_PRESENCE for PUT) to help developers understand which scopes are needed for these endpoints.

@tudddorrr tudddorrr merged commit f017bf0 into main Nov 20, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release This pull request contains a release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants