Repository navigation
feat(budget): deny-by-default side-effect lockout on the grace turn (Guard D-core) - #37
Conversation
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-attribute |
9 |
invalid-argument-type |
1 |
invalid-assignment |
1 |
First entries
tests/agent/test_budget_grace_gate.py:317: [unresolved-attribute] unresolved-attribute: Object of type `AIAgent` has no attribute `_budget_grace_call`
tests/agent/test_budget_grace_gate.py:122: [unresolved-attribute] unresolved-attribute: Unresolved attribute `_use_prompt_caching` on type `AIAgent`
tests/agent/test_budget_grace_gate.py:121: [unresolved-attribute] unresolved-attribute: Unresolved attribute `_cached_system_prompt` on type `AIAgent`
tests/agent/test_budget_grace_gate.py:308: [unresolved-attribute] unresolved-attribute: Unresolved attribute `_in_budget_grace` on type `AIAgent`
tests/agent/test_budget_grace_gate.py:123: [unresolved-attribute] unresolved-attribute: Unresolved attribute `tool_delay` on type `AIAgent`
tests/agent/test_budget_grace_gate.py:124: [unresolved-attribute] unresolved-attribute: Unresolved attribute `compression_enabled` on type `AIAgent`
tests/agent/test_budget_grace_gate.py:309: [unresolved-attribute] unresolved-attribute: Unresolved attribute `_budget_grace_call` on type `AIAgent`
tests/agent/test_budget_grace_gate.py:290: [unresolved-attribute] unresolved-attribute: Object of type `AIAgent` has no attribute `_in_budget_grace`
tests/agent/test_budget_grace_gate.py:125: [unresolved-attribute] unresolved-attribute: Unresolved attribute `save_trajectories` on type `AIAgent`
tests/agent/test_budget_grace_gate.py:57: [invalid-argument-type] invalid-argument-type: Argument to function `is_readonly_grace_tool` is incorrect: Expected `str`, found `None`
tests/run_agent/test_credits_notices_toggle.py:76: [invalid-assignment] invalid-assignment: Object of type `None` is not assignable to attribute `_credits_session_start_micros` of type `int`
✅ Fixed issues (2):
| Rule | Count |
|---|---|
unresolved-attribute |
2 |
First entries
tests/run_agent/test_credits_notices_toggle.py:76: [unresolved-attribute] unresolved-attribute: Unresolved attribute `_credits_session_start_micros` on type `AIAgent`
run_agent.py:2931: [unresolved-attribute] unresolved-attribute: Object of type `Self@get_credits_spent_micros` has no attribute `_credits_session_start_micros`
Unchanged: 5860 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
3e89710 to
10b13dd
Compare
|
| Filename | Overview |
|---|---|
| agent/budget_grace_gate.py | New pure module implementing the deny-by-default grace-turn allowlist, block message, and block result helpers. Logic is correct, allowlist is small and audited, mutating-set override is robust, and __all__ export is consistent with what the dispatchers actually import. |
| agent/conversation_loop.py | Correctly sets _in_budget_grace = False at the top of every iteration (guaranteed reset) and True only when the grace flag is consumed; the grace flag is cleared before the gate is raised, preventing any re-arm. |
| agent/tool_executor.py | Both concurrent (parse-phase) and sequential paths now check _in_budget_grace before other block types, use grace_block_result() for the model-visible JSON (with the budget_grace_block metadata key), and pass middleware_trace to _emit_terminal_post_tool_call consistently with sibling block paths. |
| agent/agent_init.py | Adds _in_budget_grace = False initialization alongside the existing _budget_grace_call flag; correct placement and default value. |
| tests/agent/test_budget_grace_gate.py | 13 tests covering both predicate-layer and real-dispatcher integration for sequential and concurrent paths, including mixed-batch per-call gating, deny-by-default for unknown tools, the budget_grace_block metadata key in both dispatchers, and the no-rearm invariant. One inline comment is stale after the shared-helper fix (see comment). |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Loop iteration start] --> B["agent._in_budget_grace = False"]
B --> C{_budget_grace_call?}
C -- Yes --> D["_budget_grace_call = False\n_in_budget_grace = True"]
C -- No --> E{Budget remaining?}
E -- No --> F[break — budget_exhausted]
E -- Yes --> G[API call → tool calls returned]
D --> G
G --> H{_in_budget_grace?}
H -- False --> I[Normal tool execution]
H -- True --> J{is_readonly_grace_tool?}
J -- Yes / allowlisted --> K[Execute tool normally]
J -- No / side-effect / unknown --> L["grace_block_result()\nerror_type=budget_grace_block\n_emit_terminal_post_tool_call"]
L --> M[Append blocked role=tool message]
K --> N[Append result role=tool message]
M --> O[Next iteration → _in_budget_grace reset to False → budget exhausted → break]
N --> O
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Loop iteration start] --> B["agent._in_budget_grace = False"]
B --> C{_budget_grace_call?}
C -- Yes --> D["_budget_grace_call = False\n_in_budget_grace = True"]
C -- No --> E{Budget remaining?}
E -- No --> F[break — budget_exhausted]
E -- Yes --> G[API call → tool calls returned]
D --> G
G --> H{_in_budget_grace?}
H -- False --> I[Normal tool execution]
H -- True --> J{is_readonly_grace_tool?}
J -- Yes / allowlisted --> K[Execute tool normally]
J -- No / side-effect / unknown --> L["grace_block_result()\nerror_type=budget_grace_block\n_emit_terminal_post_tool_call"]
L --> M[Append blocked role=tool message]
K --> N[Append result role=tool message]
M --> O[Next iteration → _in_budget_grace reset to False → budget exhausted → break]
N --> O
Reviews (4): Last reviewed commit: "feat(budget): deny-by-default side-effec..." | Re-trigger Greptile
| def grace_block_result(tool_name: str) -> str: | ||
| """Build the synthetic role=tool content string for a grace-refused call. | ||
|
|
||
| Shape mirrors the plugin/guardrail block path in the dispatchers | ||
| (``{"error": ...}``) so the model sees a consistent blocked-tool result. | ||
| """ | ||
| return json.dumps( | ||
| { | ||
| "error": grace_block_message(tool_name), | ||
| "budget_grace_block": {"tool_name": tool_name, "reason": "budget_exhausted_grace_turn"}, | ||
| }, | ||
| ensure_ascii=False, | ||
| ) |
There was a problem hiding this comment.
grace_block_result() is exported in __all__ and tested in test_block_message_and_result_shape, but neither dispatcher imports or calls it. Both the concurrent path (line 341 of tool_executor.py) and the sequential path (line 962) build their own json.dumps({"error": _grace_msg}) directly, omitting the budget_grace_block nested object. As a result the structured field — which the docstring says is intended to carry telemetry — never actually appears in any tool result message. test_block_message_and_result_shape is therefore verifying behaviour of a dead code path rather than what the model or a log parser would see. Either the dispatchers should call grace_block_result() instead of constructing their own JSON, or the function should be removed (or at least not re-exported) to avoid the false guarantee.
|
| Filename | Overview |
|---|---|
| agent/budget_grace_gate.py | New deny-by-default gate module; grace_block_result() is exported in __all__ and unit-tested but never called by either dispatcher, creating a tested JSON shape that the model never actually receives. |
| agent/tool_executor.py | Both dispatch paths correctly gate on _in_budget_grace; block result is built inline as {"error": ...} rather than via grace_block_result(), omitting the budget_grace_block metadata key that the unit test validates. |
| agent/conversation_loop.py | Reset-then-set pattern for _in_budget_grace is safe; grace flag is consumed before the flag is armed, ensuring no re-arm is possible. |
| agent/agent_init.py | Cleanly initializes _in_budget_grace = False alongside the existing grace-call flags; no issues. |
| tests/agent/test_budget_grace_gate.py | Good integration and predicate coverage; test_block_message_and_result_shape validates grace_block_result() shape but the dispatchers never call that function, so the budget_grace_block key assertion has no live counterpart; also missing a concurrent mixed-batch (read + side-effect) test. |
Comments Outside Diff (1)
-
agent/tool_executor.py, line 960-975 (link)Sequential path has the same divergence: it builds
json.dumps({"error": _block_msg})inline rather than usinggrace_block_result(). Replace so both paths produce the same structure and thebudget_grace_blockmetadata key appears in the actual tool result.
Reviews (2): Last reviewed commit: "feat(budget): deny-by-default side-effec..." | Re-trigger Greptile
…Guard D-core) The tool-calling loop in agent/conversation_loop.py runs one optional grace turn after the iteration budget is exhausted (the `_budget_grace_call` hook). That grace turn would execute whatever tools the model returns — including side-effecting ones (terminal, execute_code, write_file, delegate_task, send_message). For a runaway worker that's one more chance to spawn/write past its budget. This adds a deny-by-default gate for the grace turn: - New pure module agent/budget_grace_gate.py: a small audited read-only allowlist (read_file, search_files, TaskSearch, session_search, mem0_search/ profile, skill_view, skills_list, read-only MCP fs) and is_readonly_grace_tool() which refuses everything else — including unknown/future/third-party tool names. The known-mutating set always wins over the allowlist. - conversation_loop.py sets agent._in_budget_grace True ONLY on the grace turn (recomputed False every other iteration). Refusing a call never re-arms the grace flag, so a deny can't loop. - Both dispatch paths (concurrent + sequential, agent/tool_executor.py) refuse non-allowlisted tools during the grace turn with a clear blocked tool result (error_type=budget_grace_block) and skip real execution — per-call gating, so a mixed batch runs the read and refuses the side effect. Note: `_budget_grace_call` is currently never armed to True in-tree, so this is defense-in-depth for a latent hole, not a live bug. Tested accordingly by driving _in_budget_grace directly through the real dispatchers. Tests (tests/agent/test_budget_grace_gate.py, 13): predicate allow/deny/unknown/ mutating-overlap/message-shape + real-dispatch integration on both paths (refuse side-effect, allow read-only, refuse unknown, mixed-batch per-call gate, no-grace control, no-rearm). RED-proven: stashing the dispatcher gate fails exactly the 4 side-effect-refusal integration tests; restored → green. No regression in the existing guardrail-runtime/guardrail suites (35 passed).
10b13dd to
0803c67
Compare
…39, #35 sibling) - #37 import map is module-level; function-local imports bind only in their function - #38 sink_dotted honoured when sink_names is None - #39 a function-local import shadows a same-file def - offload-call args walked (eager), lambda args still deferred New precision arms: 4 red on base, 5/5 green. Consumer gates 32/32; the widened walker surfaced 2 pre-existing telegram get_label->requests.get reaches (same shape as baselined matrix entry), added to REACHABLE_BASELINE.
…39, #35 sibling) - #37 import map is module-level; function-local imports bind only in their function - #38 sink_dotted honoured when sink_names is None - #39 a function-local import shadows a same-file def - offload-call args walked (eager), lambda args still deferred New precision arms: 4 red on base, 5/5 green. Consumer gates 32/32; the widened walker surfaced 2 pre-existing telegram get_label->requests.get reaches (same shape as baselined matrix entry), added to REACHABLE_BASELINE.
Guard D-core — deny-by-default side-effect lockout on the budget grace turn
Problem
The tool-calling loop (
agent/conversation_loop.py) runs one optional grace turn after the iteration budget is exhausted, gated by the_budget_grace_callhook. That grace turn executes whatever tools the model returns — including side-effecting ones (terminal,execute_code,write_file,delegate_task,send_message, …). For a runaway worker, that's one more chance to spawn a subprocess or write to disk after its budget is already gone.This is the root-cause complement to the runaway-worker guards (B/E/F) that cap blast radius around the loop. This stops the loop itself from taking a side-effecting action past budget.
Change
agent/budget_grace_gate.py— a small, audited read-only allowlist (read_file,search_files,TaskSearch,session_search,mem0_search/mem0_profile,skill_view,skills_list, read-only MCP filesystem tools) andis_readonly_grace_tool()which refuses everything else, including unknown / future / third-party tool names (deny-by-default, not a denylist). The known-mutating set (MUTATING_TOOL_NAMES) always wins over the allowlist as defense-in-depth.conversation_loop.pysetsagent._in_budget_grace = Trueonly on the grace turn (recomputedFalseevery other iteration). A refusal never re-arms_budget_grace_call, so a deny can't loop the agent alive.agent/tool_executor.pyconcurrent + sequential) refuse non-allowlisted tools during the grace turn with a clear blocked tool result (error_type=budget_grace_block) and skip real execution. Per-call gating: a mixed batch executes the read-only call and refuses the side-effecting one individually.Honest scope note
_budget_grace_callis currently never armed toTrueanywhere in-tree — the grace turn is a dormant hook, so today the loopbreaks on budget exhaustion before any tool runs. This change is therefore defense-in-depth for a latent hole, not a fix for a live exploit. Because the path is dormant, an "exhaust budget end-to-end" test would be vacuously green; the tests instead drive_in_budget_gracedirectly through the real dispatchers — the exact state the loop sets on the grace turn.Tests —
tests/agent/test_budget_grace_gate.py(13)handle_function_callnot called, blocked role=tool message appended), allow read-only, refuse unknown, mixed-batch per-call gate, no-grace control (side effect runs normally), no-rearm of the grace flag.test_tool_call_guardrail_runtime.py+test_tool_guardrails.py+ this suite = 35 passed.ruff checkclean on all changed files.Rollback
Self-contained: revert the commit. The new module is additive; the loop/dispatcher changes are guarded by
_in_budget_grace(defaultFalse), so behavior outside the grace turn is unchanged.