fix(ci) Fix shell-tool-mcp.yml

.github/workflows/shell-tool-mcp.yml

@@ -408,9 +408,8 @@ jobs:
         shell: bash
         run: |
           set -euo pipefail
-          git clone --depth 1 https://git.code.sf.net/p/zsh/code /tmp/zsh
+          git clone https://git.code.sf.net/p/zsh/code /tmp/zsh
           cd /tmp/zsh
-          git fetch --depth 1 origin 77045ef899e53b9598bebc5a41db93a548a40ca6
           git checkout 77045ef899e53b9598bebc5a41db93a548a40ca6
           git apply "${GITHUB_WORKSPACE}/shell-tool-mcp/patches/zsh-exec-wrapper.patch"
           ./Util/preconfig
@@ -487,9 +486,8 @@ jobs:
         shell: bash
         run: |
           set -euo pipefail
-          git clone --depth 1 https://git.code.sf.net/p/zsh/code /tmp/zsh
+          git clone https://git.code.sf.net/p/zsh/code /tmp/zsh
           cd /tmp/zsh
-          git fetch --depth 1 origin 77045ef899e53b9598bebc5a41db93a548a40ca6
           git checkout 77045ef899e53b9598bebc5a41db93a548a40ca6
           git apply "${GITHUB_WORKSPACE}/shell-tool-mcp/patches/zsh-exec-wrapper.patch"
           ./Util/preconfig

codex-rs/core/src/exec_policy.rs

@@ -170,23 +170,26 @@ impl ExecPolicyManager {
             prefix_rule,
         } = req;
         let exec_policy = self.current();
-        let (commands, used_heredoc_fallback) = commands_for_exec_policy(command);
+        let (commands, used_complex_parsing) = commands_for_exec_policy(command);
         // Keep heredoc prefix parsing for rule evaluation so existing
         // allow/prompt/forbidden rules still apply, but avoid auto-derived
         // amendments when only the heredoc fallback parser matched.
-        let auto_amendment_allowed = !used_heredoc_fallback;
+        let auto_amendment_allowed = !used_complex_parsing;
         let exec_policy_fallback = |cmd: &[String]| {
             render_decision_for_unmatched_command(
                 approval_policy,
                 sandbox_policy,
                 cmd,
                 sandbox_permissions,
+                used_complex_parsing,
             )
         };
         let evaluation = exec_policy.check_multiple(commands.iter(), &exec_policy_fallback);
 
-        let requested_amendment =
-            derive_requested_execpolicy_amendment(prefix_rule.as_ref(), &evaluation.matched_rules);
+        let requested_amendment = derive_requested_execpolicy_amendment_from_prefix_rule(
+            prefix_rule.as_ref(),
+            &evaluation.matched_rules,
+        );
 
         match evaluation.decision {
             Decision::Forbidden => ExecApprovalRequirement::Forbidden {
@@ -406,8 +409,9 @@ pub fn render_decision_for_unmatched_command(
     sandbox_policy: &SandboxPolicy,
     command: &[String],
     sandbox_permissions: SandboxPermissions,
+    used_complex_parsing: bool,
 ) -> Decision {
-    if is_known_safe_command(command) {
+    if is_known_safe_command(command) && !used_complex_parsing {
         return Decision::Allow;
     }
 
@@ -534,7 +538,7 @@ fn try_derive_execpolicy_amendment_for_allow_rules(
         })
 }
 
-fn derive_requested_execpolicy_amendment(
+fn derive_requested_execpolicy_amendment_from_prefix_rule(
     prefix_rule: Option<&Vec<String>>,
     matched_rules: &[RuleMatch],
 ) -> Option<ExecPolicyAmendment> {
@@ -552,10 +556,8 @@ fn derive_requested_execpolicy_amendment(
         return None;
     }
 
-    if matched_rules
-        .iter()
-        .any(|rule_match| is_policy_match(rule_match) && rule_match.decision() == Decision::Prompt)
-    {
+    // if any policy rule already matches, don't suggest an additional rule that might conflict or not apply
+    if matched_rules.iter().any(is_policy_match) {
         return None;
     }
 
@@ -1564,14 +1566,17 @@ prefix_rule(
 
     #[test]
     fn derive_requested_execpolicy_amendment_returns_none_for_missing_prefix_rule() {
-        assert_eq!(None, derive_requested_execpolicy_amendment(None, &[]));
+        assert_eq!(
+            None,
+            derive_requested_execpolicy_amendment_from_prefix_rule(None, &[])
+        );
     }
 
     #[test]
     fn derive_requested_execpolicy_amendment_returns_none_for_empty_prefix_rule() {
         assert_eq!(
             None,
-            derive_requested_execpolicy_amendment(Some(&Vec::new()), &[])
+            derive_requested_execpolicy_amendment_from_prefix_rule(Some(&Vec::new()), &[])
         );
     }
 
@@ -1579,7 +1584,7 @@ prefix_rule(
     fn derive_requested_execpolicy_amendment_returns_none_for_exact_banned_prefix_rule() {
         assert_eq!(
             None,
-            derive_requested_execpolicy_amendment(
+            derive_requested_execpolicy_amendment_from_prefix_rule(
                 Some(&vec!["python".to_string(), "-c".to_string()]),
                 &[],
             )
@@ -1598,7 +1603,7 @@ prefix_rule(
         ] {
             assert_eq!(
                 None,
-                derive_requested_execpolicy_amendment(Some(&prefix_rule), &[])
+                derive_requested_execpolicy_amendment_from_prefix_rule(Some(&prefix_rule), &[])
             );
         }
     }
@@ -1624,7 +1629,7 @@ prefix_rule(
         ] {
             assert_eq!(
                 None,
-                derive_requested_execpolicy_amendment(Some(&prefix_rule), &[])
+                derive_requested_execpolicy_amendment_from_prefix_rule(Some(&prefix_rule), &[])
             );
         }
     }
@@ -1639,22 +1644,52 @@ prefix_rule(
 
         assert_eq!(
             Some(ExecPolicyAmendment::new(prefix_rule.clone())),
-            derive_requested_execpolicy_amendment(Some(&prefix_rule), &[])
+            derive_requested_execpolicy_amendment_from_prefix_rule(Some(&prefix_rule), &[])
         );
     }
 
     #[test]
-    fn derive_requested_execpolicy_amendment_returns_none_when_policy_prompt_matches() {
+    fn derive_requested_execpolicy_amendment_returns_none_when_policy_matches() {
         let prefix_rule = vec!["cargo".to_string(), "build".to_string()];
-        let matched_rules = vec![RuleMatch::PrefixRuleMatch {
+
+        let matched_rules_prompt = vec![RuleMatch::PrefixRuleMatch {
             matched_prefix: vec!["cargo".to_string()],
             decision: Decision::Prompt,
             justification: None,
         }];
-
         assert_eq!(
             None,
-            derive_requested_execpolicy_amendment(Some(&prefix_rule), &matched_rules)
+            derive_requested_execpolicy_amendment_from_prefix_rule(
+                Some(&prefix_rule),
+                &matched_rules_prompt
+            ),
+            "should return none when prompt policy matches"
+        );
+        let matched_rules_allow = vec![RuleMatch::PrefixRuleMatch {
+            matched_prefix: vec!["cargo".to_string()],
+            decision: Decision::Allow,
+            justification: None,
+        }];
+        assert_eq!(
+            None,
+            derive_requested_execpolicy_amendment_from_prefix_rule(
+                Some(&prefix_rule),
+                &matched_rules_allow
+            ),
+            "should return none when prompt policy matches"
+        );
+        let matched_rules_forbidden = vec![RuleMatch::PrefixRuleMatch {
+            matched_prefix: vec!["cargo".to_string()],
+            decision: Decision::Forbidden,
+            justification: None,
+        }];
+        assert_eq!(
+            None,
+            derive_requested_execpolicy_amendment_from_prefix_rule(
+                Some(&prefix_rule),
+                &matched_rules_forbidden
+            ),
+            "should return none when prompt policy matches"
         );
     }
 

codex-rs/core/tests/suite/approvals.rs

@@ -1479,6 +1479,44 @@ fn scenarios() -> Vec<ScenarioSpec> {
                 output_contains: "rejected by user",
             },
         },
+        ScenarioSpec {
+            name: "safe command with heredoc and redirect still requires approval",
+            approval_policy: AskForApproval::OnRequest,
+            sandbox_policy: workspace_write(false),
+            action: ActionKind::RunUnifiedExecCommand {
+                command: "cat <<'EOF' > /tmp/out.txt \nhello\nEOF",
+                justification: None,
+            },
+            sandbox_permissions: SandboxPermissions::RequireEscalated,
+            features: vec![Feature::UnifiedExec],
+            model_override: None,
+            outcome: Outcome::ExecApproval {
+                decision: ReviewDecision::Denied,
+                expected_reason: None,
+            },
+            expectation: Expectation::CommandFailure {
+                output_contains: "rejected by user",
+            },
+        },
+        ScenarioSpec {
+            name: "compound command with one safe command still requires approval",
+            approval_policy: AskForApproval::OnRequest,
+            sandbox_policy: workspace_write(false),
+            action: ActionKind::RunUnifiedExecCommand {
+                command: "cat ./one.txt && touch ./two.txt",
+                justification: None,
+            },
+            sandbox_permissions: SandboxPermissions::RequireEscalated,
+            features: vec![Feature::UnifiedExec],
+            model_override: None,
+            outcome: Outcome::ExecApproval {
+                decision: ReviewDecision::Denied,
+                expected_reason: None,
+            },
+            expectation: Expectation::CommandFailure {
+                output_contains: "rejected by user",
+            },
+        },
     ]
 }
 
@@ -1890,14 +1928,15 @@ async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts
     Ok(())
 }
 
+// todo(dylan) add ScenarioSpec support for rules
 #[tokio::test(flavor = "current_thread")]
 #[cfg(unix)]
-async fn heredoc_with_chained_allowed_prefix_still_requires_approval() -> Result<()> {
+async fn compound_command_with_one_safe_command_still_requires_approval() -> Result<()> {
     skip_if_no_network!(Ok(()));
 
     let server = start_mock_server().await;
     let approval_policy = AskForApproval::UnlessTrusted;
-    let sandbox_policy = SandboxPolicy::new_read_only_policy();
+    let sandbox_policy = SandboxPolicy::new_workspace_write_policy();
     let sandbox_policy_for_config = sandbox_policy.clone();
     let mut builder = test_codex().with_config(move |config| {
         config.permissions.approval_policy = Constrained::allow_any(approval_policy);
@@ -1913,12 +1952,12 @@ async fn heredoc_with_chained_allowed_prefix_still_requires_approval() -> Result
     )?;
 
     let call_id = "heredoc-with-chained-prefix";
-    let command = "cat <<'EOF' > /tmp/test.txt && touch allow-prefix.txt\nhello\nEOF";
+    let command = "touch ./test.txt && rm ./test.txt";
     let (event, expected_command) = ActionKind::RunCommand { command }
         .prepare(&test, &server, call_id, SandboxPermissions::UseDefault)
         .await?;
     let expected_command =
-        expected_command.expect("heredoc chained command scenario should produce a shell command");
+        expected_command.expect("compound command should produce a shell command");
 
     let _ = mount_sse_once(
         &server,
@@ -1940,7 +1979,7 @@ async fn heredoc_with_chained_allowed_prefix_still_requires_approval() -> Result
 
     submit_turn(
         &test,
-        "heredoc chained prefix",
+        "compound command",
         approval_policy,
         sandbox_policy.clone(),
     )

codex-rs/shell-command/src/bash.rs

@@ -128,9 +128,6 @@ pub fn parse_shell_lc_single_command_prefix(command: &[String]) -> Option<Vec<St
     if root.has_error() {
         return None;
     }
-    if !has_named_descendant_kind(root, "heredoc_redirect") {
-        return None;
-    }
 
     let command_node = find_single_command_node(root)?;
     parse_heredoc_command_words(command_node, script)
@@ -237,20 +234,6 @@ fn is_literal_word_or_number(node: Node<'_>) -> bool {
     node.named_children(&mut cursor).next().is_none()
 }
 
-fn has_named_descendant_kind(node: Node<'_>, kind: &str) -> bool {
-    let mut stack = vec![node];
-    while let Some(current) = stack.pop() {
-        if current.kind() == kind {
-            return true;
-        }
-        let mut cursor = current.walk();
-        for child in current.named_children(&mut cursor) {
-            stack.push(child);
-        }
-    }
-    false
-}
-
 fn is_allowed_heredoc_attachment_kind(kind: &str) -> bool {
     matches!(
         kind,
@@ -532,7 +515,10 @@ mod tests {
             "-lc".to_string(),
             "echo hello > /tmp/out.txt".to_string(),
         ];
-        assert_eq!(parse_shell_lc_single_command_prefix(&command), None);
+        assert_eq!(
+            parse_shell_lc_single_command_prefix(&command),
+            Some(vec!["echo".to_string(), "hello".to_string()])
+        );
     }
 
     #[test]
@@ -548,6 +534,16 @@ mod tests {
         );
     }
 
+    #[test]
+    fn parse_shell_lc_single_command_prefix_rejects_herestring_with_chaining() {
+        let command = vec![
+            "bash".to_string(),
+            "-lc".to_string(),
+            r#"echo hello > /tmp/out.txt && cat /tmp/out.txt"#.to_string(),
+        ];
+        assert_eq!(parse_shell_lc_single_command_prefix(&command), None);
+    }
+
     #[test]
     fn parse_shell_lc_single_command_prefix_rejects_herestring_with_substitution() {
         let command = vec![