githubnext / githubnext/ado-aw

refactor(compile): unify AzureCLI@2 token-mint steps behind a shared typed helper

Open
#1,378 1 comment 0 reactions 0 assignees View on GitHub
refactor rust
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.