feat(diff): abstract PR hosting behind a provider trait; add Azure DevOps#162
Open
Joehoel wants to merge 11 commits into
Open
feat(diff): abstract PR hosting behind a provider trait; add Azure DevOps#162Joehoel wants to merge 11 commits into
Joehoel wants to merge 11 commits into
Conversation
…vOps
The "lumen diff --pr" PR review flow was hardcoded to GitHub and the gh
CLI throughout. Introduce a PrProvider trait (src/command/diff/pr_provider.rs)
so other forges can be supported, and move the existing GitHub logic behind a
GitHubProvider impl.
Add an AzureProvider backed by the Azure DevOps REST API
(src/command/diff/azure.rs):
- parses dev.azure.com and *.visualstudio.com PR URLs and remotes (HTTPS + SSH)
- fetches PR metadata, the iteration change list, and blob content over REST
- diffs against the merge base (iteration changes with $compareTo=0), i.e. the
same three-dot view the Azure web UI shows, instead of a tip-to-tip diff
- fetches blobs concurrently over pooled HTTP (reusing the existing async
reqwest + tokio; no new dependencies)
- supports --detect-pr and open-in-browser ('o')
Why REST and not the az CLI: az has no first-class command to list a PR's
changed files or fetch file content; the only CLI route is the generic
`az devops invoke` passthrough, which spawns a Python process per call
(2 per file) and is unusably slow for an interactive diff. REST also lets us
drop the azure-devops extension requirement entirely.
Auth resolves to a PAT (ADO_PAT / AZURE_DEVOPS_EXT_PAT) via HTTP Basic, or a
bearer token from `az account get-access-token` (core az, no extension).
Per-file viewed-state sync stays a GitHub-only capability via a default no-op
on the trait (Azure DevOps has no equivalent API).
The provider is selected from the PR URL, falling back to the git origin
remote, so bare PR numbers route to the right forge. Adds unit tests for
URL/remote parsing, provider detection, and Azure change-entry parsing.
Refs jnsahaj#118
- blob_text returns Result so a failed fetch can't silently empty a side and flip a file's status to Added/Deleted; the load fails instead. - drop the unreachable HTTP 203 branch in auth_hint. - hoist a single percent_encode helper into git.rs, used by both the Azure REST client and the PR web-URL builder.
Reorganise the PR-provider abstraction so adding a forge is one module plus one registry entry, and provider-specific code stops leaking across files: - pr_provider/ becomes a directory module: github.rs (all the gh-CLI logic, moved out of diff/mod.rs and git.rs and de-prefixed) and azure/ (mod.rs for URL/remote parsing + the PrProvider impl, client.rs for the REST client). - Replace ProviderKind + handler() match + per-method dispatch with a &'static [&dyn PrProvider] registry; selection iterates it. Store the provider as &'static dyn PrProvider directly on PrInfo (trait gains : Sync so it can cross the viewed-sync threads), dropping the enum entirely. - git.rs is now generic VCS only; mod.rs no longer holds github_* functions. Behaviour-preserving; errors are still String and PrInfo still carries the Azure Option fields (addressed in follow-up commits).
The PrProvider trait and its impls now return a thiserror PrError whose variant is the kind — Auth / NotFound / InvalidRef / Other — so the UI can react to the failure (e.g. prompt for credentials on Auth) rather than only display a string. Classification happens where it's reliable: - Azure REST: HTTP 401/403 -> Auth, 404 -> NotFound; missing/!ok az creds -> Auth. - Parse failures -> InvalidRef; "no PR for branch" -> NotFound. A From<String>/From<&str> impl lets the remaining opaque CLI/transport errors fall through to Other via `?`, so internal helpers stay terse.
PrInfo carried node_id (GitHub-only) plus project/org_url (Azure-only) as Option fields that every provider saw as None for the others. Replace them with a single ProviderData enum whose variant holds exactly the fields its forge needs, so each provider pattern-matches its own data (no spurious None, and adding a forge is a new variant rather than two more Options). Common fields (refs, repo owners) stay on PrInfo.
A supports_viewed_sync() -> bool can silently drift from the fetch/set impls (override one, forget the other). Replace the three methods with a single viewed_sync() -> Option<&dyn ViewedSync>: having the impl *is* the capability, so it can't disagree with itself. GitHub returns Some(self) and implements ViewedSync; Azure inherits the None default. The fetch/set facades route through the Option, no-op when absent.
Stop hand-navigating serde_json::Value (and, on the GitHub side, scanning raw JSON with str::find) and deserialize into small typed structs instead — the compiler now checks the wire shape and the structs document it. - azure/client.rs: a generic get::<T: DeserializeOwned> replaces get_json; IterationList, ChangesPage/RawChange/RawItem, PrDetail, PrList, and TokenResponse model each endpoint. RawChange::into_change keeps the folder-skip / rename-fallback logic (fallible, so a method returning Option rather than a From impl). - github.rs: GraphQl<T> + RepoNode<T> envelopes with #[serde(rename_all = camelCase)] replace extract_json_string / extract_nested_login and the viewerViewedState string scan, which were brittle to whitespace/key order. - Add parse tests locking the GraphQL wire contract (incl. deleted head fork). Unknown fields are ignored by default, preserving the prior "read only what we need" tolerance. Behaviour-preserving; 148 tests pass.
ADO_PAT is a made-up alias nothing sets; AZURE_DEVOPS_EXT_PAT is the conventional Azure DevOps PAT var (read by the az devops extension), and a plain az login is enough on its own via the bearer-token fallback. Reorder the PAT precedence to check the conventional var first, and point the docs, README, and error messages at az login / AZURE_DEVOPS_EXT_PAT instead of the alias.
The // ---- divider banners and step-narrating comments (// Try to parse
as a URL first, etc.) don't match the rest of the codebase, which uses
neither. Keep only the why-comments.
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
2 participants
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.
lumen diff --prnow works with Azure DevOps, not just GitHub. This is the provider-trait abstraction proposed in #118: the GitHub plumbing moved behind aPrProvidertrait, so adding a forge (GitLab, Gitea) is one module plus a registry line. GitHub behaviour is unchanged. GitLab itself is a small follow-up.Azure auth is just
az login(we mint a bearer token viaaz account get-access-token), or a PAT inAZURE_DEVOPS_EXT_PAT; noazure-devopsextension needed. Diffs use the iteration-changes api endpoint, the same merge-base view the web UI shows.CleanShot.2026-06-09.at.20.25.06.mp4