-
Notifications
You must be signed in to change notification settings - Fork 539
Harden cloud-hypervisor privileged runtime path and add explicit human-review warning #52757
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c7b3bdb
3f5eeec
8daa546
d98767c
f35c15c
2fc078d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -191,7 +191,6 @@ func TestEmitExperimentalFeatureWarningsGHAWDetection(t *testing.T) { | |
| os.Stderr = w | ||
| t.Cleanup(func() { | ||
| os.Stderr = oldStderr | ||
| _ = w.Close() | ||
| _ = r.Close() | ||
| }) | ||
|
|
||
|
|
@@ -218,6 +217,40 @@ func TestEmitExperimentalFeatureWarningsGHAWDetection(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| func TestEmitGeneralToolWarningsCloudHypervisorReviewTrigger(t *testing.T) { | ||
| compiler := NewCompiler() | ||
| workflowData := &WorkflowData{ | ||
| SandboxConfig: &SandboxConfig{ | ||
| Agent: &AgentSandboxConfig{ | ||
| Runtime: AgentRuntimeCloudHypervisor, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| oldStderr := os.Stderr | ||
| r, w, err := os.Pipe() | ||
| require.NoError(t, err) | ||
| os.Stderr = w | ||
| t.Cleanup(func() { | ||
| os.Stderr = oldStderr | ||
| _ = r.Close() | ||
| }) | ||
|
|
||
| compiler.emitGeneralToolWarnings(workflowData, "test.md") | ||
|
|
||
| require.NoError(t, w.Close()) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnosing-bugs] 💡 FixRemove the explicit var buf bytes.Buffer
t.Cleanup(func() {
_ = w.Close()
_, _ = io.Copy(&buf, r)
_ = r.Close()
os.Stderr = oldStderr
})
compiler.emitGeneralToolWarnings(workflowData, "test.md")
// assertions moved here, but buf must be populated in Cleanup aboveThis mirrors the pattern the adjacent test already uses after its own cleanup fix in this diff. @copilot please address this. |
||
| os.Stderr = oldStderr | ||
|
|
||
| var buf bytes.Buffer | ||
| _, err = io.Copy(&buf, r) | ||
| require.NoError(t, err) | ||
| stderrOutput := buf.String() | ||
|
|
||
| assert.Contains(t, stderrOutput, "sandbox.agent.runtime: cloud-hypervisor uses a privileged KVM preview path") | ||
| assert.Contains(t, stderrOutput, "Require a human security review before merge or rollout") | ||
| assert.Equal(t, 1, compiler.GetWarningCount()) | ||
| } | ||
|
|
||
| // TestValidatePermissions tests permission parsing and MCP tool constraint validation. | ||
| func TestValidatePermissions(t *testing.T) { | ||
| tests := []struct { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/codebase-design]
exit 1insidevalidate_extracted_fileterminates the entire shell script directly rather than returning a non-zero status to the caller. This is intentional here, but it prevents callers from wrapping the function in conditional logic and makes the function harder to unit-test in isolation.💡 Suggestion
If the exit-on-failure behaviour is deliberate (consistent with the surrounding script style), add a brief comment explaining that. If reusability is desired, replace
exit 1withreturn 1and let callers dovalidate_extracted_file "$x" || exit 1.@copilot please address this.