fix(compile): quote pipeline name in generated YAML to handle colons and quotes#568
Merged
Merged
Conversation
Contributor
🔍 Rust PR ReviewSummary: Looks good — correct fix, well-tested, and the security invariants hold. One minor residual exposure worth noting. Findings
|
jamesadevine
force-pushed
the
fix/quote-pipeline-display-name
branch
from
5434f37 to
92c5d4d
Compare
jamesadevine
changed the title
Contributor
🔍 Rust PR ReviewSummary: Solid fix with good test coverage — one stale-documentation issue in the new fixture files. Findings🐛 Bugs / Logic Issues
✅ What Looks Good
|
…and quotes
Front-matter agent names like `Daily safe-output smoke: noop` produced
invalid YAML when substituted bare into the top-level `name:` line:
ADO YAML parsers (and `yaml.safe_load`) saw the second colon as a
mapping indicator and rejected the file with "Mapping values are not
allowed in this context." A sister bug affected hand-quoted
`displayName: "{{ agent_name }}"` lines in 1es-base.yml:37 and
stage-base.yml:6: an agent name containing an embedded `"` would
produce `displayName: "My "special" agent"` — invalid YAML.
Two new template markers split the responsibilities cleanly:
* `{{ pipeline_name }}` — for the top-level pipeline `name:` line
(the ADO build-number format). Emits the agent name plus the
`-$(BuildID)` suffix as a YAML double-quoted scalar. The BuildID
suffix is required: ADO needs a varying token in the build-number
format string, otherwise every run shows the same name in the runs
view.
* `{{ agent_display_name }}` — for `displayName:` positions on
stages and jobs. Emits the agent name as a quoted scalar with NO
BuildID suffix; stage labels are static and shouldn't carry
per-run uniqueness suffixes.
Both go through a new `yaml_double_quoted` helper that escapes `\`,
`"`, `\n`, `\r`, `\t`, and other ASCII control characters. `$(...)`
ADO macros pass through untouched — `$` has no special meaning inside
a YAML double-quoted scalar.
Substitutions in the templates:
* src/data/base.yml — `name: {{ agent_name }}-$(BuildID)`
→ `name: {{ pipeline_name }}`
* src/data/1es-base.yml — `name: {{ agent_name }}-$(BuildID)`
→ `name: {{ pipeline_name }}`
* src/data/1es-base.yml:37 — `displayName: "{{ agent_name }}"`
→ `displayName: {{ agent_display_name }}`
* src/data/stage-base.yml:6 — `displayName: "{{ agent_name }}"`
→ `displayName: {{ agent_display_name }}`
`{{ agent_name }}` is now used only inside src/data/threat-analysis.md
(a markdown body, not parsed as YAML), where bare substitution is safe.
Tests:
* 8 new unit tests for `yaml_double_quoted` cover plain strings, the
original colon bug, backslash / embedded-quote / control-character
escaping, ADO macro passthrough, and unicode.
* 2 new integration tests
(`test_compiled_yaml_survives_tricky_agent_name_{standalone,1es}`)
compile fixtures whose names contain both embedded `"` and `:`,
parse the output via serde_yaml, and assert the escaped forms
appear verbatim in the generated `name:` and `displayName:` lines.
The 1ES test specifically asserts that the stage displayName does
NOT carry the `-$(BuildID)` suffix.
* `test_compiled_yaml_structure` updated to require the new
`{{ pipeline_name }}` marker instead of the old `{{ agent_name }}`
reference on the top-level `name:` line.
* All 26 smoke-suite lock files in tests/safe-outputs regenerated
with properly quoted names.
* docs/template-markers.md documents both new markers and tightens
the `{{ agent_name }}` warning to make clear it's only safe in
non-YAML positions.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jamesadevine
force-pushed
the
fix/quote-pipeline-display-name
branch
from
92c5d4d to
e5685f2
Compare
Contributor
🔍 Rust PR ReviewSummary: Fix is correct and well-structured — two minor issues worth addressing before merge. Findings🐛 Bugs / Logic Issues
|
Collaborator
Author
|
@copilot fix reviewer comments |
Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/163bd94d-faca-4fe4-8ecf-b309a2fb9fd8 Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Contributor
Done in 2a3f120. I fixed the reviewer items by updating the stale fixture marker references to Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
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
Reported by user on #563 after the merge:
Front-matter agent names like
Daily safe-output smoke: noopwere substituted bare into the top-levelname:line via{{ agent_name }}-$(BuildID), producing:ADO YAML parsers (and
yaml.safe_load) read the second colon as a mapping indicator and reject the file. Every smoke fixture in #563 plus any user pipeline whose name contains a colon was affected.The same family of bug applied to
displayName: "{{ agent_name }}"in1es-base.yml:37andstage-base.yml:6— an agent name containing an embedded"would corrupt the hand-wrapped quoted scalar.reject_pipeline_injectiondoesn't currently reject"in name values, so this was reachable from user input.Fix
Two new template markers, each with a clear single responsibility:
{{ pipeline_name }}— top-level pipelinename:line only. Emits the agent name plus the-$(BuildID)suffix as a YAML double-quoted scalar. The BuildID suffix is required: ADO needs a varying token in the build-number format string, otherwise every run shows the same name in the runs view.{{ agent_display_name }}— stage / jobdisplayName:positions. Emits the agent name as a quoted scalar with NO BuildID suffix; stage labels are static and shouldn't carry per-run uniqueness.Both go through a new
yaml_double_quotedhelper that escapes\,",\n,\r,\t, and other ASCII control characters.$(...)ADO macros pass through untouched —$has no special meaning inside a YAML double-quoted scalar.Substitutions in the templates:
Sample expansions for an agent named
My "special": agent:{{ agent_name }}is now used in exactly one place:src/data/threat-analysis.md(a markdown body, not parsed as YAML), where bare substitution is safe.Design notes — why two markers, not one
A previous draft consolidated to a single
{{ pipeline_name }}marker (with BuildID) used in both positions, on the theory that stage labels like"My agent-12345"give a useful visual link to the run. That was wrong: the BuildID suffix only belongs in the build-number format field, where ADO requires a varying token to disambiguate runs. StagedisplayName:is a static label and putting BuildID there is just noise. The two-marker split makes the requirement explicit at every call site.Test plan
yaml_double_quoted: plain strings, the exact colon bug, backslash / embedded-quote / control-char escaping, ADO macro passthrough, and unicode.test_compiled_yaml_survives_tricky_agent_name_{standalone,1es}): compile fixtures whose names contain both embedded"and:, parse the output viaserde_yaml::from_str, and assert the escaped form appears verbatim inname:anddisplayName:lines. The 1ES test specifically asserts the stagedisplayName:does not carry the-$(BuildID)suffix.test_compiled_yaml_structureto require the new{{ pipeline_name }}marker instead of the old{{ agent_name }}reference on thename:line.name:line, e.g.ado-aw check.cargo build— clean.cargo test— all suites pass: 8 yaml_double_quoted + 91 compiler (incl. 2 new tricky-name) + every other suite;bash_lint_testswithENFORCE_BASH_LINT=1and shellcheck on PATH passes.cargo clippy— no net-new warnings on changed files.docs/template-markers.mddocuments both new markers and tightens the{{ agent_name }}warning to make clear it's only safe in non-YAML positions.