You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Give maintainers a simple way to review external PRs without running untrusted code or taking over the author's work. Reviews stay read-only unless the user or a verified maintainer requests follow-up changes. Long Azure builds are watched in the background instead of repeatedly interrupting other work.
Why: these safety checks, contribution boundaries, and CI waiting rules should not need to be re-explained for every PR.
Validation status: all 81 checks pass, including full Azure CI. Current-head Copilot review has no findings. Human approval remains.
flowchart LR
accTitle: External PR review
accDescr: Start with safety review. Changes need authorization, and contributor sign-off follows passing checks.
A["Read-only review"] --> B["Safety and permission checks"]
B -->|No follow-up requested| C["Return findings"]
B -->|Approved| D["Scoped changes and background CI"]
D -->|Checks pass| E["Contributor sign-off"]
B -->|Unresolved risk| F["Stop and report"]
Loading
Recheck safety after edits. Secret-dependent CI needs separate approval; failed or pending checks remain blockers.
Workflow and implementation details
synapseml-external-contributor-review adds a short procedure and a natural thank-you/sign-off message. It reuses synapseml-pr-loop.
External scope is based on verified Osmos group/team membership and the trusted target branch's CODEOWNERS. A fork alone does not make an author external.
Before running external code or approving CI, inspect prompt-injection and credential-exfiltration risks using trusted instructions. If the required guidance is absent from the trusted base or installation, stop before execution. A PR cannot authorize itself.
Editing permission does not authorize CI. The default readiness command is read-only; all trigger paths require explicit CI authorization and the current-head safety check. External PRs use a one-shot trigger followed by read-only waiting.
Keep contributor history and comments intact. Make only requested follow-ups, ask whether the additions fit, and offer to revert them. Resolve addressed review threads only when explicitly requested, after replying with evidence.
The background watcher queries only microsoft/SynapseML and accepts the trusted Azure project's GUID or A365 URL forms. It polls every 10 minutes. Each run's deadline is two hours after its /azp run kickoff; a new run gets a fresh window, but restarting the watcher for the same run does not extend it.
The PR loop now requires outcome-led titles, a clear what/why opening, useful visuals, and technical detail disclosed later.
No Spark runtime, dependency, or pipeline configuration changes.
How is this patch tested?
Local checks and Azure evidence
The monitor's 30 tests cover polling, kickoff deadlines, late starts, same-run restarts, replacement runs, failures, quiet output, repository binding, trusted Azure URL forms, and malformed-input boundaries. Regressions demonstrate the failures before their fixes and pass afterward.
The suite now lives under tools/ci/tests, so the existing CIHelpers job runs it without pipeline changes. On this head, Azure's helper job passed 318 tests and 63 subtests, including the watcher suite.
The Python files passed Black 22.3.0. Documentation metadata, local links, and whitespace were checked.
Read-only live checks verified completed-build failure and replacement-run handoff.
Azure build 237087784 completed successfully at 15:20:49 UTC on September 22, before its original 15:37:36 cutoff. All 66 jobs completed; one retained only non-blocking cache warnings. Test execution and coverage publication passed.
Earlier attempts hit Azure certificate errors. Retrying only the failed translation job completed the build while retaining the other 65 jobs. No TLS checks or pipeline settings were changed, and the same-run retry did not reset the deadline.
The watcher tests had no skips. The runtime suites retain 19 existing Scala ignores and three explicit Python skips. Release compatibility correctly skipped this tooling-only diff.
Current-head Copilot review reports no findings. All 12 review threads are resolved. The reviewed head is up to date with master; only the required human approval remains.
Related Issues/PRs
Based on #2710. Includes the polling work from the closed#2731.
Does this PR change any dependencies?
No.
Does this PR add a new feature? If so, have you added samples on website?
No public library feature; this changes agent workflow guidance and adds a CI watcher.
Hey Rana Singh (@ranadeepsingh) 👋!
Thank you so much for contributing to our repository 🙌.
Someone from SynapseML Team will be reviewing this pull request soon.
We use semantic commit messages to streamline the release process.
Before your pull request can be merged, you should make sure your first commit and PR title start with a semantic prefix.
This helps us to create release messages and credit you for your hard work!
Examples of commit messages with semantic prefixes:
fix: Fix LightGBM crashes with empty partitions
feat: Make HTTP on Spark back-offs configurable
docs: Update Spark Serving usage
build: Add codecov support
perf: improve LightGBM memory usage
refactor: make python code generation rely on classes
style: Remove nulls from CNTKModel
test: Add test coverage for CNTKModel
To test your commit locally, please follow our guild on building from source.
Check out the developer guide for additional guidance on testing your change.
## Summary
Replace the long contributor-review checklist with six practical steps and a
short thank-you message. Reuse synapseml-pr-loop for shared validation.
## Prompting Intent
The engineer asked to keep the skill simple and genuine, preserve all existing
PR comments, and use synapseml-pr-loop instead of duplicating its workflow.
They also asked to include the related polling change in the same PR.
## Linked Sources
- Skill PR: microsoft#2730
- Polling PR to consolidate: microsoft#2731
- Existing workflow: .github/skills/synapseml-pr-loop/SKILL.md
- Contributor example: microsoft#2710 (comment)
## Rationale
Keep only the contributor-specific decisions here: independent triage,
authorization, preserving their work and discussion, and thanking them after
validation with a clear sign-off request and an offer to revert. Leave shared
testing and CI detail in the PR-loop skill.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dd10f95b-f7b6-49cc-9814-ac315de3aa8d
## Summary
Set a 10-minute polling cadence for long-running Azure builds in the
SynapseML PR-loop skill and its CI triage reference.
## Prompting Intent
The engineer requested a separate PR updating synapseml-pr-loop so that,
after /azp run, it checks the pipeline every 10 minutes instead of making
repeated short status checks.
## Linked Sources
- Workflow: .github/skills/synapseml-pr-loop/SKILL.md
- CI guidance: .github/skills/synapseml-pr-loop/references/ci-triage.md
- Readiness helper: .github/skills/synapseml-pr-loop/scripts/Get-PrReadiness.ps1
- Readiness gates: .github/skills/synapseml-pr-loop/references/readiness-gates.md
- Repository guidance: AGENTS.md
- Related skill PR: microsoft#2730
## Rationale
Use explicit 600-second waits for pipeline completion, retaining the initial
queue check and prompt handling of completion notifications. Keep review and
check-registration polling separate because the readiness helper does not
wait for full pipeline completion. Document the cadence rather than changing
an unrelated helper default or adding a new monitoring script. Preserve the
existing success gates and prohibit duplicate builds during status polling.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dd10f95b-f7b6-49cc-9814-ac315de3aa8d
## Summary
Replace the long contributor-review checklist with six practical steps and a
short thank-you message. Reuse synapseml-pr-loop for shared validation.
## Prompting Intent
The engineer asked to keep the skill simple and genuine, preserve all existing
PR comments, and use synapseml-pr-loop instead of duplicating its workflow.
They also asked to include the related polling change in the same PR.
## Linked Sources
- Skill PR: microsoft#2730
- Polling PR to consolidate: microsoft#2731
- Existing workflow: .github/skills/synapseml-pr-loop/SKILL.md
- Contributor example: microsoft#2710 (comment)
## Rationale
Keep only the contributor-specific decisions here: independent triage,
authorization, preserving their work and discussion, and thanking them after
validation with a clear sign-off request and an offer to revert. Leave shared
testing and CI detail in the PR-loop skill.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dd10f95b-f7b6-49cc-9814-ac315de3aa8d
## Summary
Set a 10-minute polling cadence for long-running Azure builds in the
SynapseML PR-loop skill and its CI triage reference.
## Prompting Intent
The engineer requested a separate PR updating synapseml-pr-loop so that,
after /azp run, it checks the pipeline every 10 minutes instead of making
repeated short status checks.
## Linked Sources
- Workflow: .github/skills/synapseml-pr-loop/SKILL.md
- CI guidance: .github/skills/synapseml-pr-loop/references/ci-triage.md
- Readiness helper: .github/skills/synapseml-pr-loop/scripts/Get-PrReadiness.ps1
- Readiness gates: .github/skills/synapseml-pr-loop/references/readiness-gates.md
- Repository guidance: AGENTS.md
- Related skill PR: microsoft#2730
## Rationale
Use explicit 600-second waits for pipeline completion, retaining the initial
queue check and prompt handling of completion notifications. Keep review and
check-registration polling separate because the readiness helper does not
wait for full pipeline completion. Document the cadence rather than changing
an unrelated helper default or adding a new monitoring script. Preserve the
existing success gates and prohibit duplicate builds during status polling.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dd10f95b-f7b6-49cc-9814-ac315de3aa8d
## Summary
Add a read-only Azure check monitor for synapseml-pr-loop and instructions
to launch it as one attached background terminal process.
## Prompting Intent
The engineer asked for token-efficient background monitoring so other work
can continue, retaining 10-minute polling and setting a two-hour maximum.
Keep the contributor skill short and update the existing combined PR.
## Linked Sources
- Current skill PR: microsoft#2730
- Prior polling PR: microsoft#2731
- Existing readiness helper: .github/skills/synapseml-pr-loop/scripts/Get-PrReadiness.ps1
- CI guidance: .github/skills/synapseml-pr-loop/references/ci-triage.md
- Repository guidance: AGENTS.md
## Rationale
Run polling inside one process rather than spending model turns on status
queries. Reuse GitHub CLI authentication and Azure check status already used
by the readiness helper. Bind the monitor to the expected head and build so
it cannot silently follow a different revision or retry. Bound both queries
and sleeps by a monotonic deadline, report non-success explicitly, and never
cancel or retrigger the remote build. Deterministic clock tests cover the
full two-hour boundary without waiting in real time.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dd10f95b-f7b6-49cc-9814-ac315de3aa8d
## Summary
Measure the monitor's two-hour limit from the pipeline kickoff, not process
startup. Hand off replacement builds so each new run gets its own window.
## Prompting Intent
The engineer clarified that the cutoff is two hours after /azp run kickoff,
and that a newly triggered run must reset the timer.
## Linked Sources
- Combined skill PR: microsoft#2730
- Monitoring guidance: .github/skills/synapseml-pr-loop/references/ci-triage.md
- Monitor: .github/skills/synapseml-pr-loop/scripts/watch_azure_pipeline.py
- Observed replacement trigger: microsoft#2730 (comment)
## Rationale
Require an explicit verified kickoff timestamp, then preserve its remaining
budget with a monotonic deadline. Restarting a watcher for the same build
cannot extend that build's allowance. Report new build IDs as a handoff
instead of a query error, allowing the skill to launch one new background
job using that run's own kickoff. Keep polling token-efficient and avoid
guessing which trigger caused a run from the time it was first observed.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dd10f95b-f7b6-49cc-9814-ac315de3aa8d
## Summary
Reject other repositories before querying and bind GitHub requests to the
canonical microsoft/SynapseML repository. Accept both the verified Azure
project GUID and A365 alias on modern and legacy result URLs.
## Prompting Intent
The engineer reported that Copilot feedback was still unresolved. After
requesting a fresh review on the current target, address its repository-binding
and project-alias findings with regressions, then refresh current-head CI.
## Linked Sources
- Repository finding: microsoft#2730 (comment)
- Alias finding: microsoft#2730 (comment)
- Fresh review: microsoft#2730 (review)
- Canonical Azure project links: README.md:16
- Verified Azure project: A365, b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f
- Monitoring guide: .github/skills/synapseml-pr-loop/references/ci-triage.md
## Rationale
This watcher already trusts a repository-specific check name and Azure project,
so allowing arbitrary GitHub repositories was misleading. Keep the existing
flag and default for compatibility, accept case-insensitive spelling of the
canonical repository, and reject unrelated repositories before any query.
Bind the low-level query command too, rather than trusting a mutable namespace.
Azure confirms that A365 names the existing trusted project GUID. Supporting
both path forms preserves legitimate links without allowing another project
or organization. Match the verified project paths case-insensitively.
The new repository and alias cases fail before these changes. Afterward all
28 tests and 53 subtests pass, and the existing CI command discovers all 28
watcher tests among 316 helper tests. No pipeline configuration changes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dd10f95b-f7b6-49cc-9814-ac315de3aa8d
build_ids[0] is only checked with a digit regex, so an untrusted status URL containing thousands of digits passes validation but int(...) can raise Python's digit-limit ValueError. main catches only MonitorError, so the watcher exits without its final JSON/error result instead of handling the malformed check; catch the conversion (or bound the input length) and add an oversized numeric buildId regression.
## Summary
Validate Azure build IDs against the documented positive int32 range before
converting URL values, and enforce the same ceiling on CLI input. Preserve
valid leading-zero forms and boundary IDs. Report UTC normalization overflow
as an argument error, and use explicit UTF-8 decoding with structured failures.
## Prompting Intent
The engineer requested resolution of all Copilot feedback. The fresh review
contains a Previously missed finding about oversized numeric build IDs
escaping as ValueError instead of producing the final monitor result.
Audit adjacent malformed-input boundaries rather than stopping at that line.
## Linked Sources
- Collapsed finding: microsoft#2730 (review)
- Azure buildId schema: https://learn.microsoft.com/en-us/rest/api/azure/devops/build/builds/get?view=azure-devops-rest-7.1
- Watcher: .github/skills/synapseml-pr-loop/scripts/watch_azure_pipeline.py
- Regression suite: tools/ci/tests/test_watch_azure_pipeline.py
## Rationale
Bounding the ID using the service contract avoids depending on a Python
interpreter's configurable integer-string limit. Strip insignificant zeroes
before checking length so existing valid URL forms remain supported.
Oversized values now produce an error instead of a traceback or false
replacement, while IDs 1 and 2147483647 remain valid.
The adjacent parser and transport paths also allowed unhandled exceptions:
UTC conversion could overflow, and locale-dependent subprocess decoding
could raise UnicodeDecodeError. Focused regressions reproduce these failures;
specific error handling now preserves the existing CLI/JSON error contracts.
All 30 tests and 63 subtests pass with Black 22.3.0. Existing CI discovery
collects the full watcher suite among 318 tests without configuration changes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dd10f95b-f7b6-49cc-9814-ac315de3aa8d
Addressed the Previously missed oversized-buildId finding from review 5277511930 in c52af0b. Azure documents buildId as int32, so URL IDs are bounded before integer conversion and CLI IDs use the same positive range. A 10,000-digit value now emits a final JSON error instead of raising ValueError; valid minimum/maximum IDs and leading-zero URLs remain supported. The adjacent input audit also reproduced and fixed UTC-normalization overflow and invalid UTF-8 decoding. All 30 tests and 63 subtests pass; existing CI discovers the suite among 318 tests. No pipeline configuration changed.
When the watcher starts after the kickoff-based deadline (or a query reaches the deadline), this timeout result is returned before timeout_result["url"] is populated. The final JSON therefore omits the build link, contradicting references/ci-triage.md's requirement to report the link and leaving operators unable to identify the unresolved run; initialize the result with a canonical URL derived from the validated build ID, then overwrite it with the observed URL when available.
## Summary
Make missing CI a blocker rather than implicit permission to trigger a build
in every guidance entry point. Keep external-PR triggering separate from
read-only waiting. Include a canonical build link in every timeout result,
without issuing a query after the kickoff deadline.
## Prompting Intent
The engineer requested exhaustive resolution of Copilot feedback. The latest
review identified an authorization contradiction in readiness-gates.md and
a Previously missed timeout path that omitted the build URL.
## Linked Sources
- Authorization finding: microsoft#2730 (comment)
- Timeout-link finding: microsoft#2730 (review)
- Readiness evidence: .github/skills/synapseml-pr-loop/references/readiness-gates.md
- CI waiting guidance: .github/skills/synapseml-pr-loop/references/ci-triage.md
- Watcher regression suite: tools/ci/tests/test_watch_azure_pipeline.py
## Rationale
A readiness requirement is not execution authorization. The main workflow,
readiness gates, and CI triage reference now state the same prerequisite.
For external PRs, trigger once after checking the exact head, then wait
read-only so a later head change cannot receive implicit approval.
Construct the fallback timeout link from the validated build ID and trusted
project GUID. Retain the observed URL when a query supplies one. Three
regressions fail before the fix for an expired start, a response at the
deadline, and a query failure at the deadline; all pass afterward.
The legacy observed-URL case also passes without extending the time budget.
All 30 tests and 63 subtests pass. Black 22.3.0, documentation link checks,
instruction-size checks, and whitespace checks pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dd10f95b-f7b6-49cc-9814-ac315de3aa8d
Also addressed the timeout-link finding in the Previously missed section of review 5277762393, in 3f18281. An already-expired start, a response arriving at the deadline, and a query failure at the deadline now return the canonical build link. Those three cases failed before the fix and pass afterward. A valid observed legacy link still takes precedence, and no extra query or extended deadline is needed. All 30 watcher tests and 63 subtests pass; the existing CIHelpers command discovers them. Full Azure CI is still pending for the new head.
Azure build 237079561 failed before the lightgbm1 test script started. The Azure CLI task could not obtain its OIDC token because msdata.visualstudio.com returned a certificate for *.azureedge.net. The task retried and hit the same hostname mismatch; coverage publishing then failed because no test report existed. Logs: https://dev.azure.com/msdata/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=237079561&view=logs&j=320c32d5-b58a-5840-324d-2225935ebd3a&t=c607b1b2-631f-5341-7f6d-66ee47fd4e15 . CIHelpers passed all 318 tests and 63 subtests. The only other job warnings were sbt-cache HTTP timeouts; those jobs completed their tests and Azure coverage publication. I am retrying full CI on the unchanged, reviewed head 3f18281. No pipeline settings or TLS checks were changed.
The full retry completed the LightGBM shard successfully. Its only failed job was translate: the unit-test and test-result publication steps passed, and six coverage files existed, but Azure coverage publication failed with RemoteCertificateNameMismatch. This is another Azure endpoint/TLS failure, not a test assertion. I requested one failed-job retry within build 237087784, retaining the successful jobs rather than starting another full pipeline. The original 13:37:36 UTC kickoff and 15:37:36 UTC monitoring deadline are unchanged. No pipeline settings or TLS validation were modified.
The review and CI follow-up is complete on 3f18281. All 12 review threads are resolved, and the complete current-head Copilot review contains no outstanding findings, including collapsed feedback. All 81 checks are green. Azure build 237087784 succeeded: https://dev.azure.com/msdata/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=237087784 . The retried translation job passed its tests, test-result publication, and coverage publication. The only remaining job warnings were non-blocking cache failures. The build finished before its original two-hour deadline; no TLS checks or pipeline settings were changed.
The PR is up to date with master, and the local, fork, and GitHub heads match. The earlier Copilot overview predated CI completion; its CI-pending note is now superseded by this result. Only the required human approval remains. I have not merged the PR.
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
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.
What changes are proposed in this pull request?
Give maintainers a simple way to review external PRs without running untrusted code or taking over the author's work. Reviews stay read-only unless the user or a verified maintainer requests follow-up changes. Long Azure builds are watched in the background instead of repeatedly interrupting other work.
Why: these safety checks, contribution boundaries, and CI waiting rules should not need to be re-explained for every PR.
Validation status: all 81 checks pass, including full Azure CI. Current-head Copilot review has no findings. Human approval remains.
flowchart LR accTitle: External PR review accDescr: Start with safety review. Changes need authorization, and contributor sign-off follows passing checks. A["Read-only review"] --> B["Safety and permission checks"] B -->|No follow-up requested| C["Return findings"] B -->|Approved| D["Scoped changes and background CI"] D -->|Checks pass| E["Contributor sign-off"] B -->|Unresolved risk| F["Stop and report"]Recheck safety after edits. Secret-dependent CI needs separate approval; failed or pending checks remain blockers.
Workflow and implementation details
synapseml-external-contributor-reviewadds a short procedure and a natural thank-you/sign-off message. It reusessynapseml-pr-loop.CODEOWNERS. A fork alone does not make an author external.microsoft/SynapseMLand accepts the trusted Azure project's GUID orA365URL forms. It polls every 10 minutes. Each run's deadline is two hours after its/azp runkickoff; a new run gets a fresh window, but restarting the watcher for the same run does not extend it.No Spark runtime, dependency, or pipeline configuration changes.
How is this patch tested?
Local checks and Azure evidence
tools/ci/tests, so the existingCIHelpersjob runs it without pipeline changes. On this head, Azure's helper job passed 318 tests and 63 subtests, including the watcher suite.237087784completed successfully at 15:20:49 UTC on September 22, before its original 15:37:36 cutoff. All 66 jobs completed; one retained only non-blocking cache warnings. Test execution and coverage publication passed.master; only the required human approval remains.Related Issues/PRs
Based on #2710. Includes the polling work from the closed #2731.
Does this PR change any dependencies?
Does this PR add a new feature? If so, have you added samples on website?