chore: secure privileged Azure pipeline triggers - #2692
Closed
Rana Singh (ranadeepsingh) wants to merge 8 commits into
Closed
chore: secure privileged Azure pipeline triggers#2692Rana Singh (ranadeepsingh) wants to merge 8 commits into
Rana Singh (ranadeepsingh) wants to merge 8 commits into
Conversation
## Summary Teach Copilot code review to assess credential-exfiltration risk before maintainers authorize `/azp run`, and harden the trusted PR-readiness helper so stale, ambiguous, attacker-influenced, or unauthorized trigger attempts fail closed. ## Prompting Intent The engineer asked for a follow-up pull request after microsoft#2691 that directs automatic Copilot reviews to decide whether an Azure Pipelines run is safe. The command must remain restricted to SynapseML maintainers because pull-request code can execute where trusted credentials are available. ## Linked Sources - Follow-up to PR microsoft#2691: microsoft#2691 - Post-merge Azure build: https://msdata.visualstudio.com/A365/_build/results?buildId=234418267&view=results - GitHub repository instructions: https://docs.github.com/en/copilot/how-tos/copilot-on-github/customize-copilot/add-custom-instructions/add-repository-instructions - GitHub Copilot code review instructions: https://docs.github.com/en/copilot/how-tos/use-copilot-agents/request-a-code-review/use-code-review - Head-branch instruction behavior: https://github.blog/changelog/2026-07-17-copilot-code-review-customization-and-configurability-improvements/ ## Rationale Use layered, commit-bound checks rather than trusting an AI-authored verdict alone. The helper requires a separately confirmed head SHA from a write-authorized maintainer, rejects unresolved or suppressed findings, fully inventories changed files, blocks PR-controlled Copilot instructions and setup, and rechecks the head immediately before commenting. Polling and triggering are deliberately separate so a waiting process cannot auto-authorize credential-bearing CI. Behavioral tests mock GitHub end to end and prove blocked states never post `/azp run`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 476d113f-dd35-40c6-bc79-005dcccd7b79
Rana Singh (ranadeepsingh)
requested a review
from Brendan Walsh (BrendanWalsh)
as a code owner
September 4, 2026 01:58
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Hey Rana Singh (@ranadeepsingh) 👋! We use semantic commit messages to streamline the release process. Examples of commit messages with semantic prefixes:
To test your commit locally, please follow our guild on building from source. |
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
September 4, 2026 01:59
View session
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The verdict examples in Copilot instructions can be emitted with Markdown formatting that breaks the readiness script’s exact-line verdict parsing, and one documented helper invocation is missing a required parameter.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
.github/copilot-instructions.md — The readiness helper detects AZP safety by requiring the review body’s last line to exactly match… |
|
.github/skills/synapseml-pr-loop/SKILL.md — This usage example omits the mandatory -PullRequest <number> argument. As written,… |
What changed in this PR
Strengthens the security posture around privileged Azure Pipelines PR triggers by requiring an explicit, current-head safety review verdict and a trusted, maintainer-confirmed readiness helper flow before posting /azp run.
Changes:
- Adds a strict “AZP safety verdict” contract and fail-closed gating logic to
Get-PrReadiness.ps1before it can post/azp run. - Documents the maintainer-only, two-step wait/inspect/confirm workflow across contributor guidance and PR-loop references.
- Introduces contract tests to enforce the new safety-gating behavior and review-input inventory checks.
| File | Description |
|---|---|
tools/ci/tests/test_azp_review_safety.py |
Adds contract tests covering safe/unsafe verdict parsing, fail-closed trigger gating, and review-input file inventory behavior. |
AGENTS.md |
Updates contributor guidance to treat /azp run as privileged and tied to exact-head safety review evidence. |
.github/skills/synapseml-pr-loop/SKILL.md |
Documents a maintainer-only readiness + explicit head-SHA confirmation flow before triggering privileged CI. |
.github/skills/synapseml-pr-loop/scripts/Get-PrReadiness.ps1 |
Implements permission checks, exact-head verdict parsing, file-inventory checks, and an explicit -ConfirmHeadSha requirement for triggering. |
.github/skills/synapseml-pr-loop/references/readiness-gates.md |
Adds readiness gates requiring a current-head safe verdict and blocking review-input changes from automated triggering. |
.github/copilot-instructions.md |
Introduces Copilot review instructions requiring an explicit AZP safety verdict line for exact-head assessment. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
## Summary Emit unformatted AZP safety verdict examples and include the mandatory pull-request number in the trusted trigger command. ## Prompting Intent Address every current-head Copilot finding on PR microsoft#2692 without weakening the fail-closed safety contract or maintainer-only trigger flow. ## Linked Sources - Pull request: microsoft#2692 - Verdict-format finding: microsoft#2692 (comment) - Missing-parameter finding: microsoft#2692 (comment) ## Rationale The parser intentionally accepts only a single exact plain-text final line, so the review prompt now explicitly forbids Markdown delimiters around the verdict. The command example includes all mandatory parameters so maintainers can copy it without receiving a parameter-binding failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 476d113f-dd35-40c6-bc79-005dcccd7b79
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
September 4, 2026 02:06
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It modifies security-critical CI authorization and Copilot instruction bootstrapping, so it needs final independent maintainer review despite the added tests and fail-closed checks.
Review tier: Lite
Findings: None
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
.github/skills/synapseml-pr-loop/SKILL.md — This usage example omits the mandatory -PullRequest <number> argument. As written,… View resolved comment |
|
.github/copilot-instructions.md — The readiness helper detects AZP safety by requiring the review body’s last line to exactly match… View resolved comment |
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
## Summary Treat Copilot's AZP marker as advisory review evidence while keeping explicit unsafe verdicts blocking and making the maintainer's separate exact-SHA confirmation the authorization decision. ## Prompting Intent Adapt PR microsoft#2692 to the observed GitHub Copilot review behavior: the current-head review completed without findings but the standard overview did not include the requested custom final marker. Preserve a fail-closed security boundary without depending on unsupported overview formatting. ## Linked Sources - Pull request review: microsoft#2692 - GitHub custom-instruction limitations: https://docs.github.com/en/copilot/tutorials/customize-code-review#what-not-to-include-in-custom-instructions - GitHub code review customization: https://docs.github.com/en/copilot/how-tos/use-copilot-agents/request-a-code-review/use-code-review#customizing-copilots-reviews-with-custom-instructions ## Rationale A missing AI-formatted marker cannot reliably mean safe or unsafe. The helper now blocks explicit unsafe or ambiguous markers, but a missing marker can proceed only when the exact-head review is complete, every finding is clear, review inputs are trusted, the file inventory is complete, the caller has write permission, and a maintainer separately confirms the exact SHA outside polling. This keeps human authorization authoritative while retaining Copilot's security assessment as defense-in-depth evidence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 476d113f-dd35-40c6-bc79-005dcccd7b79
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
September 4, 2026 02:21
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes a security-critical privileged trigger workflow (/azp run) and explicitly requires independent maintainer verification beyond an automated review.
Review tier: Lite
Findings: None
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/skills/synapseml-pr-loop/scripts/Get-PrReadiness.ps1:410
- Get-PrSnapshot always calls Get-PullRequestFileInventory (REST
pulls/{n}+ paginatedpulls/{n}/files) even when invoked repeatedly under -WaitForReview polling. For large PRs this can generate many API calls per poll and slow down readiness checks or risk rate limiting. Consider caching the file inventory per PR head SHA within a single invocation (similar to viewerTriggerPermission) and only re-fetching when the head SHA changes.
## Summary Cache the fully paginated pull-request file inventory for each PR base/head pair during one readiness invocation. ## Prompting Intent Resolve the current-head Copilot review's suppressed rate-limit concern without weakening detection of changed review instructions or stale heads. ## Linked Sources - Pull request review: microsoft#2692 - GitHub REST pull-request files API: https://docs.github.com/en/rest/pulls/pulls#list-pull-requests-files ## Rationale Changed files are stable only while both the target and source commits remain unchanged. Keying the cache by PR number, base SHA, and head SHA eliminates repeated pagination during review polling while automatically invalidating on either a contributor push or a target-branch advance. A behavioral test forces two polling snapshots and proves the inventory APIs run once. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 476d113f-dd35-40c6-bc79-005dcccd7b79
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
September 4, 2026 02:33
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The current gate allows automated triggering even when the AZP verdict marker is missing from the review body, which conflicts with the new “missing is not safe” guidance and weakens the fail-closed contract.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
.github/skills/synapseml-pr-loop/scripts/Get-PrReadiness.ps1 — -RunPipeline currently blocks only when the AZP verdict is explicitly unsafe or ambiguous,… |
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tools/ci/tests/test_azp_review_safety.py:302
- This test claims to model GitHub omitting the requested summary marker, but it actually removes the verdict line from the review body (
FAKE_REVIEW_BODYhas no marker). Since the readiness script reads the full review body via GraphQL, an overview-rendering omission should not affectazpSafetyVerdict; this test therefore enforces allowing truly missing verdicts, which appears to contradict the new "missing is not safe" guidance.
.github/skills/synapseml-pr-loop/scripts/Get-PrReadiness.ps1:683
completeness.completetreatsazpSafetyVerdict == "missing"as passing (-notin @("unsafe","ambiguous")). Given the new requirement that a missing marker is not safe, this summary flag can report a head as complete even though the safety verdict is absent, which is likely to mislead readiness checks. Consider requiringazpSafetyVerdict -eq "safe"forcompleteso missing verdicts fail closed.
complete = (
@($truncatedThreadComments).Count -eq 0 -and
$fileInventory.complete -and
@($reviewInfluenceChanges).Count -eq 0 -and
$automatedReviewCoversHead -and
$azpSafetyVerdict -notin @("unsafe", "ambiguous") -and
@($unresolved).Count -eq 0 -and
## Summary Require the trusted Azure Pipelines trigger helper and completeness summary to accept only the exact current-head safe verdict. Align the maintainer workflow documentation and mocked trigger behavior with that fail-closed rule. ## Prompting Intent The engineer asked for Copilot review to determine whether /azp run is safe before maintainers authorize credential-bearing Azure Pipelines. The current-head review found that a missing machine-readable verdict could still pass the trusted helper, so this change closes that authorization gap without allowing maintainer SHA confirmation to substitute for safety evidence. ## Linked Sources - Copilot review: microsoft#2692 (review) - High-severity finding: microsoft#2692 (comment) - Follow-up to PR microsoft#2691: microsoft#2691 ## Rationale Requiring the exact safe marker is simpler and safer than treating an absent marker as implicit approval. The separate maintainer SHA attestation remains necessary but cannot establish facts the review did not emit; instruction-changing bootstrap PRs continue to require independent review and manual authorization. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 476d113f-dd35-40c6-bc79-005dcccd7b79
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
September 4, 2026 03:07
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes a security-critical privileged CI trigger gate and introduces bootstrap review-instruction mechanics that require maintainer verification.
Review tier: Lite
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
.github/skills/synapseml-pr-loop/scripts/Get-PrReadiness.ps1 — -RunPipeline currently blocks only when the AZP verdict is explicitly unsafe or ambiguous,… View resolved comment |
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
## Summary Replace the unsupported machine-parsed Copilot overview verdict with repository instructions and a review-focused privileged-pipeline checklist. Preserve the trusted maintainer gate for current-head review coverage, unresolved and suppressed findings, complete changed-file inventory, write permission, explicit SHA confirmation, and a final head recheck. ## Prompting Intent The engineer asked the automatic GitHub Copilot pull-request reviewer to assess whether running /azp run is safe, while keeping that credential-bearing Azure pipeline trigger restricted to SynapseML maintainers. After observing that the original fixed-verdict mechanism did not direct the reviewer reliably, correct the design using supported GitHub customization surfaces. ## Linked Sources - Pull request and review feedback: microsoft#2692 - GitHub repository custom-instructions documentation: https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot - GitHub Copilot code-review customization documentation: https://docs.github.com/en/copilot/customizing-copilot/customizing-copilot-code-review ## Rationale GitHub supports repository instructions and review skills but documents their application as non-deterministic and does not support controlling the pull-request overview format. Copilot is therefore advisory: unsafe or uncertain code should produce an actionable finding, while only a write-authorized maintainer can inspect the evidence, attest the exact head, and trigger Azure. This avoids treating generated prose as an authorization token while retaining fail-closed deterministic gates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 476d113f-dd35-40c6-bc79-005dcccd7b79
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
September 4, 2026 04:53
View session
Collaborator
Author
|
The original machine-readable The actual automatic-review customization is now visible in two supported locations:
GitHub documents both mechanisms, but also says custom review instructions are non-deterministic and cannot control the PR overview format: https://docs.github.com/en/copilot/tutorials/customize-code-review. Copilot is therefore advisory, not the authority that emits or authorizes The Copilot review of this exact head now explicitly classifies the privileged-CI gating change as Needs a closer look, requires final human verification, and reports no inline findings. |
## Summary Replace mutable pull-request file inventory with validated, content-addressed commit and Git-tree comparisons. Recheck both PR refs after evidence collection and remove all scripted /azp run posting so the helper cannot claim an unqualified comment is atomically bound to the reviewed head. ## Prompting Intent The engineer asked GitHub Copilot's automatic reviewer to assess whether privileged Azure Pipelines execution is safe while keeping /azp run restricted to SynapseML maintainers. Correct the PR after review showed that generated verdict text and a check-then-comment trigger could not provide deterministic exact-head authorization. ## Linked Sources - Pull request and review discussion: microsoft#2692 - GitHub Copilot code-review customization: https://docs.github.com/en/copilot/tutorials/customize-code-review - Azure Pipelines GitHub comment triggers: https://learn.microsoft.com/en-us/azure/devops/pipelines/repos/github?view=azure-devops#comment-triggers ## Rationale The PR-files endpoint is mutable and can race with force-pushes even when cached under SHA labels. Content-addressed commit and tree objects make the inventory reproducible and fail closed on truncation or malformed responses. Because GitHub offers no conditional comment API and /azp run has no SHA argument, removing trigger automation is safer than narrowing but misrepresenting the race; adversarial concurrent pushes require a trusted exact-commit control plane outside PR-controlled code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 476d113f-dd35-40c6-bc79-005dcccd7b79
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
September 4, 2026 05:43
View session
Collaborator
Author
|
Security follow-up to the earlier design note: independent review found two high-risk TOCTOU gaps before this head was sent to Azure Pipelines.
Both are resolved in
The focused contract suite has 29 passing tests, the immutable-tree logic succeeded against this live PR, and the independent security re-review returned SAFE. The remaining platform limitation is explicit: |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Get-PrReadiness.ps1 uses Sort-Object -CaseSensitive, which can fail on Windows PowerShell 5.1 and breaks the readiness helper in a supported environment.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
.github/skills/synapseml-pr-loop/scripts/Get-PrReadiness.ps1 — Sort-Object -CaseSensitive is not available in Windows PowerShell 5.1, but this helper is… |
## Summary Remove the PowerShell 7-only CaseSensitive switch from presentation ordering while retaining ordinal case-sensitive dictionaries and sets for path identity. Add a contract assertion preventing the incompatible switch from returning. ## Prompting Intent Address the exact-head Copilot finding that the readiness helper must run under both PowerShell 7 and Windows PowerShell 5.1 without weakening immutable changed-path comparison. ## Linked Sources - Copilot review finding: microsoft#2692 (review) - Pull request: microsoft#2692 ## Rationale Ordering is not part of the security decision: ordinal Dictionary and HashSet instances already preserve distinct case-sensitive paths before every path is checked. Removing the unsupported display sort keeps that identity guarantee and restores Windows PowerShell 5.1 compatibility without adding version-specific branches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 476d113f-dd35-40c6-bc79-005dcccd7b79
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
September 4, 2026 05:52
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes security-critical CI authorization/readiness behavior around privileged credentials and should receive final maintainer security review before approval.
Review tier: Lite
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
.github/skills/synapseml-pr-loop/scripts/Get-PrReadiness.ps1 — Sort-Object -CaseSensitive is not available in Windows PowerShell 5.1, but this helper is… View resolved comment |
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2692 +/- ##
=======================================
Coverage 87.41% 87.41%
=======================================
Files 341 341
Lines 20742 20742
Branches 2166 2166
=======================================
Hits 18131 18131
Misses 2611 2611 🚀 New features to boost your workflow:
|
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.
Related Issues/PRs
Follow-up to #2691.
What changes are proposed in this pull request?
code-reviewskill to direct every automatic Copilot review to inspect theexact head for credential-exfiltration risk. Unsafe or uncertain code must
produce an actionable finding that says
/azp runmust not be authorized.mastercopy ofGet-PrReadiness.ps1a read-only evidencecollector. It verifies current-head review coverage, active and suppressed
findings, and changes to head-controlled review inputs, but never posts
/azp run.Reject mismatched, truncated, malformed, or duplicate tree data and recheck
both live PR refs after collecting evidence.
/azp runcannot be atomically bound to a reviewed commit.GitHub's Copilot code review customization
documentation
describes custom instructions as non-deterministic and explicitly says they
cannot control the pull-request overview format. This PR therefore does not
treat Copilot text as a machine authorization token. The trusted helper reports
whether current-head review evidence is complete, but a maintainer must inspect
the review and diff. Because Copilot reads review inputs from the PR head, a PR
changing those inputs requires an independent maintainer security review.
The exact-head Copilot
review
ran at the Lite review tier, explicitly classified this as security-critical CI
authorization/readiness behavior, and left no unresolved findings. GitHub does
not support requiring an affirmative safety verdict; repository administrators
can select the Balanced review tier for deeper security-sensitive analysis.
GitHub provides no conditional comment operation, and
/azp runcontains nocommit SHA. A check followed by that comment is therefore subject to a
check/comment race. The helper no longer automates the command or claims it is
SHA-bound. An adversarial author capable of pushing concurrently requires a
trusted control plane that pins the reviewed commit before credentials are
exposed.
How is this patch tested?
The current implementation passes 29 mocked behavioral and contract tests on
both PowerShell 7 and Windows PowerShell 5.1, PowerShell syntax parsing, pinned
Black, and a live immutable-tree snapshot of this PR. Independent security
re-review found no remaining helper-side vulnerability;
/azp run'splatform-level lack of SHA binding is documented as a residual limitation.
Azure Pipelines build
234450872
completed successfully. It tested merge commit
7d49f7b7b2e0ba8533069b3e927a2b97f351045f, whose ordered parents are base25815741901ab8d9062c51eb3a7f06070550e607and exact PR head74e1cc5e92fd83fece45a7d2a80008c37cf516d9. Three infrastructure-only failureswere retried selectively: two Azure certificate-name mismatches after their
tests passed and one hosted-agent disconnect during setup. All retry attempts
passed.
Does this PR change any dependencies?
Does this PR add a new feature? If so, have you added samples on website?