fix: address code review issues in Tool Safety Guard

1. Fix RESET NameError in _print_summary when --no-color (tool_safety_check.py)
   - RESET was only defined in the color branch, causing crash with --no-color.

2. Fix allow_patterns overriding blocklist DENY (_scanner.py)
   - allow_patterns now only upgrades NEEDS_HUMAN_REVIEW → ALLOW,
     never overrides DENY from risk-level or blocklist decisions.

3. Fix ||/&& false positives in _check_operators (_bash_scanner.py)
   - shlex splits || into two | tokens causing false pipe detection.
   - Similarly && was falsely detected as background operator.
   - Now skips adjacent |/| and &/& to correctly identify shell operators.

4. Update tests to reflect new allow_patterns behavior (test_tool_safety.py)

5. Update DESIGN.md and README.md to document fixed behaviors
   - Decision pipeline: allow_patterns only upgrades NEEDS_HUMAN_REVIEW
   - Oversized scripts: always DENY with blocklist pre-check

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: lll-peanut

examples/tool_safety/DESIGN.md

@@ -128,14 +128,17 @@ Safety Guard 是**执行前治理层**。它对脚本、命令、参数、工作
 └──────────┬───────────┘
            ▼
 ┌──────────────────────┐
-│ 允许模式检查          │ ← 如果 allow_pattern 匹配 → ALLOW（覆盖上述）
+│ 允许模式检查          │ ← 如果 allow_pattern 匹配且当前
+│                      │   为 NEEDS_HUMAN_REVIEW → ALLOW
+│                      │   不会覆盖 DENY（黑名单始终优先）
 └──────────┬───────────┘
            ▼
        最终决策
 ```
 
 **关键约束**：黑名单模式始终升级为 DENY，即使基于风险级别的决策是 ALLOW。
-允许模式可以将非 DENY 决策降回 ALLOW，但不能覆盖来自黑名单的 DENY。
+允许模式仅将 NEEDS_HUMAN_REVIEW 升级为 ALLOW，不会覆盖任何 DENY 决策
+（无论是来自风险级别判定还是黑名单命中）。
 
 ---
 

scripts/tool_safety_check.py

@@ -61,35 +61,41 @@ def main() -> int:
         """,
     )
     parser.add_argument(
-        "--file", "-f",
+        "--file",
+        "-f",
         type=str,
         help="Path to a script file to scan.",
     )
     parser.add_argument(
-        "--type", "-t",
+        "--type",
+        "-t",
         type=str,
         choices=["python", "bash", "auto"],
         default="auto",
         help="Script language hint (default: auto-detect).",
     )
     parser.add_argument(
-        "--tool-name", "-n",
+        "--tool-name",
+        "-n",
         type=str,
         default="cli_tool",
         help="Name of the tool being scanned (for audit / report).",
     )
     parser.add_argument(
-        "--policy", "-p",
+        "--policy",
+        "-p",
         type=str,
         help="Path to a custom safety policy YAML file.",
     )
     parser.add_argument(
-        "--output", "-o",
+        "--output",
+        "-o",
         type=str,
         help="Write the JSON report to this file (default: stdout).",
     )
     parser.add_argument(
-        "--audit", "-a",
+        "--audit",
+        "-a",
         type=str,
         help="Append an audit event to this JSONL file.",
     )
@@ -173,7 +179,7 @@ def main() -> int:
 def _print_summary(report, no_color: bool) -> None:
     """Print a colourised summary to stderr."""
     if no_color:
-        R, G, Y, W, B = "", "", "", "", ""
+        R, G, Y, W, B, RESET = "", "", "", "", "", ""
     else:
         R, G, Y, W, B = "\033[91m", "\033[92m", "\033[93m", "\033[97m", "\033[94m"
         RESET = "\033[0m"
@@ -200,7 +206,11 @@ def _print_summary(report, no_color: bool) -> None:
         print(f"\n{B}  Findings:{RESET}", file=sys.stderr)
         for f in report.findings:
             colour = {
-                "critical": R, "high": R, "medium": Y, "low": W, "info": W,
+                "critical": R,
+                "high": R,
+                "medium": Y,
+                "low": W,
+                "info": W,
             }.get(f.risk_level.value, W)
             print(f"    [{colour}{f.rule_id}{RESET}] {f.message}", file=sys.stderr)
             if f.evidence:

tests/test_tool_safety.py

@@ -1298,8 +1298,8 @@ def test_scanner_allow_patterns_override():
     assert report.decision == Decision.ALLOW
 
 
-def test_scanner_allow_patterns_override_with_dangerous():
-    """allow_patterns must override DENY even for dangerous scripts (when pattern matches)."""
+def test_scanner_allow_patterns_never_overrides_deny():
+    """allow_patterns must NOT override DENY — blocklist always wins (security fix)."""
     from trpc_agent_sdk.tools.safety._policy import SafetyPolicy
     policy = SafetyPolicy(
         allow_patterns=[r"rm\s+-rf\s+/tmp/safedir"],
@@ -1312,8 +1312,9 @@ def test_scanner_allow_patterns_override_with_dangerous():
             script_type=ScriptType.BASH,
             tool_name="allow_dangerous_test",
         ))
-    # allow_patterns should take effect
-    assert report.decision == Decision.ALLOW
+    # allow_patterns must NOT override DENY from CRITICAL risk (rm -rf)
+    assert report.decision == Decision.DENY, \
+        f"allow_patterns should NOT override DENY, got {report.decision}"
 
 
 def test_scanner_reload_policy():
@@ -1541,9 +1542,7 @@ def test_process_medium_risk_for_pipe():
             script_type=ScriptType.BASH,
             tool_name="pipe_test",
         ))
-    proc_findings = [
-        f for f in report.findings if f.category == RiskCategory.PROCESS_AND_SYSTEM
-    ]
+    proc_findings = [f for f in report.findings if f.category == RiskCategory.PROCESS_AND_SYSTEM]
     assert len(proc_findings) > 0, "Pipe should still trigger a PROC finding (INFO level)"
     # Verify it's INFO, not MEDIUM — whitelisted commands → safe pipe
     assert all(f.risk_level == RiskLevel.INFO for f in proc_findings), \
@@ -1556,9 +1555,7 @@ def test_process_medium_risk_for_pipe():
             script_type=ScriptType.BASH,
             tool_name="pipe_test2",
         ))
-    med_findings = [
-        f for f in report2.findings if f.risk_level == RiskLevel.MEDIUM
-    ]
+    med_findings = [f for f in report2.findings if f.risk_level == RiskLevel.MEDIUM]
     assert len(med_findings) > 0, "Pipe with non-whitelisted commands should trigger MEDIUM"
 
 
@@ -2080,16 +2077,18 @@ def test_scanner_blocklist_override_invalid_regex():
 
 
 def test_scanner_allow_override_with_findings():
-    """allow_patterns must override DENY to ALLOW when script triggers rules."""
+    """allow_patterns upgrades NEEDS_HUMAN_REVIEW → ALLOW (never overrides DENY)."""
     import tempfile
     import yaml
-    # Build a temp policy that blocks curl but allows a specific domain pattern
+    # Build a temp policy that allows a specific pattern.
+    # The script triggers MEDIUM risk (long sleep → NEEDS_HUMAN_REVIEW),
+    # and allow_patterns upgrades the decision to ALLOW.
     policy_data = {
         "global": {
             "max_script_lines": 500
         },
         "whitelists": {
-            "domains": ["localhost"],
+            "domains": [],
             "commands": [],
             "patterns": []
         },
@@ -2100,10 +2099,10 @@ def test_scanner_allow_override_with_findings():
             "patterns": []
         },
         "rules": {
-            "network_egress": {
+            "resource_abuse": {
                 "enabled": True,
-                "risk_level": "high",
-                "bash_commands": ["curl", "wget"],
+                "risk_level": "medium",
+                "long_sleep_threshold_seconds": 60,
             }
         },
         "sanitization": {
@@ -2118,16 +2117,16 @@ def test_scanner_allow_override_with_findings():
         from trpc_agent_sdk.tools.safety._policy import PolicyLoader
         policy = PolicyLoader(policy_path).load()
         scanner = SafetyScanner(policy)
-        # Script triggers NET egress (curl to non-whitelisted domain) AND
-        # has an allow_pattern match → should be ALLOW
+        # Script triggers MEDIUM risk (sleep 120 > 60s threshold → NEEDS_HUMAN_REVIEW)
+        # AND has an allow_pattern match → should be ALLOW
         report = scanner.scan(
             SafetyScanInput(
-                script_content="echo safe_override_test; curl https://evil.com",
+                script_content="echo safe_override_test; sleep 120",
                 script_type=ScriptType.BASH,
                 tool_name="allow_override2",
             ))
         assert report.decision == Decision.ALLOW, \
-            f"allow_patterns should override, got {report.decision}"
+            f"allow_patterns should upgrade NEEDS_HUMAN_REVIEW to ALLOW, got {report.decision}"
     finally:
         os.unlink(policy_path)
 

trpc_agent_sdk/tools/safety/README.md

@@ -312,8 +312,8 @@ python scripts/tool_safety_check.py -p custom_policy.yaml -f script.sh
 ```yaml
 # ---- 全局设置 ----
 global:
-  max_script_lines: 500        # 超过此行数触发 needs_human_review
-  max_script_bytes: 524288     # 超过此字节数触发 needs_human_review
+  max_script_lines: 500        # 超过此行数直接 DENY（先做黑名单预检查，防止填充绕过）
+  max_script_bytes: 524288     # 超过此字节数触发 DENY
   max_timeout_seconds: 300     # 建议的最长执行时间
   max_output_bytes: 10485760   # 最大输出大小