feat(compile)!: add on.push trigger and make on: the complete run declaration - #1786
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
/review |
|
✅ Test Quality Sentinel completed the test quality analysis. No test review performed: the pre-fetched PR diff, metadata, and review comment files (/tmp/gh-aw/agent/pr-diff.patch, pr-meta.json, pr-review-comments.json) were all empty for PR #1786, so there was no diff content available to assess test quality against. |
Not wired into the compiler yet - types only, pending design review. Motivation: ADO treats a MISSING top-level trigger: key as 'CI on every branch'. Today on.pr.mode: synthetic (the default) relies on that implicit default, and there is no way to express 'never start on a push' - omitting on: entirely selects the implicit CI trigger rather than disabling it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 46384967-1028-4498-b60c-0583971aadcb
…laration
`on:` now fully declares when a pipeline runs. Azure DevOps reads a missing
top-level `trigger:` key as "run CI on every branch" rather than "no CI", so
the compiler now always emits both `trigger:` and `pr:` explicitly.
Adds `on.push`, a sibling of `on.schedule` / `on.pr` / `on.pipeline`, mapping
onto ADO's `trigger:` key:
on:
push: none # never start on a push
on:
push: # start only on matching pushes
branches:
include: [main]
paths:
include: ["src/**"]
An explicit `on.push` always wins, including over the all-branches trigger
that `on.pr`'s default synthetic mode emits as its delivery mechanism and the
`none` that a schedule or `mode: policy` would otherwise produce. It controls
only `trigger:`; the `pr:` half stays driven by `on.pr`.
BREAKING CHANGE: a workflow with no `on:` key now compiles to a manual /
API-queued-only pipeline instead of inheriting ADO's implicit "CI on every
branch" default. Codemod 0006_explicit_push_trigger migrates affected sources
automatically by pinning the legacy behaviour as an explicit `on.push`.
Because the shape being migrated is an *absence* — and that same absence is
the valid new spelling of "manual-only" — the codemod cannot rely on
detection alone. `CodemodContext` gains `source_compiler_version`, read by the
caller from the source's committed `.lock.yml` header, so the codemod fires
only for sources that predate the change and self-retires afterwards. It is
additionally gated on the running binary having reached the cutover release,
which keeps `compile` and `check` consistent: a pre-cutover binary stamps its
own version into the lock file it writes, which the provenance gate alone
would read back as "old" on the very next `check`.
The cutover constant is 0.49.0: v0.48.0 shipped the old semantics, so sources
carrying that version are exactly the ones needing migration.
Also promotes `semver` to a direct dependency behind a new `crate::version`
module and folds in the three hand-rolled `split('.')` version parsers, which
mis-ordered pre-releases.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6a85562f-fb62-4fe3-a18b-df0a1b951348
fce3057 to
457755f
Compare
|
/review |
|
✅ Rust Code Quality Reviewer completed the Rust code quality review. |
There was a problem hiding this comment.
Security review
Reviewed this diff against main for regressions in the injection, validation, token-scope, and network-boundary areas.
No findings. This PR is security-neutral for those concerns:
- The new
on.pushbranch/path filter strings are validated withreject_pipeline_injectioninvalidate_front_matter_identity, mirroring the existingon.prchecks — covered by dedicated tests for newline injection and$(...)ADO expressions in each of the four new fields. PushTriggerConfigis a closed, deserialize-validated enum (Disabled/Filtered); no new raw-Stringfields bypasssrc/secure.rsnewtypes.- The new
0006_explicit_push_triggercodemod andsrc/version.rssemver comparisons fail closed on unparseable/missing version strings, so a malformed or absent lock-file header can never be mistaken for "needs migration" or vice versa. - No changes to
src/compile/ado_bundle.rstoken projection,src/allowed_hosts.rs/src/ecosystem_domains.rsnetwork allowlists, or safe-output write paths. lower_ci_triggervalues are serialized viaserde_yaml, not string-concatenated intobash:/##vso[...]sinks.
No merge-blocking issues found.
🔒 Security review by PR Security Reviewer · aut00 · 32.7 AIC · ⌖ 3.79 AIC · ⊞ 11.2K
Comment /review to run again
There was a problem hiding this comment.
Test Quality Sentinel review
Reviewed the test changes for the on.push trigger feature (codemod, compiler, IR, validation).
Overall: strong test coverage, no blocking issues.
Highlights of what's done well:
build_triggersgets a full truth-table of unit tests (build_triggers_*) covering every combination ofon.push/on.pr/on.schedule/on.pipeline, including the "explicit push wins" precedence rules, not just the happy path.- The new
0006_explicit_push_triggercodemod has dedicated unit tests for both gates (running-binary cutover and source-provenance), idempotency, pre-release version ordering, and thepush: noneverbatim-preservation case — plus integration tests (codemod_integration_test.rs) exercising the real registry end-to-end, including the important regression testcompile_then_reparse_reports_no_pending_migrationthat guards the exact "just-compiled source looks stale" trap the doc comments warn about. - Injection validation for the new
on.push.branches/on.push.pathsfields is tested for all four fields (include/exclude × branches/paths) plus a valid-input and apush: noneno-op case — good defense-in-depth coverage that isn't just implied by other tests. src/version.rshas solid edge-case coverage: pre-release ordering, unparseable-input fail-closed behavior, and lexical-vs-numeric ordering pitfalls.- No weakened or removed assertions found anywhere in the diff.
Minor observations (non-blocking):
tests/compiler_tests.rs's new fixture-based tests (test_push_trigger_emits_native_branch_and_path_filters, etc.) are a reasonable end-to-end complement to thebuild_triggers_*unit tests rather than duplication — they also verify serialization to YAML, which the unit tests don't.- I did not find a test where
on.pushfilters combine withon.pr.mode: policyand a schedule simultaneously (three-way precedence), but the pairwise tests already present give good confidence in the precedence logic, so this is a nice-to-have rather than a gap.
No test-quality concerns rise to merge-blocking; commenting only.
🧪 Test quality analysis by Test Quality Sentinel · aut00 · 63.1 AIC · ⌖ 3.87 AIC · ⊞ 10.7K
Comment /review to run again
There was a problem hiding this comment.
Reviewed the on.push trigger contract changes. The compiler/codemod/type/doc/test surfaces are consistently wired: PushTriggerConfig is threaded through types.rs, agentic_pipeline.rs trigger-building, common.rs injection validation, and ir/lower.rs; the new 0006_explicit_push_trigger codemod is registered in codemods/mod.rs's CODEMODS array and gated correctly on both the running-binary and source-provenance versions via the new src/version.rs; parse_markdown_detailed_for_source is properly threaded from all three source-provenance-aware call sites (check_pipeline, build_pipeline_ir, the integration test); docs/codemods.md and docs/front-matter.md are updated; and semver is added to Cargo.toml. No bundle/codegen/lock drift found in the changed-files list.
One documentation-sync gap: the new src/version.rs module isn't listed in AGENTS.md's architecture tree (inline comment).
🏗️ Compiler contract review by Compiler Contract Reviewer · aut00 · 62.4 AIC · ⌖ 3.87 AIC · ⊞ 11.9K
Comment /review to run again
There was a problem hiding this comment.
Rust code quality review
No merge-blocking Rust engineering defects found in the diff scope (src/**, tests/**, Cargo.toml).
Summary of what I checked:
src/version.rs(new): clean semver wrapper, fail-closed on unparsable input, well-tested including pre-release ordering.src/compile/codemods/0006_explicit_push_trigger.rs(new): dual-gate provenance logic (running-binary version + source.lock.ymlversion) is well-reasoned and thoroughly tested, including idempotency and dormancy-before-cutover cases.src/compile/agentic_pipeline.rs/ir/mod.rs/ir/lower.rs:CiTrigger/PrTriggerrefactor with constructor helpers (disabled(),all_branches()) is a clean improvement over inline struct literals; defensivetrigger: {}guard inlower_ci_triggeris correct and tested.src/compile/common.rs: newon.pushinjection-sweep validation mirrors the existingon.prpattern; test coverage for newline/ADO-expression injection in branch/path filters is thorough.- Threaded
source_compiler_version: Option<String>throughparse_markdown_detailed_with_registry/CodemodContextcleanly; call sites (compile,check,build_pipeline_ir) correctly source it from the existing compiled output, andenable.rscorrectly documents why it doesn't need to. - No
unwrap()/expect()on user-input paths introduced; error handling usesanyhow::Contextconsistently; no lossy casts, no blocking-in-async, no HashMap-ordering nondeterminism introduced. cargo build,cargo test(full suite) andcargo fmt --checkon the changed files all pass cleanly — the onlycargo fmtdiffs are in unrelated pre-existing files, not touched by this PR.
Nice work — the version-comparison consolidation into src/version.rs (replacing three ad-hoc parsers) and the codemod's source-provenance gating are good structural improvements.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
spsprodeus21.vssps.visualstudio.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "spsprodeus21.vssps.visualstudio.com"
See Network Configuration for more information.
🦀 Rust code quality review by Rust Code Quality Reviewer · aut00 · 118.3 AIC · ⌖ 3.86 AIC · ⊞ 11K
Comment /review to run again
…rebase `main` landed `0006_explicit_push_trigger` (#1786) while this branch was open, so both claimed slot 0006. The registry is append-only and main's codemod merged first, so this branch's codemod moves to 0007: 0006_promote_debug_create_github_issue.rs -> 0007_promote_debug_create_github_issue.rs Registration, `CODEMODS` ordering, `docs/codemods.md` and the AGENTS.md tree are updated to match, and the codemod's test context now supplies the `source_compiler_version` field that `CodemodContext` gained in #1786. This codemod migrates a renamed key rather than a changed default, so it stays unconditional and ignores source provenance. Also adds the missing `0006_explicit_push_trigger.rs` entry to the AGENTS.md codemod tree — main introduced the file without listing it, and that tree is meant to itemize every codemod. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9cc16936-10fe-4069-9a37-0d2d1335b6e5
* feat(safeoutputs): add GitHub issue outputs
Promote GitHub issue creation to a public safe output, add native issue type updates with temporary-ID linkage, and isolate PAT/App credentials to Stage 3.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 48973d5e-a00b-43bc-9ef8-433f66ca12af
* fix(safeoutputs): address PR review feedback
Accept quoted numeric GitHub issue numbers with clear deserialization and report target configuration errors before duplicate temporary-ID errors.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 48973d5e-a00b-43bc-9ef8-433f66ca12af
* fix(safeoutputs): address follow-up review feedback
Quote GitHub API URLs in generated YAML, reject URL fragments, make temporary-ID registration failures operator-facing, and document intentional clear semantics.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 48973d5e-a00b-43bc-9ef8-433f66ca12af
* refactor(safeoutputs): rename GitHub issue outputs to create-github-issue
Rename the agent-facing safe outputs create-issue -> create-github-issue and
set-issue-type -> set-github-issue-type so agents cannot confuse GitHub issues
with the Azure DevOps create-work-item surface at runtime. Renames the MCP tool
names, front-matter keys, Rust modules/types, catalog entries, approval-summary
labels, and docs. The 0006 codemod still detects the legacy
ado-aw-debug.create-issue key and now migrates it to
safe-outputs.create-github-issue.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7b23603c-c5a9-4f16-ab27-31a7bcebdf28
* fix(safeoutputs): complete ADO_AW_GITHUB_TOKEN rename in ops tooling
The `create-issue` → `create-github-issue` promotion moved the smoke
failure reporter onto the default executor token variable,
`ADO_AW_GITHUB_TOKEN` (`DEFAULT_SAFE_OUTPUTS_GITHUB_TOKEN_VAR`), and
`tests/safe-outputs/REGISTERED.md` was updated to match. Three call
sites were left on the old `ADO_AW_DEBUG_GITHUB_TOKEN` name:
* `scripts/rotate-agentplayground-secrets.ps1` still provisioned
`ADO_AW_DEBUG_GITHUB_TOKEN` on the reporter definitions. The next
rotation would have written the PAT to a variable the pipeline no
longer reads, silently breaking `smoke-failure-reporter` (2549)
Stage 3 GitHub auth.
* The `executor-e2e` and `trigger-e2e` harnesses fell back to the old
variable when their dedicated `*_E2E_GITHUB_TOKEN` was unset.
Also adds the new `set_github_issue_type.rs` safe output to the
`src/safe_outputs/` tree in AGENTS.md — it was the only module in that
directory missing from the architecture listing.
Remaining `ADO_AW_DEBUG_GITHUB_TOKEN` references are intentional: the
0006 codemod and its tests/docs must know the legacy name to migrate it,
and `tests/safe-outputs/smoke-failure-reporter.lock.yml` is release-owned
and regenerated by the release workflow, not from a dev checkout.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9cc16936-10fe-4069-9a37-0d2d1335b6e5
* test(safeoutputs): cover App-token write rejection and issue-type default-allow
Closes two review-flagged gaps in security-relevant gates that had no
regression protection.
1. `engine.github-app-token.permissions` write rejection. GitHub issue
safe outputs may inherit the engine App credentials, and that token is
also handed to Agent/Detection. A `write` scope there would leak
write-capable GitHub credentials into Stage 1, breaking the PR's core
"isolate credentials to Stage 3" invariant. The sibling empty-permissions
branch was already tested; the write branch was not. Adds the negative
test plus a read-only positive counterpart asserting the token is
accepted and scoped to the configured target.
2. `set-github-issue-type.allowed` default-allow. Verified this is intended
and documented, not a bug: issue types are a closed set defined by the
repository owner, so an empty list is already bounded by configuration
the agent cannot influence. Labels are free-form strings the agent can
invent, hence the default-deny on
`create-github-issue.allowed-labels`. Adds three tests pinning the
documented behaviour: empty list permits any type, a non-empty list is
strictly enforced (asserting no HTTP request is issued), and a match is
case-insensitive but canonicalised to the configured casing.
Both new gates were mutation-checked: disabling the write guard and
flipping the empty-allowed branch to default-deny each fail the
corresponding new test.
Also documents the deliberate allowlist asymmetry in docs/safe-outputs.md
so the next reader does not "fix" it for consistency.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9cc16936-10fe-4069-9a37-0d2d1335b6e5
* fix(safeoutputs): reject App permission key collisions and cover error paths
Addresses three review findings.
1. Rust review — silent permission-key collision. The App-token mint step
normalizes '-' to '_' before serializing `--permissions-json` (the GitHub
API spells permissions with underscores) and collects into a BTreeMap, so
`pull-requests` and `pull_requests` collapse onto one entry. The source map
is itself a BTreeMap, so '_' sorts after '-' and deterministically wins:
an author writing both spellings could have an intended `read` silently
replaced by `write`. `GithubAppTokenConfig::validate_for` now rejects any
two keys that normalize to the same identifier, so this fails at compile
time with a clear message instead of dropping a declared permission. A
single dashed spelling remains valid, so existing front matter is
unaffected.
2. Test-quality review — two untested reachable error paths in
`resolve_target_repo`: a GitHub Enterprise pipeline source still pointing
at `api.github.com`, and a GitHub-backed build where ADO did not surface
`BUILD_REPOSITORY_NAME`. Adds both negative tests plus a GHE positive
counterpart.
3. Compiler-contract review — advisory to move `target-repo` to a
`src/secure.rs` newtype. Deliberately NOT applied: `get_tool_config`
deserializes with `.ok().unwrap_or_default()`, so a value rejected at
deserialization time would collapse the whole config to `Default`
(`target_repo: None`), which `resolve_target_repo` then resolves to the
*current* repository. The newtype would therefore convert a loud failure
into a silent redirect to the wrong repo — the opposite of the intent.
`target-repo` is already validated at compile time
(`validate_github_issue_outputs_config`) and re-validated at every Stage 3
call site. Adds a regression test pinning the no-silent-redirect
behaviour, with the rationale recorded on the test.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9cc16936-10fe-4069-9a37-0d2d1335b6e5
* fix(codemods): renumber GitHub issue promotion codemod to 0007 after rebase
`main` landed `0006_explicit_push_trigger` (#1786) while this branch was
open, so both claimed slot 0006. The registry is append-only and main's
codemod merged first, so this branch's codemod moves to 0007:
0006_promote_debug_create_github_issue.rs
-> 0007_promote_debug_create_github_issue.rs
Registration, `CODEMODS` ordering, `docs/codemods.md` and the AGENTS.md
tree are updated to match, and the codemod's test context now supplies
the `source_compiler_version` field that `CodemodContext` gained in
#1786. This codemod migrates a renamed key rather than a changed default,
so it stays unconditional and ignores source provenance.
Also adds the missing `0006_explicit_push_trigger.rs` entry to the
AGENTS.md codemod tree — main introduced the file without listing it, and
that tree is meant to itemize every codemod.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9cc16936-10fe-4069-9a37-0d2d1335b6e5
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 48973d5e-a00b-43bc-9ef8-433f66ca12af
Copilot-Session: 7b23603c-c5a9-4f16-ab27-31a7bcebdf28
Copilot-Session: 9cc16936-10fe-4069-9a37-0d2d1335b6e5
Summary
on:is now the complete declaration of when a pipeline runs. Azure DevOps reads a missing top-leveltrigger:key as "run CI on every branch" rather than "no CI", so the compiler now always emits bothtrigger:andpr:explicitly.This adds
on.push— a sibling ofon.schedule/on.pr/on.pipeline— mapping onto ADO'strigger:key. It closes a gap where there was no way to say "never start this pipeline on a push": omittingon:selected the implicit CI trigger rather than disabling it.What gets emitted
trigger:on:at allnone— manual / API-queued onlyon.scheduleoron.pipelineonlynoneon.pr(defaultmode: synthetic)include: ['*'])on.pr.mode: policynoneon.push: nonenoneon.push: {branches, paths}An explicit
on.pushalways wins — including over the all-branches trigger thatmode: syntheticemits as its delivery mechanism, and over thenonethat a schedule ormode: policywould otherwise produce. "Run nightly, and also whenevermainmoves" is a legitimate shape.on.pushcontrols onlytrigger:; thepr:half stays driven byon.pr.The all-branches trigger under
mode: syntheticis deliberate and load-bearing: synthPr resolves the open PR forBuild.SourceBranchat runtime, so it needs CI-triggered builds to react to. It is not auto-narrowed —pr.branches.includelists PR target branches, buttrigger:fires on pushes to the listed branches.Breaking change
A workflow with no
on:key now compiles to a manual / API-queued-only pipeline instead of inheriting ADO's implicit "CI on every branch" default.0006_explicit_push_triggermigrates affected sources automatically, pinning the legacy behaviour as an expliciton.push. Because the shape being migrated is an absence — and that same absence is the valid new spelling of "manual-only" — detection alone is insufficient: a permanent codemod would make manual-only pipelines unreachable, rewriting every newly authoredon:-less workflow.So
CodemodContextgainssource_compiler_version, read by the caller from the source's committed.lock.ymlheader (codemods stay pure). The codemod fires only for sources that predate the change, and self-retires once everything has been recompiled.It is additionally gated on the running binary having reached the cutover release, mirroring
0002_pool_object_form. That second gate keepscompileandcheckconsistent: a pre-cutover binary stamps its own version into the lock file it writes, which the provenance gate alone would read back as "old" on the very nextcheck— reporting a pending migration for a file that had just been compiled. Five existing tests incodemod_tests.rscaught this.The cutover constant is 0.49.0. v0.48.0 (released 2026-07-31) shipped the old semantics, so sources stamped
0.48.0are precisely the ones needing migration, and afeat!bumps 0.48.0 → 0.49.0. If release-please lands this under a different version, updateINTRODUCED_IN— a too-high value leaves the codemod dormant (safe), a too-low one skips the sources that need it.Drive-by
semverwas only a transitive build-dep ofrustc_version. It is now a direct dependency behind a newcrate::versionmodule, folding in the three hand-rolledsplit('.')parsers (update_check.rs, codemod0002, and the new one). Naive numeric triples mis-order pre-releases — semver puts1.0.0-alphabefore1.0.0.Test plan
cargo test— 2779 unit + 221 compiler + 8 codemod tests, 0 failures.cargo clippy --all-targetsclean. Rebased ontomain@0942c61e; the one conflict was a pure append-at-EOF intests/compiler_tests.rsagainst #1785's runtime tests, and both sides are retained.Coverage added for behaviour that previously had none — the earlier silent regression passed the entire suite, which is why the guard below matters most:
build_triggerstruth table (10 tests) — every row above, incl.test_no_on_config_emits_manual_only_pipelineas the regression guard, and each override interaction.lower_ci_triggershapes (3) — filtered / all-branches / never emits invalidtrigger: {}.reject_pipeline_injectionoveron.push.branches.*andon.push.paths.*, pluspush: nonecarrying no free-form strings.job/stagetemplate targets still emit no top-leveltrigger:.compile_then_reparse_reports_no_pending_migrationcovering thecompile→checkbug.crate::version(7) — numeric-vs-lexical ordering (0.9.0<0.48.0), pre-release precedence, fail-closed parsing.Docs updated: new "Push (CI) Triggering (
on.push)" section infront-matter.md(with the correctedon.pr.modetable and auto-narrowing rationale), and a "Source provenance" section incodemods.mdamending invariant 5, which previously stated codemods cannot inspect the lock file.