feat(compile): warn about out-of-date compiled definitions on recompile#638
Open
Copilot wants to merge 6 commits into
Open
feat(compile): warn about out-of-date compiled definitions on recompile#638Copilot wants to merge 6 commits into
Copilot wants to merge 6 commits into
Conversation
…overy
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>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
- `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> Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
- `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>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
- `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> Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
- `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> Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
jamesadevine
May 19, 2026 08:36
View session
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.
Summary
Branched off PR #624 (
devinejames/secret-setting) which added smart pipeline-discovery via embedded# ado-aw-metadata: {…}markers.This PR adds version-staleness checks to the compiler so users immediately see when their compiled lock files were produced by an older version of
ado-aw.Changes (
src/compile/mod.rs)read_existing_pipeline_version(new async helper)Reads the first five lines of an existing compiled pipeline file and extracts the version from the
# @ado-aw source=… version=…header viaparse_header_line. ReturnsNonewhen the file is absent, unreadable, or has no header.compile_pipeline_inner(single-pipeline path)Snapshots the existing lock file's version before overwriting it. After writing the new YAML, if the old version differs from the current
CARGO_PKG_VERSION, prints:This surfaces version bumps in the terminal and in CI logs without requiring the user to diff the output.
compile_all_pipelines(batch auto-discover path)The per-pipeline listing now annotates each entry with its staleness status instead of the raw version number:
Test plan
cargo test: 1634 passed, 0 failedcargo clippy --all-targets --all-features: zero new warningsread_existing_pipeline_version:None@ado-awheader →NoneNone