Commit 6d4c824
feat(secrets): add --all-repos and --source via Pipeline Preview discovery (#624)
* feat(secrets): add --all-repos and --source via Pipeline Preview discovery
Adds project-scope token management via two new flags on `secrets set`,
`secrets list`, and `secrets delete`:
- `--all-repos` — operate on every ado-aw pipeline ADO knows about in
the project (direct ado-aw definitions *and* consumer pipelines that
include ado-aw templates), regardless of which repo their root YAML
lives in.
- `--source <path>` — filter to consumers of one specific template.
Both flags activate a new Preview-driven discovery path that calls
`POST /_apis/pipelines/{id}/preview` per definition and scans the
expanded YAML for an `# ado-aw-metadata: {…}` JSON marker. The legacy
lexical local-fixture matcher remains the default; `--definition-ids`
remains the explicit-ID escape hatch.
To make discovery work, every compiled pipeline now carries a marker
via a new always-on `AdoAwMarkerExtension`. The marker lives inside a
bash Setup-job step because ADO's Preview API strips top-of-document
comments during YAML expansion (verified empirically against live def
2434 in `msazuresphere/4x4`) but preserves comments inside step
bodies. Uniform across all four targets (standalone / 1es / job /
stage); no per-target placement special-casing.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(secrets): address Rust PR review feedback on #624
- `is_direct_match`: return `false` (not `markers.is_empty()`) for the
marker-less case. The previous code returned `true` for 0 markers,
which would misclassify a non-ado-aw definition as `Direct` if any
future caller hit that path. Belt-and-braces — the current callsite
in `classify_definition` already guards against it.
- `discovered_to_matched`: doc-comment claimed `UnknownRequiredParams`
was propagated; implementation drops it. Keep the safer drop
behaviour (we can't act on a marker-less definition) but surface a
`warn!` summary from `resolve_definitions_via_discovery` so
`secrets set --all-repos` operators can see when pipelines were
skipped because of required-template-parameters / 403 / other
preview failures. Doc comment updated to match.
- `AdoContext::repo_url`: percent-encode `project` and `repo_name` so
`DiscoveryScope::CurrentRepo` works for projects whose names contain
reserved characters (e.g. spaces). The lowercase normalize step
can't reconcile a decoded local name with the encoded form ADO
returns in `repository.url`.
- `AdoAwMarkerExtension`: bash-quote-escape the source path embedded
in the runtime `echo` line. A markdown filename containing `'`
(e.g. `agents/foo's.md`) would otherwise produce syntactically
broken bash. New `bash_single_quote_escape` helper applies the
canonical `'\''` idiom; the JSON marker line keeps the raw value
because JSON has no quoting concern with `'`. Two new tests cover
the idiom and a `foo's-agent.md` path.
- `src/detect.rs`: drop the now-stale `#[allow(dead_code)]` attrs on
`MARKER_STEP_PREFIX`, `MarkerMetadata`, and `parse_marker_step`.
All three are actively consumed by `src/ado/discovery.rs`.
- `resolve_for_command`: thread local-lock-file paths into discovery
so the `process.yamlFilename` fast-path can skip Preview calls for
locally-compiled pipelines. Best-effort scan — failures fall back
to Preview-for-everything cleanly.
All 1741 tests pass; clippy clean on touched files.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(secrets): address second-round Rust PR review feedback on #624
- `AdoAwMarkerExtension`: neutralise `##vso[` and `##[` logging-command
prefixes in the source path before embedding it in the marker step's
runtime `echo` line. Without this, a markdown filename like
`agents/##vso[task.setvariable variable=FOO]value.md` would echo a
literal `##vso[...]` sequence that the ADO build agent's stdout
scanner treats as a task command — a logging-command-injection
primitive any attacker controlling a filename could trigger. New
`sanitize_for_vso_logging` helper mirrors the existing convention in
`crate::agent_stats::sanitize_for_markdown` (`[vso-filtered][` /
`[filtered][`). The `# ado-aw-metadata:` JSON line keeps the raw
value (it's a YAML comment, not echoed to stdout). Two new tests:
the sanitiser unit test and an end-to-end attack-payload roundtrip
asserting the echo line is neutralised.
- `resolve_definitions_via_discovery`: the previous skip-counter
implementation counted `UnknownRequiredParams` / `Forbidden` /
`PreviewFailed` failures *before* applying `source_filter`, so under
`--source agents/foo.md` the warnings would tell the user "N
definitions skipped requiring template parameters" for definitions
that had nothing to do with `agents/foo.md`. Split the counting:
* without `--source`: per-status counts are honest (we're operating
on every ado-aw pipeline) and the existing three warnings stand;
* with `--source`: a single conservative `uninspectable` counter,
surfaced as one warning that explicitly acknowledges we can't tell
whether any of those skipped definitions would have been consumers
of the filtered template.
- `src/ado/discovery.rs`: drop the file-level `#![allow(dead_code)]`.
`resolve_definitions_via_discovery` and `discovered_to_matched` are
now wired into `secrets.rs`; the suppression was hiding future
dead-code regressions. Build is clean without it.
- `src/main.rs` (`SecretsCmd`): clarified `--source` help text — calls
out that **without `--all-repos`, only the current repository is
searched**. Saves the user-confusion case "I ran `secrets set
GITHUB_TOKEN --source agents/foo.md` and got zero results" when
they're in a different repo than the consumer pipelines.
All 1743 tests pass; clippy clean on touched files.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(secrets): address third-round Rust PR review feedback on #624
- `resolve_definitions_via_discovery`: normalize the user-supplied
`--source` value through `normalize_source_path` before comparing
against `marker.source`. The marker stores the canonical form
(`agents/foo.md`), so without normalization a user typing
`--source ./agents/foo.md` or `--source agents\foo.md` (Windows)
silently matched nothing. Re-export `normalize_source_path` from
`crate::compile` so callers outside the compile module tree can
reach it cleanly. New test asserts the three common variants
(canonical, leading-`./`, backslash) all produce the same normalized
string.
- `classify_definition` / `DiscoveryStatus::NotFound`: 404 from the
Preview endpoint almost certainly means the definition was deleted
between `list_definitions` and `preview_pipeline` (TOCTOU race).
Previously routed through `PreviewFailed`, which inflated
`skipped_failed` counts and confused operators. New
`DiscoveryStatus::NotFound` variant is excluded from skip-warning
counters in `resolve_definitions_via_discovery` and dropped by
`discovered_to_matched` like the other non-actionable statuses.
Debug-logged with the definition id+name so `--debug` users can
still see what happened.
- `DefinitionSummary::revision`: doc comment claimed Preview-driven
discovery uses it as a cache key, but no caching is implemented.
Rewrote to say it's deserialised for a future cache and there is
*no* caching yet, with a clear "see the discovery module for current
behaviour" pointer.
- `DiscoveryScope::Explicit`: clarified the docstring to call out
that no production callsite constructs this variant — `--definition-ids`
uses the legacy `resolve_definitions` path before discovery ever
runs. Variant is kept (not removed and not `#[cfg(test)]`-gated)
because direct API consumers may want to feed pre-filtered IDs into
discovery; the existing unit-test construction stays.
- `secrets::resolve_for_command`: bail early with a targeted error
when `--source` is used without `--all-repos` outside a recognised
ADO repo. The previous behaviour was a generic "No ado-aw pipelines
found via Preview-driven discovery" message that gave no hint that
the empty result was caused by the missing git remote. New error
spells out the cause and suggests `--all-repos`.
All 1746 tests pass; clippy clean on touched files.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(secrets): address fourth-round Rust PR review feedback on #624
- `normalize_repo_url`: percent-decode before comparing, so a project
named "My Project" matches whether ADO returns the encoded form
(`My%20Project`) or the decoded form. The previous implementation
assumed ADO always returns percent-encoded URLs; that assumption is
documented in code now and the comparison is encoding-independent.
New unit tests cover the encoded/decoded equivalence and the
case-insensitive/trailing-slash behaviour.
- `discovered_to_matched`: stop silently truncating consumers that
include multiple ado-aw templates. The `yaml_path` field used by
`print_matched_summary` now joins every marker source with `, ` so
e.g. `agents/a.md, agents/b.md` shows up honestly in the CLI
summary. New unit test asserts both sources are surfaced.
- `##vso[` defence-in-depth: the marker step's runtime echo already
neutralises `##vso[` and `##[` prefixes, but the same raw source
string was flowing through `MarkerMetadata` -> `MatchedDefinition::yaml_path`
-> `print_matched_summary` (which writes to stdout). When the CLI
is invoked from inside an ADO pipeline step, the agent's stdout
scanner would still pick up an attacker-controlled `##vso[...]`
payload. New `sanitize_for_vso_logging` helper in the discovery
module applies the same convention (`##vso[` -> `[vso-filtered][`,
`##[` -> `[filtered][`) when building the `yaml_path`. New unit test
asserts the sanitisation.
- `ADO_AW_PREVIEW_CONCURRENCY=0` now emits a `warn!` before clamping
to 1, instead of silently masking the typo. Operators who set `=0`
will see the warning and can correct the env value rather than
wondering why their concurrency tuning had no effect.
- New unit test for the `--source` + no-git-remote bail in
`secrets::resolve_for_command`: previously the helpful "no Azure
DevOps git remote was detected; try --all-repos" error path was
untested. Now asserted via a `tokio::test` that constructs an empty
AdoContext and verifies the error message contains both the cause
and the suggested mitigation.
All 1753 tests pass; clippy clean on touched files.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* refactor(secrets): consolidate vso sanitization onto canonical helper (#624)
Round-5 PR review cleanup. No bugs flagged — three maintainability
suggestions, all addressed:
- **F1: reuse `crate::sanitize::neutralize_pipeline_commands`.** The
codebase already has a canonical pipeline-command neutraliser at
`src/sanitize.rs:145` (used by the `SanitizeContent` /
`SanitizeConfig` pipelines for safe outputs and front-matter
sanitisation). The round-4 fix introduced two private
`sanitize_for_vso_logging` helpers — one in
`src/compile/extensions/ado_aw_marker.rs`, one in
`src/ado/discovery.rs` — that hard-coded the
`[vso-filtered][` / `[filtered][` form copied from
`agent_stats::sanitize_for_markdown`. Both private copies removed in
favour of the canonical helper. The canonical helper uses
backtick-wrapping (`` `##vso[` `` / `` `##[` ``) which equally
defeats the ADO agent's stdout scanner; the threat model is
unchanged. Tests in both files updated to assert the canonical
output. The two now-redundant unit tests for the local helpers are
removed; their behavioural coverage already lives in
`src/sanitize.rs:570-605`.
- **F2: two-pass classify-then-filter in
`resolve_definitions_via_discovery`.** The previous shape mutated
four counter variables as a side-effect inside a `.filter()` closure
that *also* decided inclusion — the local `kept` variable then held
items that would be dropped later by `discovered_to_matched`,
misleading any reader. Rewritten as an explicit `for` loop that
classifies + counts in pass 1, emits warnings + converts to
`MatchedDefinition` in pass 2. Renamed `kept` to `selected` to
match what the variable actually holds.
- **F3: comment on the non-`.md` fallthrough in `is_direct_match`.**
Explains that the unreachable-today branch is a forward-compat
measure for hypothetical `.yaml` / `.json` agent sources and that
the conservative behaviour (treat as `Consumer`, not `Direct`)
keeps write commands acting on the definition while labelling it
honestly in the summary.
Net test delta: −2 (the two duplicate sanitizer unit tests). 1751
tests pass; clippy clean on touched files.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(secrets): address fifth-round Rust PR review feedback on #624
Three concrete issues from the latest review pass:
1. `normalize_source_path` double-escaped `"` in the JSON marker.
The helper escaped `"` -> `\"` before the path reached
`serde_json::json!`, which then escaped the backslash again. The
marker stored `"source":"agents/foo\\\"bar.md"` instead of the
canonical `"source":"agents/foo\"bar.md"` — every round-trip
carried a spurious backslash. Move the `"` -> `\"` escape into
`generate_header_comment` (the only YAML-comment surface that
needs it) and leave `normalize_source_path` returning the
canonical form for the JSON marker and the `--source` filter.
2. `is_direct_match` false-positive for same-stem in different
directories. The previous `yaml_normalized.ends_with("/{stem}")`
branch would label an unrelated pipeline `Direct` when its
`yamlFilename` happened to share the same trailing
`<stem>.lock.yml` (e.g. marker `agents/foo.md` matched
`other/agents/foo.lock.yml`). Both sides are already normalised
to repo-root-relative paths without a leading slash, so strict
equality is the correct check. Updated test name and added a
regression test for the same-stem-different-directory case.
3. Documented case-sensitivity of `--source` in CLI help and the
discovery module's doc comment so Windows users don't get silent
zero-result runs from a case mismatch.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* feat(secrets): embed ADO org+repo in marker JSON for disambiguation
When two repos in the same ADO project both define a file of the
same name (e.g. each has its own `agents/foo.md`), today's
`--source agents/foo.md` would silently match consumers of either
template — the marker JSON carried only `source` and operators had
no way to scope writes to "my" agents/foo.md.
Add `org` and `repo` to the marker payload:
* `src/detect.rs` — `MarkerMetadata` gains two `#[serde(default)]`
`String` fields; struct now derives `Default` so test fixtures
can use the `..Default::default()` spread.
* `src/compile/extensions/ado_aw_marker.rs` — pull org from
`ctx.ado_org()` and repo from `ctx.ado_context.repo_name`,
lower-cased (ADO identifiers are case-insensitive). Empty when the
compiler ran outside an ADO checkout; the non-GitHub-remote guard
ensures this is rare in production. Echo line gains
`org=... repo=...` for build-log readability.
* `src/ado/mod.rs` — add `AdoContext::org_name()` accessor that
mirrors `CompileContext::ado_org`; lives on the struct so
non-compile callers (Preview-driven discovery) can reuse it.
* `src/ado/discovery.rs` — when `source_filter` is active,
`resolve_definitions_via_discovery` also requires the marker's
`(org, repo)` to equal the current `ctx`'s. Markers with empty
`org` and `repo` (legacy / non-ADO compiles) match leniently
so already-deployed pipelines remain discoverable until they are
recompiled. Helper `marker_origin_matches` extracted with four
dedicated unit tests covering strict, case-insensitive, lenient,
and half-populated cases.
Schema version stays at 1 — this PR has not shipped, so there are
no v1-without-org/repo markers in the wild that would need the
bump for forward-compat detection. New fields parse fine on either
side via `#[serde(default)]`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(secrets): three correctness fixes on Preview-driven discovery
1. `src/ado/discovery.rs:732` — skip-warning text overclaimed
ado-aw membership.
In `--all-repos` mode the per-status counters
(`skipped_required_params` / `_forbidden` / `_failed`) fired
for every failed Preview, including pipelines that aren't ado-aw
at all. A project with hundreds of non-ado-aw pipelines that
legitimately require `templateParameters` would emit
"Discovery skipped N definitions whose Pipeline Preview requires
templateParameters" when none of them were ado-aw consumers in
the first place. We literally cannot tell which is which without
successful Preview output.
Consolidated the three separate `warn!` messages into one that's
honest about uncertainty ("any ado-aw pipelines among them have
been silently skipped"), with the per-status breakdown demoted to
`debug!` for operators investigating. Counter variables renamed
(`uninspectable_*`) to reflect what they actually count.
2. `src/compile/extensions/mod.rs:130` — `Path::parent()` returns
`Some(Path::new(""))` for bare filenames, not `None`, so the
`unwrap_or(Path::new("."))` fallback never fired for inputs like
`foo.md`. Empty paths passed to `git -C "" remote get-url`
behave differently from `git -C "."`. Match-arm now normalises
both the `None` and empty-`Some` cases to `.`.
3. `src/secrets.rs:202` + `src/ado/discovery.rs` —
`--all-repos --source` could silently exclude every strict
marker when `ctx.org_name()` returned `None` or
`ctx.repo_name` was empty. The existing guard only fired in the
`!all_repos` branch. Extended the guard to fire for any
`--source` invocation when the current `(org, repo)` is
unresolvable, with an actionable error pointing the user to
`--definition-ids` as the escape hatch. New test:
`source_with_all_repos_bails_when_org_url_unresolvable`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* refactor(marker): emit ado-aw marker via prepare_steps, not setup_steps
The always-on `ado-aw-marker` extension was emitting its metadata
step via `setup_steps`, which forced `generate_setup_job` to spin
up a dedicated `- job: Setup` block in every compiled pipeline.
Minimal agentic pipelines with no user `setup:` declarations, no
PR/pipeline filters, and no other Setup-contributing extensions were
paying for a whole extra pool agent + agent boot just to emit a
single-line metadata comment.
Move the marker emission to `prepare_steps` so the step lands
inside the always-present Agent job's `{{ prepare_steps }}` block.
No extra job, no extra pool time — the marker is still parsed by
`parse_marker_step` the same way (scans for the
`# ado-aw-metadata:` line anywhere in the YAML).
The change requires extending the `CompilerExtension::prepare_steps`
signature to accept `&CompileContext` — the marker needs
`input_path`, `ado_context`, and `front_matter.target` to
build its JSON. All five existing `prepare_steps` implementors
(cache_memory, lean, python, node, dotnet) gain an unused
`_ctx: &CompileContext` parameter. The macro dispatcher and
`generate_prepare_steps` propagate ctx through.
Tests:
* All five existing `prepare_steps` unit tests in `extensions/tests.rs`
updated to construct a `CompileContext` via the existing `ctx_from`
helper.
* All seven `generate_prepare_steps` tests in `common.rs` updated to
pass a `CompileContext::for_test(&fm)`.
* All seven marker-emit unit tests in `ado_aw_marker.rs` updated to
call `prepare_steps(&ctx)` (now returns `Vec<String>` directly,
no `Result` wrapper).
* New regression test `test_marker_does_not_create_setup_job_for_minimal_pipeline`
asserts a minimal compiled pipeline contains the marker line but
no `- job: Setup` block — guards against future re-introduction
of the unnecessary Setup job.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(secrets): final-round review fixes on Preview-driven discovery
1. `src/ado/discovery.rs::build_lock_path_map` — was hand-rolling the
`\\` → `/` + leading-slash-strip normalisation while the lookup
side used `super::normalize_ado_yaml_path`. Functionally
equivalent today, but the two would silently diverge if
`normalize_ado_yaml_path` ever gained case-folding or
URL-decoding. Route both sides through the shared helper.
2. `src/ado/discovery.rs::resolve_definitions_via_discovery` — in
`--all-repos` mode (no source filter), uninspectable
definitions (`UnknownRequiredParams` / `UnknownForbidden` /
`PreviewFailed`) and `NotAdoAw` / `NotFound` defs were
ending up in the `selected` vec only to be discarded by
`discovered_to_matched`. In a large project where most
definitions are non-ado-aw, this could allocate a vec of
hundreds of dead entries. Push only actionable statuses
(`Direct` / `Consumer`) into `selected`. Counts are still
tallied before the guard so the warning text is unchanged.
3. `src/compile/extensions/ado_aw_marker.rs` — added a trailing
newline (was failing `cargo fmt --check`).
4. `src/ado/discovery.rs::is_direct_match` — expanded the
`markers.len() > 1` comment to explain *why* multiple markers
imply Consumer: a compiled ado-aw pipeline's expanded YAML carries
exactly one marker (its own Setup-job step); more than one means
the YAML was produced by expanding a consumer that
`template:`-includes multiple ado-aw lock files.
5. `src/secrets.rs` — empty-match hints used to lump all three
flag combinations into one of two messages. Factored an
`empty_match_hint(all_repos, source)` helper that branches on
the four `(bool, Option)` cases, so `--source agents/foo.md`
on its own now points the user at `--all-repos` when nothing
matches in the current repo. All three callers (`run_set` /
`run_list` / `run_delete`) consolidated onto the helper.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* docs: correct stale `Setup-job` wording for the ado-aw-marker hook
The marker extension was originally wired via `setup_steps` but the
`refactor(marker)` commit moved emission to `prepare_steps` so a
metadata-only pipeline doesn't spawn a Setup pool agent. Several doc
comments still describe the marker as a Setup-job step, which is
misleading for anyone tracing the code from a comment.
* `src/compile/extensions/ado_aw_marker.rs` — module-level and
struct-level comments now describe the Agent-job prepare-phase
injection (and explain why `prepare_steps` was chosen over
`setup_steps`).
* `src/detect.rs` — `MARKER_STEP_PREFIX` doc updated to say
"injected Agent-job prepare step" instead of "Setup-job step".
* `src/ado/discovery.rs` — the `markers.len() > 1` rationale in
`is_direct_match` now says "its own Agent-job prepare step".
PR description on #624 also corrected via `gh pr edit` to match.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>1 parent 3a0c9e7 commit 6d4c824
22 files changed
Lines changed: 2520 additions & 101 deletions
File tree
- docs
- src
- ado
- compile
- extensions
- runtimes
- dotnet
- lean
- node
- python
- tools/cache_memory
- tests
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
44 | 44 | | |
45 | 45 | | |
46 | 46 | | |
| 47 | + | |
| 48 | + | |
47 | 49 | | |
48 | 50 | | |
49 | 51 | | |
50 | 52 | | |
| 53 | + | |
51 | 54 | | |
52 | 55 | | |
53 | 56 | | |
54 | 57 | | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
55 | 68 | | |
56 | 69 | | |
57 | 70 | | |
| |||
0 commit comments