Skip to content

fix: allow_patterns take priority over deny_patterns in ExecTool#3594

Merged
Re-bin merged 6 commits intoHKUDS:mainfrom
chengyongru:fix/allow-patterns-override-deny
May 2, 2026
Merged

fix: allow_patterns take priority over deny_patterns in ExecTool#3594
Re-bin merged 6 commits intoHKUDS:mainfrom
chengyongru:fix/allow-patterns-override-deny

Conversation

@chengyongru
Copy link
Copy Markdown
Collaborator

@chengyongru chengyongru commented May 2, 2026

Problem

  1. allow_patterns cannot override deny_patterns: ExecTool._guard_command() checks deny_patterns before allow_patterns. When a command matches a deny pattern, it is immediately blocked with no way to exempt it via allow_patterns. The built-in deny list can never be whitelisted.

  2. user-supplied deny_patterns replace built-in list: config deny_patterns uses or [list] which accidentally disables default safety patterns.

  3. deny pattern blocks silently abort turns: deny pattern error message "blocked by safety guard" is included in _WORKSPACE_BLOCK_MARKERS, causing deny_pattern blocks to be misclassified as fatal workspace violations. LLMs have no chance to retry — the turn is aborted immediately, and if MessageTool._sent_in_turn is True, the error message is silently swallowed (returns None) with no notification sent to the user.

Changes

File Change
nanobot/agent/tools/shell.py allow_patterns checked first; if matched, skip deny check. deny_patterns now appends to built-in list. Error messages use distinct phrasing to separate deny/allowlist blocks from workspace path errors.
nanobot/agent/config/schema.py Add allow_patterns and deny_patterns fields to ExecToolConfig
nanobot/agent/runner.py Remove "blocked by safety guard" from _WORKSPACE_BLOCK_MARKERS so deny_pattern errors are treated as normal tool errors (LLM can retry) instead of fatal violations
nanobot/agent/loop.py Pass config allow/deny patterns to ExecTool
nanobot/agent/subagent.py Pass config allow/deny patterns to ExecTool (same as loop.py)
tests/tools/test_exec_allow_patterns.py Tests covering priority, whitelist-only, extra deny patterns, and updated assertions

Config Example

{
  "exec": {
    "allowPatterns": ["rm\\s+-rf\\s+/tmp/"],
    "denyPatterns": ["\\bping\\b"]
  }
}

This allows destructive commands under /tmp/ while still blocking other usage.

Previously deny_patterns were checked first with no bypass, meaning
allow_patterns could never exempt commands from the built-in deny list.
This made it impossible to whitelist destructive commands for specific
directories (e.g. build/cleanup tasks).

Changes:
- shell.py: check allow_patterns first; if matched, skip deny check
- shell.py: deny_patterns now appends to built-in list (not replaces)
- schema.py: add allow_patterns/deny_patterns to ExecToolConfig
- loop.py/subagent.py: pass allow_patterns/deny_patterns to ExecTool
- Add test_exec_allow_patterns.py covering priority semantics
The deny pattern error message "Command blocked by safety guard" was
included in _WORKSPACE_BLOCK_MARKERS, causing deny_pattern blocks to be
misclassified as fatal workspace violations. This meant LLMs had no
chance to retry with a different command — the turn was aborted
immediately.

Changes:
- shell.py: deny/allowlist error messages now use distinct phrasing
  ("blocked by deny pattern filter" / "blocked by allowlist filter")
- runner.py: remove "blocked by safety guard" from
  _WORKSPACE_BLOCK_MARKERS so deny_pattern errors are treated as normal
  tool errors (LLM can retry) instead of fatal violations
- workspace path errors still use "blocked by safety guard" and remain
  fatal as intended
@chengyongru chengyongru marked this pull request as ready for review May 2, 2026 15:55
Address review feedback on the deny/allow_patterns rework:

- runner.py: re-add "internal/private url detected" to
  _WORKSPACE_BLOCK_MARKERS. The earlier marker removal also stripped
  fatal classification from SSRF / internal-URL rejections (whose
  message still says "blocked by safety guard"), turning a hard
  security boundary into something the LLM could retry.
- loop.py / subagent.py: drop `or None` between ExecToolConfig and
  ExecTool. The schema default is an empty list and ExecTool already
  normalizes None back to [], so the indirection was a no-op.
- shell.py: extract `explicitly_allowed` flag in _guard_command so
  allow_patterns are scanned once instead of twice and the control
  flow no longer relies on a no-op `pass + else` branch.
- tests/agent/test_runner.py: add a regression test asserting that
  the SSRF block message is treated as fatal, while deny/allowlist
  filter messages are deliberately non-fatal.
@chengyongru chengyongru added the bug Something isn't working label May 2, 2026
Keep the new ExecTool allow-pattern coverage clean under ruff.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copy link
Copy Markdown
Collaborator

@Re-bin Re-bin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right kind of fix.

The PR tightens the ExecTool policy boundary in the intended way: configured deny patterns are appended to the built-in deny list instead of replacing it, allow patterns can explicitly exempt narrow commands from the deny-pattern filter, and deny/allowlist blocks are no longer misclassified as fatal workspace violations. I also checked that this does not bypass the harder safety boundaries: SSRF/internal URL and workspace path guards still run after the allow-pattern decision.

I pushed one tiny cleanup commit (d137cc44) to remove an unused import from the new test file.

Local validation:

  • uv run pytest tests/tools/test_exec_allow_patterns.py tests/tools/test_exec_security.py tests/agent/test_runner.py -q -> 110 passed
  • uv run ruff check nanobot/agent/tools/shell.py nanobot/config/schema.py nanobot/agent/runner.py nanobot/agent/loop.py nanobot/agent/subagent.py tests/tools/test_exec_allow_patterns.py tests/tools/test_exec_security.py && git diff --check -> passed
  • uv run pytest -q -> 2616 passed, 2 skipped

From my side, this is clean and ready to merge once CI is green.

@Re-bin Re-bin merged commit 5853d5d into HKUDS:main May 2, 2026
8 checks passed
bibistellar pushed a commit to bibistellar/nanobot that referenced this pull request May 3, 2026
上游新提交:
- refactor(setup): enhance SKILL.md for upgrade process clarity
- fix: improve media failure diagnostics and token fallback coverage
- fix: allow_patterns take priority over deny_patterns in ExecTool (HKUDS#3594)

冲突解决:
- shell.py: 保留我们的 rm -rf 移除,采用上游新增的 deny patterns

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants