mirror of
https://github.com/openai/codex.git
synced 2026-05-15 08:42:34 +00:00
address hook rewrite review feedback
This commit is contained in:
@@ -354,11 +354,7 @@ impl ToolEmitter {
|
||||
(event, result)
|
||||
}
|
||||
Err(ToolError::UpdatedInput(updated_input)) => {
|
||||
let event = ToolEventStage::Failure(ToolEventFailure::Message(
|
||||
"tool input rewritten by hook".to_string(),
|
||||
));
|
||||
let result = Err(FunctionCallError::UpdatedInput(updated_input));
|
||||
(event, result)
|
||||
return Err(FunctionCallError::UpdatedInput(updated_input));
|
||||
}
|
||||
};
|
||||
self.emit(ctx, event).await;
|
||||
|
||||
@@ -476,6 +476,16 @@ impl NetworkApprovalService {
|
||||
PermissionRequestDecision::Allow {
|
||||
updated_input: Some(_),
|
||||
} => {
|
||||
if let Some(owner_call) = owner_call.as_ref() {
|
||||
self.record_call_outcome(
|
||||
&owner_call.registration_id,
|
||||
NetworkApprovalOutcome::DeniedByPolicy(
|
||||
"updatedInput is not supported for network approval requests"
|
||||
.to_string(),
|
||||
),
|
||||
)
|
||||
.await;
|
||||
}
|
||||
pending.set_decision(PendingApprovalDecision::Deny).await;
|
||||
let mut pending_approvals = self.pending_host_approvals.lock().await;
|
||||
pending_approvals.remove(&key);
|
||||
|
||||
@@ -397,7 +397,7 @@ impl ToolOrchestrator {
|
||||
if evaluate_permission_request_hooks
|
||||
&& let Some(permission_request) = tool.permission_request_payload(req)
|
||||
{
|
||||
let permission_request_tool_input = permission_request.tool_input.clone();
|
||||
let permission_request_for_noop_check = permission_request.clone();
|
||||
match run_permission_request_hooks(
|
||||
approval_ctx.session,
|
||||
approval_ctx.turn,
|
||||
@@ -409,7 +409,7 @@ impl ToolOrchestrator {
|
||||
Some(PermissionRequestDecision::Allow {
|
||||
updated_input: Some(updated_input),
|
||||
}) => {
|
||||
if updated_input != permission_request_tool_input {
|
||||
if !permission_request_for_noop_check.updated_input_is_noop(&updated_input) {
|
||||
return Err(ToolError::UpdatedInput(updated_input));
|
||||
}
|
||||
let decision = ReviewDecision::Approved;
|
||||
|
||||
@@ -154,6 +154,14 @@ impl PermissionRequestPayload {
|
||||
tool_input: serde_json::Value::Object(tool_input),
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn updated_input_is_noop(&self, updated_input: &serde_json::Value) -> bool {
|
||||
if self.tool_name.name() == "Bash" {
|
||||
return self.tool_input.get("command") == updated_input.get("command");
|
||||
}
|
||||
|
||||
self.tool_input == *updated_input
|
||||
}
|
||||
}
|
||||
|
||||
// Specifies what tool orchestrator should do with a given tool call.
|
||||
|
||||
@@ -34,6 +34,30 @@ fn bash_permission_request_payload_includes_description_when_present() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bash_updated_input_noop_ignores_description() {
|
||||
let payload = PermissionRequestPayload::bash(
|
||||
"echo hi".to_string(),
|
||||
Some("network-access example.com".to_string()),
|
||||
);
|
||||
|
||||
assert!(payload.updated_input_is_noop(&json!({ "command": "echo hi" })));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn non_bash_updated_input_noop_requires_full_equality() {
|
||||
let payload = PermissionRequestPayload {
|
||||
tool_name: HookToolName::apply_patch(),
|
||||
tool_input: json!({ "command": "patch" }),
|
||||
};
|
||||
|
||||
assert!(payload.updated_input_is_noop(&json!({ "command": "patch" })));
|
||||
assert!(!payload.updated_input_is_noop(&json!({
|
||||
"command": "patch",
|
||||
"description": "ignored"
|
||||
})));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn external_sandbox_skips_exec_approval_on_request() {
|
||||
let sandbox_policy = SandboxPolicy::ExternalSandbox {
|
||||
|
||||
Reference in New Issue
Block a user