Replaces PR #3493's blanket fatal abort with a "tell the model + throttle the bypass loop" policy. Workspace-bound rejections are now ordinary recoverable tool errors enriched with a structured "this is a hard policy boundary" instruction; SSRF stays the only marker that aborts the turn. Why the fatal-abort approach broke ---------------------------------- PR #3493 promoted every shell `_guard_command` and filesystem path-resolution rejection to a turn-fatal RuntimeError. Two of those messages (`path outside working dir` and `path traversal detected`) are heuristic substring scans on the raw command, so legitimate commands like `rm <ws>/x.txt 2>/dev/null` or `find . -type f` killed the user's turn (#3599). On channels with outbound dedupe (Telegram) the user just saw silence (#3605), and the noise polluted the LLM's context until it started hallucinating guard rejections on plain relative paths (#3597). Why we still need *some* throttle --------------------------------- The original #3493 pain point was real: the LLM, refused once, would swap tools and try again -- read_file -> exec cat -> exec cp -> bash -c -> ln -sf -> python -c open(...). Just removing the fatal escape lets that loop run wild until max_iterations. What this commit does --------------------- - `nanobot/utils/runtime.py`: add `workspace_violation_signature` and `repeated_workspace_violation_error`. The signature normalizes filesystem `path` arguments and the first absolute path inside an exec command, so swapping tools against the same outside target hits the same throttle bucket. Two soft attempts are allowed; the third attempt's tool result is replaced with a hard "stop trying to bypass" message that quotes the target path and tells the model to ask the user for help. - `nanobot/agent/runner.py`: split classification into `_is_ssrf_violation` (still fatal) and `_is_workspace_violation` (now soft). All three failure branches in `_run_tool` (prep_error / exception / Error result) route through a shared `_classify_violation` that bumps the per-turn workspace_violation_counts dict and either keeps the tool's own message or substitutes the throttle escalation. `_execute_tools` now threads that dict alongside the existing external_lookup_counts. - `nanobot/agent/tools/shell.py`: append a structured boundary note to every workspace-bound guard rejection (`working_dir could not be resolved`, `working_dir is outside`, `path outside working dir`, `path traversal detected`). SSRF errors stay short and direct so the model doesn't try to "phrase around" them. Existing `2>/dev/null` allow-list and benign device passthrough from the previous commit remain. - `nanobot/agent/tools/filesystem.py`: append the same boundary note to the `outside allowed directory` PermissionError so read_file / write_file / list_dir errors give the LLM the same explicit hint. Tests ----- - `tests/utils/test_workspace_violation_throttle.py` (new): signature collapses across read_file/exec/python -c against the same path, different paths get independent budgets, escalation only fires after the third attempt. - `tests/agent/test_runner.py`: - `test_runner_does_not_abort_on_workspace_violation_anymore` -- v2 contract: filesystem PermissionError is now soft, runner moves to the next iteration and finalizes cleanly. - `test_is_ssrf_violation_remains_fatal` + the existing `test_runner_aborts_on_ssrf_violation` -- SSRF still aborts on the first attempt. - `test_runner_lets_llm_recover_from_shell_guard_path_outside` -- end to end recovery from `path outside working dir`. - `test_runner_throttles_repeated_workspace_bypass_attempts` -- four bypass attempts against the same outside target produce at least one `workspace_violation_escalated` event and the run completes naturally without aborting the turn. - The two `_execute_tools` direct-call tests now pass the new workspace_violation_counts dict. - `tests/tools/test_tool_validation.py`: relax three `==` assertions to `startswith` + "hard policy boundary" substring check to match the new structured error messages. - `tests/tools/test_exec_security.py` keeps the prior `2>/dev/null` regression and the `> /etc/issue` negative case from the previous commit on this branch -- they still pass under the new policy. Coverage status: full pytest 2648 passed / 2 skipped (was 2638 / 2 on origin/main). Ruff is clean for every file touched in this commit. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Xubin Ren
committed by
Xubin Ren
Xubin Ren
co-authored by
Cursor
Cursor
parent
7742f8fbdc
commit
b8406be215
+147
-53
@@ -313,21 +313,33 @@ async def test_runner_returns_structured_tool_error():
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_runner_stops_on_workspace_violation_without_fail_on_tool_error():
|
||||
async def test_runner_does_not_abort_on_workspace_violation_anymore():
|
||||
"""v2 behavior: workspace-bound rejections are *soft* tool errors.
|
||||
|
||||
Previously (PR #3493) any workspace boundary error became a fatal
|
||||
RuntimeError that aborted the turn. That silently killed legitimate
|
||||
workspace commands once the heuristic guard misfired (#3599 #3605), so
|
||||
we now hand the error back to the LLM as a recoverable tool result and
|
||||
rely on ``repeated_workspace_violation_error`` to throttle bypass loops.
|
||||
"""
|
||||
from nanobot.agent.runner import AgentRunSpec, AgentRunner
|
||||
|
||||
provider = MagicMock()
|
||||
provider.chat_with_retry = AsyncMock(side_effect=[
|
||||
LLMResponse(
|
||||
content="working",
|
||||
tool_calls=[ToolCallRequest(id="call_1", name="read_file", arguments={"path": "/tmp/outside.md"})],
|
||||
content="trying outside",
|
||||
tool_calls=[ToolCallRequest(
|
||||
id="call_1", name="read_file", arguments={"path": "/tmp/outside.md"},
|
||||
)],
|
||||
),
|
||||
LLMResponse(content="should not continue", tool_calls=[]),
|
||||
LLMResponse(content="ok, telling the user instead", tool_calls=[]),
|
||||
])
|
||||
tools = MagicMock()
|
||||
tools.get_definitions.return_value = []
|
||||
tools.execute = AsyncMock(
|
||||
side_effect=PermissionError("Path /tmp/outside.md is outside allowed directory /workspace")
|
||||
side_effect=PermissionError(
|
||||
"Path /tmp/outside.md is outside allowed directory /workspace"
|
||||
)
|
||||
)
|
||||
|
||||
runner = AgentRunner(provider)
|
||||
@@ -336,71 +348,92 @@ async def test_runner_stops_on_workspace_violation_without_fail_on_tool_error():
|
||||
initial_messages=[],
|
||||
tools=tools,
|
||||
model="test-model",
|
||||
max_iterations=2,
|
||||
max_iterations=3,
|
||||
max_tool_result_chars=_MAX_TOOL_RESULT_CHARS,
|
||||
))
|
||||
|
||||
assert provider.chat_with_retry.await_count == 1
|
||||
assert result.stop_reason == "tool_error"
|
||||
assert "outside allowed directory" in (result.error or "")
|
||||
assert result.tool_events == [
|
||||
{
|
||||
"name": "read_file",
|
||||
"status": "error",
|
||||
"detail": "workspace_violation: Path /tmp/outside.md is outside allowed directory /workspace",
|
||||
}
|
||||
]
|
||||
assert provider.chat_with_retry.await_count == 2, (
|
||||
"workspace violation must NOT short-circuit the loop"
|
||||
)
|
||||
assert result.stop_reason != "tool_error"
|
||||
assert result.error is None
|
||||
assert result.final_content == "ok, telling the user instead"
|
||||
assert result.tool_events and result.tool_events[0]["status"] == "error"
|
||||
# Detail still carries the workspace_violation breadcrumb for telemetry,
|
||||
# but the runner did not raise.
|
||||
assert "workspace_violation" in result.tool_events[0]["detail"]
|
||||
|
||||
|
||||
def test_is_workspace_violation_recognizes_ssrf_block():
|
||||
"""Internal/private URL block must be classified as a fatal workspace violation.
|
||||
def test_is_ssrf_violation_remains_fatal():
|
||||
"""SSRF rejections are the only marker that stays turn-fatal.
|
||||
|
||||
Regression guard: the deny/allowlist filter messages were intentionally split
|
||||
out of `_WORKSPACE_BLOCK_MARKERS` so the LLM can retry, but SSRF rejections
|
||||
are a hard security boundary and must remain fatal.
|
||||
A single successful internal-URL fetch can leak cloud metadata, so we
|
||||
never let the LLM "retry" with a different URL phrasing -- contrast
|
||||
this with workspace-bound rejections which are soft + throttled in v2.
|
||||
"""
|
||||
from nanobot.agent.runner import AgentRunner
|
||||
|
||||
ssrf_msg = "Error: Command blocked by safety guard (internal/private URL detected)"
|
||||
assert AgentRunner._is_workspace_violation(ssrf_msg) is True
|
||||
assert AgentRunner._is_ssrf_violation(ssrf_msg) is True
|
||||
|
||||
# Sanity: deny/allowlist filter messages are deliberately *not* fatal.
|
||||
assert AgentRunner._is_workspace_violation(
|
||||
"Error: Command blocked by deny pattern filter"
|
||||
) is False
|
||||
assert AgentRunner._is_workspace_violation(
|
||||
"Error: Command blocked by allowlist filter (not in allowlist)"
|
||||
) is False
|
||||
|
||||
|
||||
def test_is_workspace_violation_does_not_fatal_on_shell_guard_heuristics():
|
||||
"""#3599 / #3605 regression: shell guard heuristics must NOT be fatal.
|
||||
|
||||
``path outside working dir`` and ``path traversal detected`` are produced
|
||||
by best-effort string scans inside ``ExecTool._guard_command`` -- they
|
||||
routinely false-positive on idiomatic constructs (``2>/dev/null``,
|
||||
``sed 's|x|../y|g'``) and should be surfaced to the LLM as recoverable
|
||||
tool errors so it can switch tactics, not abort the whole turn.
|
||||
"""
|
||||
from nanobot.agent.runner import AgentRunner
|
||||
|
||||
assert AgentRunner._is_workspace_violation(
|
||||
# Workspace-bound markers are NOT classified as SSRF.
|
||||
assert AgentRunner._is_ssrf_violation(
|
||||
"Error: Command blocked by safety guard (path outside working dir)"
|
||||
) is False
|
||||
assert AgentRunner._is_workspace_violation(
|
||||
"Error: Command blocked by safety guard (path traversal detected)"
|
||||
assert AgentRunner._is_ssrf_violation(
|
||||
"Path /tmp/x is outside allowed directory /ws"
|
||||
) is False
|
||||
# Deny / allowlist filter messages stay non-fatal too.
|
||||
assert AgentRunner._is_ssrf_violation(
|
||||
"Error: Command blocked by deny pattern filter"
|
||||
) is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_runner_lets_llm_recover_from_shell_guard_path_outside():
|
||||
"""End-to-end: a guard-blocked exec is a soft tool error, not a turn-fatal.
|
||||
async def test_runner_aborts_on_ssrf_violation():
|
||||
"""SSRF still fatal-aborts the turn even though workspace ones are soft."""
|
||||
from nanobot.agent.runner import AgentRunSpec, AgentRunner
|
||||
|
||||
Reporter scenario: a previous PR turned ``path outside working dir`` into
|
||||
a turn-fatal RuntimeError, so when the false-positive guard fired the
|
||||
user got no further iterations and (depending on channel) a silent hang.
|
||||
After narrowing the marker list, the runner must hand the error back to
|
||||
the LLM and let the next iteration succeed normally.
|
||||
provider = MagicMock()
|
||||
provider.chat_with_retry = AsyncMock(side_effect=[
|
||||
LLMResponse(
|
||||
content="curl-ing metadata",
|
||||
tool_calls=[ToolCallRequest(
|
||||
id="call_ssrf",
|
||||
name="exec",
|
||||
arguments={"command": "curl http://169.254.169.254"},
|
||||
)],
|
||||
),
|
||||
LLMResponse(content="should NOT be reached", tool_calls=[]),
|
||||
])
|
||||
tools = MagicMock()
|
||||
tools.get_definitions.return_value = []
|
||||
tools.execute = AsyncMock(return_value=(
|
||||
"Error: Command blocked by safety guard (internal/private URL detected)"
|
||||
))
|
||||
|
||||
runner = AgentRunner(provider)
|
||||
result = await runner.run(AgentRunSpec(
|
||||
initial_messages=[],
|
||||
tools=tools,
|
||||
model="test-model",
|
||||
max_iterations=3,
|
||||
max_tool_result_chars=_MAX_TOOL_RESULT_CHARS,
|
||||
))
|
||||
|
||||
assert provider.chat_with_retry.await_count == 1, "SSRF must abort immediately"
|
||||
assert result.stop_reason == "tool_error"
|
||||
assert "internal/private url detected" in (result.error or "").lower()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_runner_lets_llm_recover_from_shell_guard_path_outside():
|
||||
"""Reporter scenario for #3599 / #3605 -- guard hit, agent recovers.
|
||||
|
||||
The shell `_guard_command` heuristic fires on `2>/dev/null`-style
|
||||
redirects and other shell idioms. Before v2 that abort'd the whole
|
||||
turn (silent hang on Telegram per #3605); now the LLM gets the soft
|
||||
error back and can finalize on the next iteration.
|
||||
"""
|
||||
from nanobot.agent.runner import AgentRunSpec, AgentRunner
|
||||
|
||||
@@ -443,7 +476,66 @@ async def test_runner_lets_llm_recover_from_shell_guard_path_outside():
|
||||
assert result.error is None
|
||||
assert result.final_content == "recovered final answer"
|
||||
assert result.tool_events and result.tool_events[0]["status"] == "error"
|
||||
assert "workspace_violation" not in result.tool_events[0]["detail"]
|
||||
# v2: detail keeps the breadcrumb but the runner did not raise.
|
||||
assert "workspace_violation" in result.tool_events[0]["detail"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_runner_throttles_repeated_workspace_bypass_attempts():
|
||||
"""#3493 motivation: stop the LLM bypass loop without aborting the turn.
|
||||
|
||||
LLM keeps switching tools (read_file -> exec cat -> python -c open(...))
|
||||
against the same outside path. After the soft retry budget is exhausted
|
||||
the runner replaces the tool result with a hard "stop trying" message
|
||||
so the model finally gives up and surfaces the boundary to the user.
|
||||
"""
|
||||
from nanobot.agent.runner import AgentRunSpec, AgentRunner
|
||||
|
||||
bypass_attempts = [
|
||||
ToolCallRequest(
|
||||
id=f"a{i}", name="exec",
|
||||
arguments={"command": f"cat /Users/x/Downloads/01.md # try {i}"},
|
||||
)
|
||||
for i in range(4)
|
||||
]
|
||||
responses: list[LLMResponse] = [
|
||||
LLMResponse(content=f"try {i}", tool_calls=[bypass_attempts[i]])
|
||||
for i in range(4)
|
||||
]
|
||||
responses.append(LLMResponse(content="ok telling user", tool_calls=[]))
|
||||
|
||||
provider = MagicMock()
|
||||
provider.chat_with_retry = AsyncMock(side_effect=responses)
|
||||
tools = MagicMock()
|
||||
tools.get_definitions.return_value = []
|
||||
tools.execute = AsyncMock(
|
||||
return_value="Error: Command blocked by safety guard (path outside working dir)"
|
||||
)
|
||||
|
||||
runner = AgentRunner(provider)
|
||||
result = await runner.run(AgentRunSpec(
|
||||
initial_messages=[],
|
||||
tools=tools,
|
||||
model="test-model",
|
||||
max_iterations=10,
|
||||
max_tool_result_chars=_MAX_TOOL_RESULT_CHARS,
|
||||
))
|
||||
|
||||
# All 4 bypass attempts surface to the LLM (no fatal abort), and the
|
||||
# runner finally completes once the LLM stops asking.
|
||||
assert result.stop_reason != "tool_error"
|
||||
assert result.error is None
|
||||
assert result.final_content == "ok telling user"
|
||||
# The third+ attempts must have been escalated -- look at the events.
|
||||
escalated = [
|
||||
ev for ev in result.tool_events
|
||||
if ev["status"] == "error"
|
||||
and ev["detail"].startswith("workspace_violation_escalated:")
|
||||
]
|
||||
assert escalated, (
|
||||
"expected at least one escalated workspace_violation event, got: "
|
||||
f"{result.tool_events}"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -924,6 +1016,7 @@ async def test_runner_batches_read_only_tools_before_exclusive_work():
|
||||
ToolCallRequest(id="rw1", name="write_a", arguments={}),
|
||||
],
|
||||
{},
|
||||
{},
|
||||
)
|
||||
|
||||
assert shared_events[0:2] == ["start:read_a", "start:read_b"]
|
||||
@@ -968,6 +1061,7 @@ async def test_runner_does_not_batch_exclusive_read_only_tools():
|
||||
ToolCallRequest(id="ro2", name="read_b", arguments={}),
|
||||
],
|
||||
{},
|
||||
{},
|
||||
)
|
||||
|
||||
assert shared_events[0] == "start:read_a"
|
||||
|
||||
Reference in New Issue
Block a user