ci: enable Copilot cloud agent on 1ES - #791
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 937dfad0-bd6e-454f-9046-bd773f4a3347
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 937dfad0-bd6e-454f-9046-bd773f4a3347
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 937dfad0-bd6e-454f-9046-bd773f4a3347
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 937dfad0-bd6e-454f-9046-bd773f4a3347
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 937dfad0-bd6e-454f-9046-bd773f4a3347
Configure the proven 1ES runner for Copilot setup, install Trident's documented toolchain, and validate the unit suite on the same ephemeral pool. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 937dfad0-bd6e-454f-9046-bd773f4a3347
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 937dfad0-bd6e-454f-9046-bd773f4a3347
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 937dfad0-bd6e-454f-9046-bd773f4a3347
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
A critical workflow security issue and two moderate validation issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Enables Copilot cloud agents on ephemeral 1ES runners with required toolchains, Cargo data-disk storage, and KVM/libvirt validation.
Changes:
- Installs native dependencies, Go, Rust, and protobuf.
- Configures Cargo output on the runner data disk.
- Adds Copilot setup, runner smoke tests, and unit-test validation workflows.
File summaries
| File | Description | Final review finding |
|---|---|---|
scripts/cloud-agent/setup.sh |
Provisions dependencies and configures Cargo storage. | Moderate (1 vote): Verify the expected data-disk mount before creating the Cargo target directory. |
scripts/cloud-agent/runner-smoke.sh |
Validates KVM, QEMU, and libvirt. | Moderate (1 vote): Handle nonzero virt-host-validate status instead of reporting PASS. |
.github/workflows/copilot-setup-steps.yml |
Runs Copilot setup on 1ES runners. | Critical (3 votes): Do not execute privileged setup from untrusted pull requests. |
.github/workflows/cloud-agent-runner-smoke.yml |
Runs manual runner and unit-test validation. | No findings. |
.gitattributes |
Defines line-ending rules. | No findings. |
Review details
Suppressed comments (2)
scripts/cloud-agent/runner-smoke.sh:67
- The host validator's exit status is only recorded, so a nonzero
virt-host-validateresult is ignored and the script still reachesresult.txtwithPASS. That allows the manual validation workflow to succeed while libvirt reports a host failure. Either fail on a nonzero status or explicitly treat this command as informational and exclude it from the pass claim.
set +e
sudo virt-host-validate qemu 2>&1 | tee "$log_dir/virt-host-validate.txt"
virt_validate_status=${PIPESTATUS[0]}
set -e
echo "$virt_validate_status" > "$log_dir/virt-host-validate.exit-code"
scripts/cloud-agent/setup.sh:46
- Creating this directory with
mkdir -pdoes not verify that/mnt/trident-cloud-agentis backed by the runner's data-disk mount. If the mount is missing or the pool configuration regresses, Cargo silently falls back to the nearly-full OS disk while setup still succeeds, defeating the purpose ofCARGO_TARGET_DIR. Check the expected mount/source and fail closed before creating the target directory.
sudo install -d -o "$(id -u)" -g "$(id -g)" "$CARGO_TARGET_DIR"
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Prevent fork pull requests from executing privileged setup on the self-hosted runner, require the configured data-disk mount, and fail when virt-host-validate reports an error. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 937dfad0-bd6e-454f-9046-bd773f4a3347
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate issues remain unresolved in setup and nested-virtualization validation.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
scripts/cloud-agent/runner-smoke.sh:56
- This block only records the module parameter and does not validate it: a value such as
N/0, or the missing-parameter branch, still lets the smoke test continue and reportPASS. Because this workflow is intended to validate nested KVM, parse the value and fail unless nested virtualization is enabled (and fail when the parameter cannot be found).
scripts/cloud-agent/setup.sh:31
- Installing
protocalone leaves the Go code-generation path incomplete:make bin/netlaunchrunsgo generate pkg/tridentgrpc/grpc.go, whose generator invokes both--go_outand--go-grpc_out, so it requiresprotoc-gen-goandprotoc-gen-go-grpc. Those plugins are listed in the repository's documented dependencies and installed by the existing build pipeline; without them, a Copilot task that regenerates the Go stubs fails. Install both plugins here and add their Go bin directory to the current and subsequent-stepPATH.
sudo unzip -o "$RUNNER_TEMP/$protoc_archive" -d /usr/local
rm -f "$RUNNER_TEMP/$protoc_archive"
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Install the pinned Go protobuf generators and require nested virtualization to be enabled before the runner smoke succeeds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 937dfad0-bd6e-454f-9046-bd773f4a3347
There was a problem hiding this comment.
🟡 Changes recommended
Critical rustup bootstrap and moderate runner-permission findings remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
scripts/cloud-agent/runner-smoke.sh:48
- These checks use
sudo, and the same is true of the QEMU probe and everyvirshcall below, so this smoke test can pass even when the GitHub runner account cannot access/dev/kvmorqemu:///system. That is the account Copilot and Trident's libvirt tooling use:virtdeployopens libvirt without sudo and the docs require the user to be in thelibvirtgroup. Run the validation as the runner user (using sudo only for installation/service setup), or explicitly provision and assert those permissions.
sudo test -r /dev/kvm
sudo test -w /dev/kvm
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
Require the trusted runner image to provide rustup, grant the job user precise KVM and libvirt ACLs, and run the virtualization probe as that user. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 937dfad0-bd6e-454f-9046-bd773f4a3347
There was a problem hiding this comment.
🔵 Needs a closer look
The smoke-test diagnostic upload can mask the original setup failure when no artifact directory exists.
Review details
Suppressed comments (1)
.github/workflows/cloud-agent-runner-smoke.yml:57
- If checkout or the prerequisite-install step fails, the smoke-test step never creates
$RUNNER_TEMP/trident-cloud-agent-runner-smoke, but this upload still runs because!cancelled()is true for ordinary failures. Withif-no-files-found: error, the diagnostic step then adds a second failure and hides the original setup error; use a non-error missing-files policy or create a marker directory before the earlier steps.
if-no-files-found: error
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 937dfad0-bd6e-454f-9046-bd773f4a3347
There was a problem hiding this comment.
🟡 Changes recommended
Cargo’s configured target directory conflicts with the Makefile’s hard-coded artifact paths.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 937dfad0-bd6e-454f-9046-bd773f4a3347
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the workflow trust boundary and Cargo/Makefile artifact-path mismatch.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
scripts/cloud-agent/setup.sh:67
- Setting a global Cargo
build.target-dirhere redirects outputs to/mnt/storage/..., but the Makefile still declares and consumes artifacts undertarget/(for example,target/release/tridentinMakefile:111-169andMakefile:635-637). As a result, commands such asmake validaterun Cargo into the scratch directory and then execute a missing or staletarget/release/trident; update the Makefile artifact paths or provide a target-dir-compatible mapping before applying this configuration globally.
target-dir = "$CARGO_TARGET_DIR"
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 937dfad0-bd6e-454f-9046-bd773f4a3347
There was a problem hiding this comment.
🔵 Needs a closer look
Resolve the privileged workflow trigger and Cargo/Makefile artifact path issues.
Review details
Suppressed comments (2)
.github/workflows/copilot-setup-steps.yml:10
- This
pushtrigger has no branch restriction, so a collaborator can push a feature branch that changessetup.shand automatically execute it withsudoon the self-hosted 1ES runner. That breaks the stated trusted-default-branch boundary; restrict this trigger to the default branch (or remove the automatic trigger) before running privileged setup from pushed code.
push:
paths:
- .github/workflows/copilot-setup-steps.yml
- scripts/cloud-agent/setup.sh
scripts/cloud-agent/setup.sh:67
- This global Cargo
build.target-dirredirects builds to/mnt/storage/..., but the Makefile still declares and consumes artifacts under the repository'starget/tree (for example,make buildandmake validateusetarget/release/tridentat Makefile:111-169 and 635-637). Those commands will build in the scratch directory and then look for a missing or stale binary, so update the Makefile artifact paths or provide a compatible mapping before applying this config globally.
target-dir = "$CARGO_TARGET_DIR"
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 937dfad0-bd6e-454f-9046-bd773f4a3347
There was a problem hiding this comment.
🔵 Needs a closer look
The changes affect CI infrastructure, self-hosted runner setup, and virtualization validation, warranting final human review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 937dfad0-bd6e-454f-9046-bd773f4a3347
This gives GitHub Copilot coding agents a reproducible Trident environment where they can make changes, build the project, and run unit tests before opening a PR.
Summary
Security boundaries
pull_requesteventscontents: readValidation
97f3fc7; the setup workflow and script are unchanged in the current headrustc 1.93.0,cargo 1.93.0,go1.25.5,libprotoc 33.2,protoc-gen-go v1.36.11, andprotoc-gen-go-grpc 1.6.2/mnt/storageand linked the repository'starget/path to itqemu:///systemmake buildcompleted and produced the expectedtarget/releaseartifactsmake testpassed with 1,057 tests, 0 failures, and 3 ignored testsd73e08aScope
This is the minimum cloud-agent development path. More will follow.