feat: validate tokens and repository access up front#341
Merged
GuillaumeLagrange merged 5 commits intoMay 12, 2026
Merged
Conversation
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| 🆕 | Memory | sleep 1 |
N/A | 15.6 KB | N/A |
| 🆕 | WallTime | sleep 1 |
N/A | 1 s | N/A |
| 🆕 | Simulation | sleep 1 |
N/A | 126.3 µs | N/A |
Comparing cod-2628-cli-improve-token-validation-across-commands (58520c3) with main (fd8701d)
9d888b4 to
04f58f9
Compare
adriencaccia
requested changes
May 7, 2026
Member
adriencaccia
left a comment
There was a problem hiding this comment.
I did not finish the review yet. Last two commits still left
adriencaccia
requested changes
May 10, 2026
5c67790 to
38a123f
Compare
adriencaccia
approved these changes
May 12, 2026
Pin the dependency to a revision that surfaces partial `data` on errored responses via `GraphQLError::data()`. Lets callers consume populated sibling fields when a per-field error propagates up to a nullable ancestor — used by the upcoming up-front token validation path to read the session even when `repositoryOverview` errors with REPOSITORY_NOT_FOUND. Refs COD-2628 Co-Authored-By: Claude <noreply@anthropic.com>
Token state used to live in two places: the api_client carried it for GraphQL Authorization headers, while OrchestratorConfig.token (and its clone in ExecutorConfig.token) carried it for upload Authorization headers. The two were filled from different paths and could drift — notably in CI with OIDC, where set_oidc_token mutated config but the api_client kept its original token. Centralize on the api_client: - CodSpeedAPIClient now exposes token() and set_token() and is the single mutation point for the credentials. - RunEnvironmentProvider::set_oidc_token(&mut ExecutorConfig) becomes refresh_token(&mut CodSpeedAPIClient). GHA implements it; other providers inherit the no-op default. - The Buildkite "token required" check moves from try_from(config) to check_oidc_configuration(api_client) — token presence is the api_client's authority now. - Token resolution happens once at CLI entry in build_api_client(&cli): --token / CODSPEED_TOKEN takes precedence; the persisted CLI config is only loaded as fallback. - token field deleted from OrchestratorConfig and ExecutorConfig. The uploader's Authorization header and the tokenless metadata flag both read from api_client.token(). - &mut CodSpeedAPIClient is plumbed through cli/run/exec → Orchestrator → upload_all so refresh_token can mutate in place. Refs COD-2628 Co-Authored-By: Claude <noreply@anthropic.com>
3a9ed50 to
8ede8b0
Compare
Previously, an invalid or mis-scoped token only surfaced as a 401 from the upload endpoint after the full benchmark suite had run. The local provider also had to make two separate GraphQL calls — one to verify the token and one to look up the repository — without a clean error shape for the "missing repo / valid token" case. Combine session validation and repository lookup into a single `session_and_repository_overview` query, and use it for the local provider's resolution path: - `LocalProvider` now validates the token and the repository's `CREATE_LOCAL_RUN` access in one round-trip when resolving from a `-r` override or from a detected git remote. The project-repository fallback is unchanged and still relies on `get_or_create_project_repository` to surface auth errors. - `auth status` now renders the detected git remote alongside the authentication state, using the same combined query so we don't double-roundtrip when a remote is detected. - `auth login` validates the token via `session()` before persisting, so a malformed or expired token is rejected up front instead of being written to disk. - `REPOSITORY_NOT_FOUND` is folded into the success path (`repository_overview: None`) by deserializing the partial-data payload — relies on the gql_client partial-data fork. - `CurrentUser.gql` and `GetRepository.gql` are deleted; replaced by `Session.gql` and `SessionAndRepositoryOverview.gql`. Refs COD-2628 Co-Authored-By: Claude <noreply@anthropic.com>
8ede8b0 to
58520c3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Catch invalid or mis-scoped tokens before any benchmarks run, instead of
surfacing a 401 from the upload endpoint after a full suite has executed.
The local provider now validates the token and the repository's
CREATE_LOCAL_RUNaccess in a single GraphQL round-trip when resolvingfrom a
-roverride or a detected git remote. The project-repositoryfallback is unchanged.
auth statususes the same combined query so itdoesn't double-roundtrip when a remote is detected, and
auth loginvalidates the token via
session()before persisting it.The bulk of the diff is a prerequisite refactor: the api_client is now
the single source of truth for the auth token. Previously the token
lived in two places — on the api_client for GraphQL
Authorizationheaders, and on
OrchestratorConfig/ExecutorConfigfor uploadAuthorizationheaders — and the two could drift, notably in CI withOIDC where
set_oidc_tokenmutated config but the api_client kept itsoriginal token.
CodSpeedAPIClientnow owns the token throughtoken()and
set_token(), the trait method becomesrefresh_token(&mut CodSpeedAPIClient), and&mut api_clientisplumbed through
cli/run/exec → Orchestrator → upload_all. The tokenfield is deleted from both configs.
Reviewers: the two commits are independently reviewable. The first
(
ref: make api_client the single source of truth…) is a purerefactor with no behavior change. The second (
feat: validate tokens…) is the user-facing change.Refs COD-2628