Remove cloud-hypervisor security review warning - #60304
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The unrelated and untested cache-mode schema expansion should be removed or split into a separate change.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Removes the mandatory security-review warning for the experimental cloud-hypervisor runtime.
Changes:
- Removes warning emission and counting.
- Updates regression coverage.
- Adds unrelated
cache-modeschema support.
File summaries
| File | Description |
|---|---|
pkg/workflow/compiler_validators.go |
Removes the warning logic. |
pkg/workflow/compiler_validators_test.go |
Verifies no warning is emitted or counted. |
pkg/workflow/schemas/github-workflow.json |
Adds unrelated cache-mode validation. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| "cacheMode": { | ||
| "$comment": "https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#cache-mode", | ||
| "description": "Controls the level of GitHub Actions cache access granted to a workflow or job.", | ||
| "type": "string", | ||
| "enum": ["read", "write", "write-only", "none"] |
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
No ADR enforcement needed: PR does not have the implementation label and has <=100 new lines of code in business logic directories (default_business_additions=20).
|
|
✅ Ponytail Reviewer completed successfully! Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design. The core change (removing the cloud-hypervisor security-review warning while preserving preview behavior) is small, correctly implemented, and its test is properly updated to assert the warning is gone with a zero warning count. No regression risk there.
📋 Key Themes & Highlights
Key Themes
- Scope creep:
pkg/workflow/schemas/github-workflow.jsongains a newcacheMode/cache-modedefinition unrelated to the PR's stated purpose. It isn't referenced by any Go code, docs, or tests in this repo, so it looks like unrelated schema drift bundled into this change (see inline comment). Recommend splitting into its own PR or dropping it here.
Positive Highlights
- ✅
isCloudHypervisorRuntimeand the--cloud-hypervisor-previewCLI flag path are untouched, so runtime support and preview status are correctly preserved as described in the PR body. - ✅ Test rename (
...ReviewTrigger→...DoesNotWarn) and updated assertions (NotContains+Zerowarning count) accurately reflect the new behavior — no stale test expectations left behind. - ✅ Doc comment on
emitSandboxRuntimeWarningswas updated to match the reduced scope of the function.
@copilot please address the review comments above.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 33.2 AIC · ⌖ 14.9 AIC · ⊞ 10.4K
Comment /matt to run again
| "$ref": "#/definitions/globs", | ||
| "description": "When using the push and pull_request events, you can configure a workflow to run on specific branches or tags. If you only define only tags or only branches, the workflow won't run for events affecting the undefined Git ref.\nThe branches, branches-ignore, tags, and tags-ignore keywords accept glob patterns that use the * and ** wildcard characters to match more than one branch or tag name. For more information, see https://help.github.com/en/github/automating-your-workflow-with-github-actions/workflow-syntax-for-github-actions#filter-pattern-cheat-sheet.\nThe patterns defined in branches and tags are evaluated against the Git ref's name. For example, defining the pattern mona/octocat in branches will match the refs/heads/mona/octocat Git ref. The pattern releases/** will match the refs/heads/releases/10 Git ref.\nYou can use two types of filters to prevent a workflow from running on pushes and pull requests to tags and branches:\n- branches or branches-ignore - You cannot use both the branches and branches-ignore filters for the same event in a workflow. Use the branches filter when you need to filter branches for positive matches and exclude branches. Use the branches-ignore filter when you only need to exclude branch names.\n- tags or tags-ignore - You cannot use both the tags and tags-ignore filters for the same event in a workflow. Use the tags filter when you need to filter tags for positive matches and exclude tags. Use the tags-ignore filter when you only need to exclude tag names.\nYou can exclude tags and branches using the ! character. The order that you define patterns matters.\n- A matching negative pattern (prefixed with !) after a positive match will exclude the Git ref.\n- A matching positive pattern after a negative match will include the Git ref again." | ||
| }, | ||
| "cacheMode": { |
There was a problem hiding this comment.
[/codebase-design] This cacheMode/cache-mode schema addition is unrelated to the stated purpose of this PR (removing the cloud-hypervisor security review warning) and isn't referenced anywhere else in the codebase (no Go code, docs, or tests use cache-mode).
💡 Why this matters
Bundling an unrelated schema change into a warning-removal PR makes the diff harder to review and muddies the change history — a reviewer approving "remove KVM warning" is also implicitly approving a new, unused validation surface for GitHub Actions' cache-mode field. If this addition is intentional (e.g. keeping the embedded schema in sync with upstream), it should be split into its own PR with a clear description and, ideally, a test exercising the new field. Otherwise, drop it from this PR.
@copilot please address this.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Removes the mandatory human security review warning for
sandbox.agent.runtime: cloud-hypervisorwhile retaining its experimental preview status.--cloud-hypervisor-previewbehavior.