githubnext / githubnext/ado-aw
refactor(compile): unify AzureCLI@2 token-mint steps behind a shared typed helper
- Dominant language
- Rust
- Stars
- 23
- Forks
- 8
- Avg merge
- 4d 9h
- Merged PRs (30d)
- 22
Description
## Summary
There are now multiple near-identical `AzureCLI@2` token-mint steps in the compiler that all follow the same contract (`az account get-access-token … --query accessToken -o tsv` → `##vso[task.setvariable variable=…;issecret=true]`) but are built two different ways:
- `common::generate_acquire_ado_token` (`src/compile/common.rs`) — the ADO read/write tokens (`SC_READ_TOKEN` / `SC_WRITE_TOKEN`, from `permissions.read`/`write`). Built as a **hand-assembled raw YAML string**, with manual service-connection quoting (`sc.replace('\'', "''")`).
- `provider_token_mint_step` (`src/compile/agentic_pipeline.rs`) — the BYOK provider bearer token (`AW_PROVIDER_BEARER_TOKEN`), added in #1377. Built with the **typed `AzureCli` builder** + a validated `ProviderResourceUrl` / `ServiceConnection` newtype, `set -eo pipefail`, and single-quoted resource.
The two read consistently at the contract level but diverge in implementation. The provider step follows the newer target patterns (typed IR builders, `secure.rs` newtypes); `generate_acquire_ado_token` is the older raw-YAML approach.
## Proposal
Extract a single shared typed helper, e.g.:
```rust
fn acquire_azure_token_step(service_connection: &str, resource: &str, out_var: &str /*, opts */) -> TaskStep
```
and migrate both the ADO read/write token paths and the provider path onto it. Benefits: one place for the mint idiom, typed construction everywhere, serde-handled escaping (drop the manual `replace('\'', "''")`), and consistent `set -eo pipefail`.
## Notes / open questions
- **`addSpnToEnvironment: true`** — set by `generate_acquire_ado_token`, not by `provider_token_mint_step`. It is **not required** for `az account get-access-token` (the `AzureCLI@2` task already authenticates `az` from the service connection; neither script consumes `$servicePrincipalId/Key`). The shared helper should default it **off**; confirm nothing relies on the ADO path exporting SPN env vars before removing it.
- **`set -eo pipefail`** — the shared helper should include it (the ADO path currently lacks it).
- **Resource typing** — the ADO path uses a constant `ADO_RESOURCE_ID`; the provider path uses a user-supplied, validated `ProviderResourceUrl`. The helper should accept an already-shell-safe resource string.
- **Blast radius** — `generate_acquire_ado_token` feeds `SC_READ_TOKEN` / `SC_WRITE_TOKEN` consumption across all four targets (standalone / 1es / job / stage) and interacts with `apply_bundle_auth` / `token_source_for` (`SC_WRITE_TOKEN`). This is why it was intentionally left out of #1377 (the #1372 fix) and split into this follow-up. Verify emitted lock YAML is byte-stable for the ADO paths after migration.
Follow-up to #1377 / #1372.
Contributor guide
No contributing guide indexed for this repository
Research direction
Read src/compile/common.rs and src/compile/agentic_pipeline.rs, comparing generate_acquire_ado_token with provider_token_mint_step and their typed builders. Trace apply_bundle_auth, token_source_for, and the four ADO targets before checking whether SPN environment variables are consumed. Done means both paths use the shared helper and emitted ADO lock YAML remains byte-stable.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- build-system, ci-cd
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100