[GitHub] Store accepted token scopes#11915
Conversation
Assisted-by: Codex <gpt-5.5>
Assisted-by: Codex <gpt-5.5>
Assisted-by: Codex <gpt-5.5>
Assisted-by: Codex <gpt-5.5>
Assisted-by: Codex <gpt-5.5>
Assisted-by: Codex <gpt-5.5>
Assisted-by: Codex <gpt-5.5>
Assisted-by: Codex <gpt-5.5>
Assisted-by: Codex <gpt-5.5>
Assisted-by: Codex:gpt-5.5
Assisted-by: Codex:gpt-5.5
…copes # Conflicts: # core/token-pooling/sql-token-persistence.integration.js
Assisted-by: Codex:gpt-5.5
Assisted-by: Codex:gpt-5.5
Assisted-by: Codex:gpt-5.5
Assisted-by: Codex:gpt-5.5
Assisted-by: Codex:gpt-5.5
Assisted-by: Codex:gpt-5.5
|
|
Thanks for the contribution! We've got an ongoing token issue in #11912, let's put this aside for now. I don't think your work would have much impact, but I want to minimise the number of moving parts. :) |
Moving this back to draft for now. Feel free to set this aside while #11912 is being investigated. I can rebase or rewrite it later if needed. |
|
Okay, I think it's a good point to resume this work. :) |
…-scopes # Conflicts: # core/token-pooling/token-pool.js
|
I noticed a couple of checks are still failing after this was brought back up to date. I pushed an empty commit to trigger a fresh CI run, but the failures still look external:
This seems likely related to the ongoing GitHub incident: https://www.githubstatus.com/incidents/v0b3bpsyvqtk We may need to retry the failed checks later, once GitHub's API status has recovered. |
|
The Danger failure will hopefully be fixed by #11957! |
PyvesB
left a comment
PyvesB
left a comment
There was a problem hiding this comment.
Overall this looks good, I think it will work as expected. A couple of simplifying thoughts inline.
| expect(tokens.map(({ token }) => token).sort()).to.deep.equal( | ||
| initialTokens, | ||
| ) | ||
| expect(tokens.map(({ scopes }) => scopes)).to.deep.equal([ | ||
| null, | ||
| null, | ||
| null, | ||
| ]) | ||
| }) | ||
|
|
||
| it('loads token scopes', async function () { | ||
| const scopedToken = 'd'.repeat(40) | ||
| await pool.query( | ||
| `INSERT INTO pg_temp.${tableName} (token, scopes) VALUES ($1::text, $2::text[]);`, | ||
| [scopedToken, ['read:packages', 'read:user']], | ||
| ) | ||
|
|
||
| const tokens = await persistence.initialize(pool) | ||
| expect(tokens).to.deep.include({ | ||
| token: scopedToken, | ||
| scopes: ['read:packages', 'read:user'], | ||
| }) |
There was a problem hiding this comment.
Perhaps we could combine these two tests, and insert a mixture of tokens with and without scopes in beforeEach? My understanding is that this will more closely mirror what is effectively in the pool once this is running in production.
| if (!next.isValid) { | ||
| // Discard, and | ||
| continue |
There was a problem hiding this comment.
I'm wondering whether we should be calling this.priorityQueue.deq() here. This side effect would be that we wouldn't have to call this._removeFromPriorityQueue(existingToken) and rebuild a priority queue when adding a token, we'd effectively lazily remove from priorityQueue on access.
Why
GitHub token pool entries are currently persisted as token strings only. That makes it hard to support GitHub API features which need token-level metadata, such as knowing which OAuth scopes were granted to each accepted token.
This is a preparatory change for scope-aware GitHub API usage. It stores accepted token scopes without changing the scopes requested by the existing GitHub OAuth flow.
Related background:
What
scopes text[]column togithub_user_tokens.GithubConstellation, and the GitHub API provider token pools.TokenPool.add()to refresh explicitly provided metadata for an existing token without clearing metadata when no new data is provided.Notes
scopes = NULLmeans the token scope metadata is unknown, which is the state for existing persisted tokens.scopes = '{}'means the token was accepted with no granted scopes.TokenPoolstill uses the existing FIFO / priority queue scheduling model. A more complete design would refactor token management so token identity, persistent metadata, and runtime scheduling are handled separately. This PR intentionally avoids that larger refactor and only patches duplicate-token handling so in-process token metadata can stay in sync with persistence.Validation
Manual validation beyond CI:
github_user_tokens.scopes text[]was created.NULL, empty, and populatedscopesvalues and confirmed they round-trip with distinct semantics.scopes: ['read:packages', 'read:user'], confirmed it persisted to PostgreSQL, then created a freshGithubConstellationand confirmed the metadata loaded into the standard, search, and GraphQL token pools.scopescolumn was removed.