fix(agents): honor safe_mode and context.global.unsafe for LLM-agent tool calls #129

Merged
CoreRasurae merged 1 commit from bugfix/m1-llm-tool-unsafe-mode into master 2026-08-20 17:32:34 +00:00
Member

Summary

Fixes two compounding bugs that unconditionally blocked every file_write (and other tool) call issued by a type: llm agent's tool-call loop, regardless of actor configuration:

  1. LLMAgent._execute_tool_loop() (and its two synthesis-retry mirrors) derived parent_unsafe from a dead unsafe_mode config key that nothing in the codebase ever wrote, instead of the actual safe_mode field an actor author sets (docs/index.md §4.5). Fixed via a new LLMAgent._resolve_parent_unsafe() helper, and by threading the invocation context through _execute_tool_loop() so it can also see a host-propagated _unsafe_mode: true.
  2. cleveractors.runtime_dispatch._execute_graph/_execute_graph_stream (the Executor/create_executor() path) had no host-unsafe-flag concept at all, so an actor declaring context.global.unsafe: true (§9.4) was neither refused on an unsafe host nor granted _unsafe_mode propagation when the host was unsafe. Fixed by adding Executor(unsafe=...)/create_executor(unsafe=...) and a new runtime_dispatch._enforce_unsafe_flag() helper mirroring Application._enforce_unsafe_flag.

Default safe_mode: true sandboxing for file_read/shell/directory traversal is unaffected.

Test plan

  • features/llm_agent_tool_loop.feature@tdd_issue_115 scenario now passes without @tdd_expected_fail (LLM agent safe_mode: false path)
  • features/runtime_unsafe_host_mode.feature (new) — context.global.unsafe host-mode path: refusal, propagation, and no-regression scenarios, exercised end-to-end through create_executor() → real graph → real LLMAgent/ToolAgent (only the LangChain network boundary is mocked)
  • robot/llm_tool_calling.robot (extended) — same three scenarios via Robot integration test, real file I/O
  • nox -s unit_tests — 3021/3021 scenarios passing
  • nox -s coverage_report — 96.6% (threshold 96.5%)
  • nox -s integration_tests — 355/355 passing
  • nox -s lint, nox -s format -- --check, nox -s typecheck, nox -s security_scan, nox -s dead_code — all clean

Closes #115

## Summary Fixes two compounding bugs that unconditionally blocked every `file_write` (and other tool) call issued by a `type: llm` agent's tool-call loop, regardless of actor configuration: 1. `LLMAgent._execute_tool_loop()` (and its two synthesis-retry mirrors) derived `parent_unsafe` from a dead `unsafe_mode` config key that nothing in the codebase ever wrote, instead of the actual `safe_mode` field an actor author sets (docs/index.md §4.5). Fixed via a new `LLMAgent._resolve_parent_unsafe()` helper, and by threading the invocation `context` through `_execute_tool_loop()` so it can also see a host-propagated `_unsafe_mode: true`. 2. `cleveractors.runtime_dispatch._execute_graph`/`_execute_graph_stream` (the `Executor`/`create_executor()` path) had no host-unsafe-flag concept at all, so an actor declaring `context.global.unsafe: true` (§9.4) was neither refused on an unsafe host nor granted `_unsafe_mode` propagation when the host was unsafe. Fixed by adding `Executor(unsafe=...)`/`create_executor(unsafe=...)` and a new `runtime_dispatch._enforce_unsafe_flag()` helper mirroring `Application._enforce_unsafe_flag`. Default `safe_mode: true` sandboxing for `file_read`/`shell`/directory traversal is unaffected. ## Test plan - [x] `features/llm_agent_tool_loop.feature` — `@tdd_issue_115` scenario now passes without `@tdd_expected_fail` (LLM agent `safe_mode: false` path) - [x] `features/runtime_unsafe_host_mode.feature` (new) — `context.global.unsafe` host-mode path: refusal, propagation, and no-regression scenarios, exercised end-to-end through `create_executor()` → real graph → real `LLMAgent`/`ToolAgent` (only the LangChain network boundary is mocked) - [x] `robot/llm_tool_calling.robot` (extended) — same three scenarios via Robot integration test, real file I/O - [x] `nox -s unit_tests` — 3021/3021 scenarios passing - [x] `nox -s coverage_report` — 96.6% (threshold 96.5%) - [x] `nox -s integration_tests` — 355/355 passing - [x] `nox -s lint`, `nox -s format -- --check`, `nox -s typecheck`, `nox -s security_scan`, `nox -s dead_code` — all clean Closes #115
CoreRasurae added this to the v2.1.0 milestone 2026-08-09 23:29:13 +00:00
hurui200320 requested changes 2026-08-10 07:12:39 +00:00
Dismissed
hurui200320 left a comment

PR Review: !129 (Ticket #115)

Verdict: Request Changes

The PR correctly fixes the two compounding bugs described in #115 and adds focused BDD/Robot coverage for the graph runtime path and the direct LLMAgent tool-call path. The code is well-typed, the new helpers are clearly documented, and the existing scenarios that relied on the dead unsafe_mode key have been updated appropriately.

One major functional gap remains: the new host unsafe flag is not forwarded to sub-Executors in the multi-actor dispatch path, so a multi-actor bundle containing an unsafe-declared graph actor will still be refused on an unsafe host.

Critical Issues

None.

Major Issues

  • Host unsafe flag is lost across multi-actor sub-Executors
    • File: src/cleveractors/runtime_dispatch.py (lines 736–743)
    • Problem: _execute_multi_actor creates a child Executor but does not pass unsafe=executor.unsafe (or registry_api_key=executor.registry_api_key). Consequently, a multi-actor bundle whose selected sub-actor is a graph declaring context.global.unsafe: true will raise UnsafeConfigurationError even when the parent host was placed in unsafe mode. This breaks the host-mode contract that the PR is introducing.
    • Recommendation: Forward unsafe=executor.unsafe and registry_api_key=executor.registry_api_key when constructing the sub-Executor, and add a Behave/Robot scenario that verifies unsafe-mode propagation through a multi-actor bundle.

Minor Issues

  • Single LLM actor path ignores context.global.unsafe

    • File: src/cleveractors/runtime_dispatch.py (_execute_llm, _execute_llm_stream)
    • Problem: _enforce_unsafe_flag is only wired into _execute_graph and _execute_graph_stream. A single type: llm actor config that declares context.global.unsafe: true is neither refused on a safe host nor granted _unsafe_mode on an unsafe host, leaving a spec-compliance gap outside the graph path.
    • Recommendation: Apply the same enforcement/propagation in _execute_llm and _execute_llm_stream, or explicitly document that context.global.unsafe is only honored by graph actors.
  • Streaming graph path lacks unsafe-mode coverage

    • File: features/runtime_unsafe_host_mode.feature, robot/llm_tool_calling.robot
    • Problem: _execute_graph_stream now calls _enforce_unsafe_flag, but no new scenario exercises refusal or propagation through Executor.execute_stream().
    • Recommendation: Add a Behave or Robot scenario for the streaming graph path.

Nits

  • Stale docstring in _execute_multi_actor
    • File: src/cleveractors/runtime_dispatch.py (line 703)
    • Problem: The docstring lists credentials, limits, and pricing as propagated, but now unsafe (and registry_api_key) should also be propagated.
    • Recommendation: Update the docstring to mention the newly forwarded host flags.

Summary

The core fix is sound and the tests directly validate the reported file_write blocking bug. The missing unsafe propagation in _execute_multi_actor is the only blocker; once that is fixed and covered by a test, this PR should be ready to approve.

## PR Review: !129 (Ticket #115) ### Verdict: Request Changes The PR correctly fixes the two compounding bugs described in #115 and adds focused BDD/Robot coverage for the graph runtime path and the direct `LLMAgent` tool-call path. The code is well-typed, the new helpers are clearly documented, and the existing scenarios that relied on the dead `unsafe_mode` key have been updated appropriately. One major functional gap remains: the new host `unsafe` flag is not forwarded to sub-Executors in the multi-actor dispatch path, so a multi-actor bundle containing an unsafe-declared graph actor will still be refused on an unsafe host. ### Critical Issues None. ### Major Issues - **Host `unsafe` flag is lost across multi-actor sub-Executors** - **File:** `src/cleveractors/runtime_dispatch.py` (lines 736–743) - **Problem:** `_execute_multi_actor` creates a child `Executor` but does not pass `unsafe=executor.unsafe` (or `registry_api_key=executor.registry_api_key`). Consequently, a multi-actor bundle whose selected sub-actor is a graph declaring `context.global.unsafe: true` will raise `UnsafeConfigurationError` even when the parent host was placed in unsafe mode. This breaks the host-mode contract that the PR is introducing. - **Recommendation:** Forward `unsafe=executor.unsafe` and `registry_api_key=executor.registry_api_key` when constructing the sub-Executor, and add a Behave/Robot scenario that verifies unsafe-mode propagation through a multi-actor bundle. ### Minor Issues - **Single LLM actor path ignores `context.global.unsafe`** - **File:** `src/cleveractors/runtime_dispatch.py` (`_execute_llm`, `_execute_llm_stream`) - **Problem:** `_enforce_unsafe_flag` is only wired into `_execute_graph` and `_execute_graph_stream`. A single `type: llm` actor config that declares `context.global.unsafe: true` is neither refused on a safe host nor granted `_unsafe_mode` on an unsafe host, leaving a spec-compliance gap outside the graph path. - **Recommendation:** Apply the same enforcement/propagation in `_execute_llm` and `_execute_llm_stream`, or explicitly document that `context.global.unsafe` is only honored by graph actors. - **Streaming graph path lacks unsafe-mode coverage** - **File:** `features/runtime_unsafe_host_mode.feature`, `robot/llm_tool_calling.robot` - **Problem:** `_execute_graph_stream` now calls `_enforce_unsafe_flag`, but no new scenario exercises refusal or propagation through `Executor.execute_stream()`. - **Recommendation:** Add a Behave or Robot scenario for the streaming graph path. ### Nits - **Stale docstring in `_execute_multi_actor`** - **File:** `src/cleveractors/runtime_dispatch.py` (line 703) - **Problem:** The docstring lists credentials, limits, and pricing as propagated, but now `unsafe` (and `registry_api_key`) should also be propagated. - **Recommendation:** Update the docstring to mention the newly forwarded host flags. ### Summary The core fix is sound and the tests directly validate the reported `file_write` blocking bug. The missing `unsafe` propagation in `_execute_multi_actor` is the only blocker; once that is fixed and covered by a test, this PR should be ready to approve.
fix(agents): honor safe_mode and context.global.unsafe for LLM-agent tool calls
Some checks failed
CI / lint (pull_request) Successful in 1m8s
CI / typecheck (pull_request) Successful in 1m30s
CI / security (pull_request) Successful in 1m18s
CI / quality (pull_request) Successful in 1m24s
CI / build (pull_request) Successful in 1m53s
CI / integration_tests (pull_request) Successful in 4m35s
CI / unit_tests (pull_request) Successful in 6m9s
CI / coverage (pull_request) Successful in 4m5s
CI / status-check (pull_request) Successful in 6s
CI / benchmark (pull_request) Failing after 26m8s
5e27912e59
LLMAgent's tool-call dispatch (_execute_tool_loop and its two synthesis-retry
mirrors) derived parent_unsafe from a dead "unsafe_mode" config key that
nothing in the codebase ever wrote, so file_write (and every other tool)
called by a type: llm agent's tool loop was unconditionally blocked
regardless of the actor's safe_mode setting. Separately, the
Executor/create_executor runtime path (runtime_dispatch._execute_graph /
_execute_graph_stream) had no host-unsafe-flag concept at all, so an actor
declaring context.global.unsafe: true was neither refused on an unsafe host
nor granted _unsafe_mode propagation when the host was actually unsafe.

Added LLMAgent._resolve_parent_unsafe() to derive unsafe status from the
agent's own safe_mode field or from an _unsafe_mode already present in the
invocation context, and threaded that context into _execute_tool_loop() so
all three dispatch sites can see it. Added Executor(unsafe=...) /
create_executor(unsafe=...) and runtime_dispatch._enforce_unsafe_flag(),
mirroring Application._enforce_unsafe_flag, so context.global.unsafe is
checked against the host flag and _unsafe_mode is propagated into every
tool-call context reachable from the graph, including an LLM agent's
internally dispatched tool calls.

Three existing scenarios in llm_agent_tool_calling.feature relied on
directly setting the dead "unsafe_mode" key to unblock a mocked file_read
call; updated them to set safe_mode: false instead.

Extended per PR review (rui.hu): _enforce_unsafe_flag() now also runs in
_execute_llm/_execute_llm_stream, so a top-level type: llm actor declaring
context.global.unsafe: true is enforced and propagated the same way a
graph actor is, not only an LLM agent embedded as a graph node.
_execute_multi_actor now forwards the parent Executor's unsafe flag (and
registry_api_key) to the sub-Executor it constructs for the bundle's
selected actor, closing a gap where a multi-actor bundle wrapping an
unsafe-declaring graph actor was always refused regardless of the host's
actual unsafe state. Added Behave coverage for both gaps plus the
previously-untested streaming graph path, and two Robot scenarios
exercising execute_stream() for the unsafe-host-mode graph actor.

ISSUES CLOSED: #115
CoreRasurae force-pushed bugfix/m1-llm-tool-unsafe-mode from ac50b16b09
Some checks failed
CI / quality (pull_request) Successful in 43s
CI / build (pull_request) Successful in 45s
CI / security (pull_request) Successful in 2m7s
CI / lint (pull_request) Successful in 2m24s
CI / typecheck (pull_request) Successful in 2m23s
CI / unit_tests (pull_request) Successful in 4m58s
CI / coverage (pull_request) Successful in 4m58s
CI / integration_tests (pull_request) Failing after 12m29s
CI / status-check (pull_request) Failing after 6s
CI / benchmark (pull_request) Failing after 21m40s
to 5e27912e59
Some checks failed
CI / lint (pull_request) Successful in 1m8s
CI / typecheck (pull_request) Successful in 1m30s
CI / security (pull_request) Successful in 1m18s
CI / quality (pull_request) Successful in 1m24s
CI / build (pull_request) Successful in 1m53s
CI / integration_tests (pull_request) Successful in 4m35s
CI / unit_tests (pull_request) Successful in 6m9s
CI / coverage (pull_request) Successful in 4m5s
CI / status-check (pull_request) Successful in 6s
CI / benchmark (pull_request) Failing after 26m8s
2026-08-10 15:04:55 +00:00
Compare
Author
Member

Thanks for the review — all four findings check out against the spec (docs/index.md §9.4/§10.3) and the code, and have been addressed in the amended commit (ac50b165e27912, force-pushed to this branch).

Critical Issues

None — no action needed.

Major Issues

Host unsafe flag lost across multi-actor sub-Executors — Confirmed, fixed. _execute_multi_actor (src/cleveractors/runtime_dispatch.py) now passes unsafe=executor.unsafe and registry_api_key=executor.registry_api_key when constructing the sub-Executor. Checked against Executor.__init__, which defaults both to False/None — so the sub-Executor previously always ran as if the host were safe, regardless of the parent's actual state, exactly as you described. Added two Behave scenarios in features/runtime_unsafe_host_mode.feature (propagation success + refusal) driving a multi-actor bundle whose selected sub-actor is an unsafe-declaring graph actor.

Minor Issues

Single LLM actor path ignores context.global.unsafe — Confirmed, fixed. _execute_llm and _execute_llm_stream now build global_context from the actor's context.global section and call _enforce_unsafe_flag(), mirroring _execute_graph/_execute_graph_stream exactly. When the host is unsafe and the actor declares it, _unsafe_mode: true is threaded into the llm_context passed to process_message()/stream_message(), where LLMAgent._resolve_parent_unsafe() already knew how to read it. Added 4 Behave scenarios covering refusal and propagation for both execute() and execute_stream() on a top-level type: llm actor.

Streaming graph path lacks unsafe-mode coverage — Confirmed as a test-coverage gap only, not a code bug: _execute_graph_stream already called _enforce_unsafe_flag() before this review. Added 2 Behave scenarios (refusal + propagation via execute_stream()) plus 2 Robot scenarios in robot/llm_tool_calling.robot ("...Via Execute Stream" variants), backed by a new execute_unsafe_host_message_stream keyword in ToolCallingTestLib.py that reuses the existing fixture setup.

Nits

Stale docstring in _execute_multi_actor — Fixed; the docstring now lists registry_api_key and the host unsafe flag among the fields forwarded to the sub-Executor.

Not changed

Nothing from this review was left unaddressed — all four findings map to a fix or test addition above.

Verification run locally (all via nox)

  • lint, format -- --check, typecheck, security_scan, dead_code — clean
  • unit_tests — 3033/3033 Behave scenarios pass (12 new, 0 regressions)
  • coverage_report — 96.6% (≥96.5% threshold; unchanged from the PR description baseline)
  • integration_tests — 11/11 Robot scenarios in llm_tool_calling.robot pass, including the 2 new streaming ones

The commit was amended in place rather than a new one added, per this repo's one-issue/one-commit convention.

Separate observation, unrelated to this review and left untouched: the PR currently shows mergeable: false against current master, due to an unrelated CHANGELOG.md conflict with other PRs merged since this branch's base commit. Flagging for visibility only.

Thanks for the review — all four findings check out against the spec (`docs/index.md` §9.4/§10.3) and the code, and have been addressed in the amended commit (`ac50b16` → `5e27912`, force-pushed to this branch). ### Critical Issues None — no action needed. ### Major Issues **Host `unsafe` flag lost across multi-actor sub-Executors** — Confirmed, fixed. `_execute_multi_actor` (`src/cleveractors/runtime_dispatch.py`) now passes `unsafe=executor.unsafe` and `registry_api_key=executor.registry_api_key` when constructing the sub-`Executor`. Checked against `Executor.__init__`, which defaults both to `False`/`None` — so the sub-Executor previously always ran as if the host were safe, regardless of the parent's actual state, exactly as you described. Added two Behave scenarios in `features/runtime_unsafe_host_mode.feature` (propagation success + refusal) driving a multi-actor bundle whose selected sub-actor is an unsafe-declaring graph actor. ### Minor Issues **Single LLM actor path ignores `context.global.unsafe`** — Confirmed, fixed. `_execute_llm` and `_execute_llm_stream` now build `global_context` from the actor's `context.global` section and call `_enforce_unsafe_flag()`, mirroring `_execute_graph`/`_execute_graph_stream` exactly. When the host is unsafe and the actor declares it, `_unsafe_mode: true` is threaded into the `llm_context` passed to `process_message()`/`stream_message()`, where `LLMAgent._resolve_parent_unsafe()` already knew how to read it. Added 4 Behave scenarios covering refusal and propagation for both `execute()` and `execute_stream()` on a top-level `type: llm` actor. **Streaming graph path lacks unsafe-mode coverage** — Confirmed as a test-coverage gap only, not a code bug: `_execute_graph_stream` already called `_enforce_unsafe_flag()` before this review. Added 2 Behave scenarios (refusal + propagation via `execute_stream()`) plus 2 Robot scenarios in `robot/llm_tool_calling.robot` ("...Via Execute Stream" variants), backed by a new `execute_unsafe_host_message_stream` keyword in `ToolCallingTestLib.py` that reuses the existing fixture setup. ### Nits **Stale docstring in `_execute_multi_actor`** — Fixed; the docstring now lists `registry_api_key` and the host `unsafe` flag among the fields forwarded to the sub-Executor. ### Not changed Nothing from this review was left unaddressed — all four findings map to a fix or test addition above. ### Verification run locally (all via `nox`) - `lint`, `format -- --check`, `typecheck`, `security_scan`, `dead_code` — clean - `unit_tests` — 3033/3033 Behave scenarios pass (12 new, 0 regressions) - `coverage_report` — 96.6% (≥96.5% threshold; unchanged from the PR description baseline) - `integration_tests` — 11/11 Robot scenarios in `llm_tool_calling.robot` pass, including the 2 new streaming ones The commit was amended in place rather than a new one added, per this repo's one-issue/one-commit convention. Separate observation, unrelated to this review and left untouched: the PR currently shows `mergeable: false` against current `master`, due to an unrelated `CHANGELOG.md` conflict with other PRs merged since this branch's base commit. Flagging for visibility only.
hurui200320 requested changes 2026-08-10 15:30:29 +00:00
Dismissed
hurui200320 left a comment

PR Review: !129 (Ticket #115)

Verdict: Request Changes

The issue #115 fixes are implemented correctly: LLMAgent._resolve_parent_unsafe() now reads safe_mode instead of the dead unsafe_mode key, the invocation context is threaded through _execute_tool_loop() and its synthesis mirrors, and Executor / create_executor() enforce context.global.unsafe across graph, single-LLM, multi-actor, and streaming paths. The four findings from the previous review cycle have all been addressed (multi-actor forwarding of unsafe/registry_api_key, single-LLM enforcement, streaming coverage, and the stale _execute_multi_actor docstring).

However, the branch is currently stale against master and the diff reverts unrelated recently-merged functionality. Merging as-is would regress agent/route package-reference resolution and delete associated docs. This needs a rebase / merge-conflict cleanup before approval.

Critical Issues

  • Stale branch reverts unrelated master changes
    • Files: src/cleveractors/runtime_dispatch.py, README.md, docs/index.md, docs/guides/reasoning-aware-llm-agents.md, docs/adr/ADR-2037-agent-and-route-package-reference-resolution.md, CHANGELOG.md
    • Problem: The PR is based on an older master. Relative to current master it removes the AgentReferenceResolver / RouteReferenceResolver wiring added by ADR-2037, strips the credentials documentation from README.md and the reasoning-aware guide, reverts docs/index.md from version 1.5.0 to 1.4.0, deletes ADR-2037, and removes the corresponding changelog entries. The author noted a CHANGELOG.md conflict, but the stale state also affects source code and other docs. Merging would break agent/route package references and leave documentation inconsistent.
    • Recommendation: Rebase this branch onto current origin/master and resolve conflicts so that the new unsafe / registry_api_key forwarding is layered on top of the ADR-2037 code. Preserve AgentReferenceResolver, RouteReferenceResolver, the docs version bump, README credentials docs, the ADR file, and the changelog entries for issues #121 / #123.

Major Issues

None.

Minor Issues

None.

Nits

  • Outdated docstring in _enforce_unsafe_flag()
    • File: src/cleveractors/runtime_dispatch.py (line 95)
    • Problem: The docstring says it enforces the contract "for a graph actor", but the helper is now also invoked for single type: llm actors in _execute_llm() and _execute_llm_stream(). Update the wording to reflect that it applies to any actor type.
    • Recommendation: Change the opening sentence to "Enforce the unsafe host contract for an actor (§9.4/§10.3)."

Summary

All of the previous review feedback has been resolved in the amended commit. The implementation correctly closes the two compounding bugs from #115 and adds good BDD/Robot coverage for the runtime host-unsafe contract. The only remaining blocker is cleaning up the stale branch state so the unrelated ADR-2037 and documentation changes are not reverted. After a clean rebase and green CI, this should be ready to approve.

## PR Review: !129 (Ticket #115) ### Verdict: Request Changes The issue #115 fixes are implemented correctly: `LLMAgent._resolve_parent_unsafe()` now reads `safe_mode` instead of the dead `unsafe_mode` key, the invocation `context` is threaded through `_execute_tool_loop()` and its synthesis mirrors, and `Executor` / `create_executor()` enforce `context.global.unsafe` across graph, single-LLM, multi-actor, and streaming paths. The four findings from the previous review cycle have all been addressed (multi-actor forwarding of `unsafe`/`registry_api_key`, single-LLM enforcement, streaming coverage, and the stale `_execute_multi_actor` docstring). However, the branch is currently stale against `master` and the diff reverts unrelated recently-merged functionality. Merging as-is would regress agent/route package-reference resolution and delete associated docs. This needs a rebase / merge-conflict cleanup before approval. ### Critical Issues - **Stale branch reverts unrelated `master` changes** - **Files:** `src/cleveractors/runtime_dispatch.py`, `README.md`, `docs/index.md`, `docs/guides/reasoning-aware-llm-agents.md`, `docs/adr/ADR-2037-agent-and-route-package-reference-resolution.md`, `CHANGELOG.md` - **Problem:** The PR is based on an older `master`. Relative to current `master` it removes the `AgentReferenceResolver` / `RouteReferenceResolver` wiring added by ADR-2037, strips the `credentials` documentation from `README.md` and the reasoning-aware guide, reverts `docs/index.md` from version 1.5.0 to 1.4.0, deletes ADR-2037, and removes the corresponding changelog entries. The author noted a `CHANGELOG.md` conflict, but the stale state also affects source code and other docs. Merging would break agent/route package references and leave documentation inconsistent. - **Recommendation:** Rebase this branch onto current `origin/master` and resolve conflicts so that the new `unsafe` / `registry_api_key` forwarding is layered on top of the ADR-2037 code. Preserve `AgentReferenceResolver`, `RouteReferenceResolver`, the docs version bump, README credentials docs, the ADR file, and the changelog entries for issues #121 / #123. ### Major Issues None. ### Minor Issues None. ### Nits - **Outdated docstring in `_enforce_unsafe_flag()`** - **File:** `src/cleveractors/runtime_dispatch.py` (line 95) - **Problem:** The docstring says it enforces the contract "for a graph actor", but the helper is now also invoked for single `type: llm` actors in `_execute_llm()` and `_execute_llm_stream()`. Update the wording to reflect that it applies to any actor type. - **Recommendation:** Change the opening sentence to "Enforce the `unsafe` host contract for an actor (§9.4/§10.3)." ### Summary All of the previous review feedback has been resolved in the amended commit. The implementation correctly closes the two compounding bugs from #115 and adds good BDD/Robot coverage for the runtime host-unsafe contract. The only remaining blocker is cleaning up the stale branch state so the unrelated ADR-2037 and documentation changes are not reverted. After a clean rebase and green CI, this should be ready to approve.
CoreRasurae force-pushed bugfix/m1-llm-tool-unsafe-mode from 5e27912e59
Some checks failed
CI / lint (pull_request) Successful in 1m8s
CI / typecheck (pull_request) Successful in 1m30s
CI / security (pull_request) Successful in 1m18s
CI / quality (pull_request) Successful in 1m24s
CI / build (pull_request) Successful in 1m53s
CI / integration_tests (pull_request) Successful in 4m35s
CI / unit_tests (pull_request) Successful in 6m9s
CI / coverage (pull_request) Successful in 4m5s
CI / status-check (pull_request) Successful in 6s
CI / benchmark (pull_request) Failing after 26m8s
to e0b20a0165
Some checks failed
CI / lint (pull_request) Has started running
CI / typecheck (pull_request) Has started running
CI / security (pull_request) Has started running
CI / unit_tests (pull_request) Has started running
CI / coverage (pull_request) Has been cancelled
CI / build (pull_request) Has been cancelled
CI / benchmark (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
CI / integration_tests (pull_request) Has been cancelled
CI / quality (pull_request) Has been cancelled
2026-08-15 11:08:15 +00:00
Compare
CoreRasurae force-pushed bugfix/m1-llm-tool-unsafe-mode from e0b20a0165
Some checks failed
CI / lint (pull_request) Has started running
CI / typecheck (pull_request) Has started running
CI / security (pull_request) Has started running
CI / unit_tests (pull_request) Has started running
CI / coverage (pull_request) Has been cancelled
CI / build (pull_request) Has been cancelled
CI / benchmark (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
CI / integration_tests (pull_request) Has been cancelled
CI / quality (pull_request) Has been cancelled
to 9b72919720
Some checks failed
CI / lint (pull_request) Failing after 7m6s
CI / typecheck (pull_request) Failing after 7m20s
CI / security (pull_request) Failing after 7m18s
CI / quality (pull_request) Failing after 6m39s
CI / build (pull_request) Failing after 6m39s
CI / integration_tests (pull_request) Failing after 8m38s
CI / unit_tests (pull_request) Failing after 10m30s
CI / coverage (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 9s
CI / benchmark (pull_request) Failing after 30m55s
2026-08-15 11:22:36 +00:00
Compare
Author
Member

Thanks — addressed in the amended commit (5e279129b72919, force-pushed).

Critical Issues

Stale branch reverts unrelated master changes — Resolved by rebasing onto current master. HEAD~1 is now identical to origin/master (single commit ahead), so AgentReferenceResolver/RouteReferenceResolver (ADR-2037), the README credentials docs, and the CHANGELOG entries for #121/#123/#131 are all preserved untouched — verified git diff origin/master -- README.md docs/index.md docs/adr/ is empty.

Nits

Outdated docstring in _enforce_unsafe_flag() — Fixed per your suggested wording: opening line now reads "Enforce the unsafe host contract for an actor (§9.4/§10.3)." Also updated the body/Args text that still said "graph"/"the graph's seed context" for the same reason, since the helper is exercised by the graph, single-LLM, and multi-actor sub-Executor paths alike.

Verification (via nox)

  • lint, typecheck — clean
  • unit_tests — 3075/3075 scenarios pass (full suite, 0 regressions)
  • Scoped re-run of the pertinent feature files (llm_agent_tool_calling.feature, llm_agent_tool_loop.feature, runtime_unsafe_host_mode.feature) — 88/88 scenarios pass

This is a docstring-only change (no executable lines touched), so no coverage delta and no Robot/benchmark re-run were needed.

Thanks — addressed in the amended commit (`5e27912` → `9b72919`, force-pushed). ### Critical Issues **Stale branch reverts unrelated `master` changes** — Resolved by rebasing onto current `master`. `HEAD~1` is now identical to `origin/master` (single commit ahead), so `AgentReferenceResolver`/`RouteReferenceResolver` (ADR-2037), the README credentials docs, and the CHANGELOG entries for #121/#123/#131 are all preserved untouched — verified `git diff origin/master -- README.md docs/index.md docs/adr/` is empty. ### Nits **Outdated docstring in `_enforce_unsafe_flag()`** — Fixed per your suggested wording: opening line now reads "Enforce the `unsafe` host contract for an actor (§9.4/§10.3)." Also updated the body/Args text that still said "graph"/"the graph's seed context" for the same reason, since the helper is exercised by the graph, single-LLM, and multi-actor sub-Executor paths alike. ### Verification (via `nox`) - `lint`, `typecheck` — clean - `unit_tests` — 3075/3075 scenarios pass (full suite, 0 regressions) - Scoped re-run of the pertinent feature files (`llm_agent_tool_calling.feature`, `llm_agent_tool_loop.feature`, `runtime_unsafe_host_mode.feature`) — 88/88 scenarios pass This is a docstring-only change (no executable lines touched), so no coverage delta and no Robot/benchmark re-run were needed.
hurui200320 left a comment

PR Review: !129 (Ticket #115)

Verdict: Approve

The implementation correctly fixes the two compounding bugs described in #115: LLMAgent._execute_tool_loop() now derives parent_unsafe from the real safe_mode field and/or a host-propagated _unsafe_mode, and the Executor/create_executor() runtime path enforces the context.global.unsafe host contract across graph, single-LLM, streaming, and multi-actor dispatch. The test suite is comprehensive (Behave + Robot) and the previous review findings have been addressed. No critical or major issues remain.

Critical Issues

None.

Major Issues

None.

Minor Issues

  1. src/cleveractors/runtime_dispatch.py_execute_tool() does not enforce the host unsafe contract

    • The new Executor.unsafe flag and _enforce_unsafe_flag() helper are wired into _execute_graph, _execute_graph_stream, _execute_llm, _execute_llm_stream, and the _execute_multi_actor sub-Executor, but _execute_tool() (single type: tool actor path) is skipped. A tool actor declaring context.global.unsafe: true will therefore not be refused on a safe host, and _unsafe_mode is never injected into its invocation context (the call is agent.process_message(message) with no context).
    • Recommendation: Apply _enforce_unsafe_flag() in _execute_tool() and pass the resulting context to agent.process_message(message, context=...). This is likely out of scope for the LLM-specific #115, but it leaves a spec §9.4 compliance gap on the Executor path.
  2. features/runtime_unsafe_host_mode.feature — missing default single-LLM regression scenario

    • The feature has a strong no-regression scenario for graph actors ("Default graph actor with no unsafe declaration keeps file_write blocked"), but there is no equivalent for a single type: llm actor with no context.global.unsafe declaration. The _execute_llm path is distinct from _execute_graph, so the default safe-mode blocking behavior for that shape is not exercised end-to-end here.
    • Recommendation: Add a scenario using _single_llm_config(declare_unsafe=False) and assert the write is blocked.
  3. robot/ToolCallingTestLib.py — temporary directories are not cleaned up

    • _create_graph_executor_for_unsafe_host_test() creates target_dir = Path(tempfile.mkdtemp(...)) but never removes it, so each Robot scenario leaks a temp directory.
    • Recommendation: Register a cleanup/teardown that removes self._unsafe_host_target_path.parent after assertions, or use a context manager / tempfile.TemporaryDirectory.

Nits

  • noxfile.py — stale coverage threshold docstring
    • The coverage_report() docstring says "Coverage threshold is enforced at >=97.0%", but COVERAGE_THRESHOLD = 96.5. This is a pre-existing inconsistency, not introduced by this PR, but it adds confusion when the issue DoD mentions 97% while the project gate is 96.5%.
    • Recommendation: Align the docstring with the constant (or vice versa).

Summary

This is a solid, focused fix. The root cause analysis in #115 is accurate, the new _resolve_parent_unsafe() helper is the right seam, and threading the invocation context through _execute_tool_loop() cleanly covers all three dispatch sites (main loop + budget/stuck-model synthesis mirrors). The Executor(unsafe=...) design mirrors Application's --unsafe flag consistently, and forwarding both unsafe and registry_api_key through the multi-actor sub-Executor fixes a real privilege-leak bug. Tests are well-structured, assertions validate actual file I/O outcomes, and the previous review cycle's findings have all been addressed. Approved with the minor notes above.

## PR Review: !129 (Ticket #115) ### Verdict: Approve The implementation correctly fixes the two compounding bugs described in #115: `LLMAgent._execute_tool_loop()` now derives `parent_unsafe` from the real `safe_mode` field and/or a host-propagated `_unsafe_mode`, and the `Executor`/`create_executor()` runtime path enforces the `context.global.unsafe` host contract across graph, single-LLM, streaming, and multi-actor dispatch. The test suite is comprehensive (Behave + Robot) and the previous review findings have been addressed. No critical or major issues remain. ### Critical Issues None. ### Major Issues None. ### Minor Issues 1. **`src/cleveractors/runtime_dispatch.py` — `_execute_tool()` does not enforce the host `unsafe` contract** - The new `Executor.unsafe` flag and `_enforce_unsafe_flag()` helper are wired into `_execute_graph`, `_execute_graph_stream`, `_execute_llm`, `_execute_llm_stream`, and the `_execute_multi_actor` sub-Executor, but `_execute_tool()` (single `type: tool` actor path) is skipped. A tool actor declaring `context.global.unsafe: true` will therefore not be refused on a safe host, and `_unsafe_mode` is never injected into its invocation context (the call is `agent.process_message(message)` with no context). - **Recommendation:** Apply `_enforce_unsafe_flag()` in `_execute_tool()` and pass the resulting context to `agent.process_message(message, context=...)`. This is likely out of scope for the LLM-specific #115, but it leaves a spec §9.4 compliance gap on the Executor path. 2. **`features/runtime_unsafe_host_mode.feature` — missing default single-LLM regression scenario** - The feature has a strong no-regression scenario for graph actors ("Default graph actor with no unsafe declaration keeps file_write blocked"), but there is no equivalent for a single `type: llm` actor with no `context.global.unsafe` declaration. The `_execute_llm` path is distinct from `_execute_graph`, so the default safe-mode blocking behavior for that shape is not exercised end-to-end here. - **Recommendation:** Add a scenario using `_single_llm_config(declare_unsafe=False)` and assert the write is blocked. 3. **`robot/ToolCallingTestLib.py` — temporary directories are not cleaned up** - `_create_graph_executor_for_unsafe_host_test()` creates `target_dir = Path(tempfile.mkdtemp(...))` but never removes it, so each Robot scenario leaks a temp directory. - **Recommendation:** Register a cleanup/teardown that removes `self._unsafe_host_target_path.parent` after assertions, or use a context manager / `tempfile.TemporaryDirectory`. ### Nits - **`noxfile.py` — stale coverage threshold docstring** - The `coverage_report()` docstring says "Coverage threshold is enforced at >=97.0%", but `COVERAGE_THRESHOLD = 96.5`. This is a pre-existing inconsistency, not introduced by this PR, but it adds confusion when the issue DoD mentions 97% while the project gate is 96.5%. - **Recommendation:** Align the docstring with the constant (or vice versa). ### Summary This is a solid, focused fix. The root cause analysis in #115 is accurate, the new `_resolve_parent_unsafe()` helper is the right seam, and threading the invocation `context` through `_execute_tool_loop()` cleanly covers all three dispatch sites (main loop + budget/stuck-model synthesis mirrors). The `Executor(unsafe=...)` design mirrors `Application`'s `--unsafe` flag consistently, and forwarding both `unsafe` and `registry_api_key` through the multi-actor sub-Executor fixes a real privilege-leak bug. Tests are well-structured, assertions validate actual file I/O outcomes, and the previous review cycle's findings have all been addressed. Approved with the minor notes above.
CoreRasurae force-pushed bugfix/m1-llm-tool-unsafe-mode from 9b72919720
Some checks failed
CI / lint (pull_request) Failing after 7m6s
CI / typecheck (pull_request) Failing after 7m20s
CI / security (pull_request) Failing after 7m18s
CI / quality (pull_request) Failing after 6m39s
CI / build (pull_request) Failing after 6m39s
CI / integration_tests (pull_request) Failing after 8m38s
CI / unit_tests (pull_request) Failing after 10m30s
CI / coverage (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 9s
CI / benchmark (pull_request) Failing after 30m55s
to 757a607c81
Some checks failed
CI / lint (pull_request) Successful in 1m30s
CI / typecheck (pull_request) Successful in 1m39s
CI / security (pull_request) Successful in 1m43s
CI / quality (pull_request) Successful in 1m8s
CI / unit_tests (pull_request) Successful in 5m24s
CI / integration_tests (pull_request) Successful in 3m37s
CI / build (pull_request) Successful in 1m4s
CI / coverage (pull_request) Successful in 5m21s
CI / status-check (pull_request) Successful in 5s
CI / benchmark (pull_request) Failing after 26m53s
2026-08-20 14:56:01 +00:00
Compare
CoreRasurae force-pushed bugfix/m1-llm-tool-unsafe-mode from 757a607c81
Some checks failed
CI / lint (pull_request) Successful in 1m30s
CI / typecheck (pull_request) Successful in 1m39s
CI / security (pull_request) Successful in 1m43s
CI / quality (pull_request) Successful in 1m8s
CI / unit_tests (pull_request) Successful in 5m24s
CI / integration_tests (pull_request) Successful in 3m37s
CI / build (pull_request) Successful in 1m4s
CI / coverage (pull_request) Successful in 5m21s
CI / status-check (pull_request) Successful in 5s
CI / benchmark (pull_request) Failing after 26m53s
to de12f778df
Some checks failed
CI / lint (pull_request) Failing after 1m8s
CI / security (pull_request) Successful in 1m48s
CI / unit_tests (pull_request) Has started running
CI / integration_tests (pull_request) Has started running
CI / typecheck (pull_request) Successful in 3m7s
CI / build (pull_request) Has started running
CI / benchmark (pull_request) Has started running
CI / quality (pull_request) Successful in 1m11s
CI / coverage (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
2026-08-20 17:11:19 +00:00
Compare
Author
Member

Thanks for the approval and the minor notes — addressed in the amended commit (9b72919 -> de12f77, force-pushed).

Minor Issues

  1. _execute_tool() does not enforce the host unsafe contract — Confirmed against spec (§9.4: "An Actor MAY declare that it requires unsafe mode" is not graph/LLM-specific) and fixed. _execute_tool now builds global_context from the actor's context.global section and calls _enforce_unsafe_flag(), mirroring _execute_graph/_execute_llm/_execute_multi_actor exactly: refused with UnsafeConfigurationError on a safe host, and _unsafe_mode: true passed to ToolAgent.process_message(message, context=...) on an unsafe host (only when actually needed, so unrelated test doubles that never expected a second argument are unaffected). You noted this was "likely out of scope for the LLM-specific #115" and might warrant a separate PR/issue — kept it in this PR instead, since it closes the same host-unsafe-mode contract this PR already introduces for every other dispatch path, rather than leaving one path spec-non-compliant in the interim. Added 3 new Behave scenarios (refusal, propagation, no-regression) in features/runtime_unsafe_host_mode.feature for this path.

  2. Missing default single-LLM regression scenario — Added: "Default single type:llm actor with no unsafe declaration keeps file_write blocked (no regression)" using _single_llm_config(declare_unsafe=False), mirroring the existing graph-actor no-regression scenario.

  3. robot/ToolCallingTestLib.py temp directories not cleaned up — Fixed: added _cleanup_unsafe_host_tempdir(), called from all three Then-keywords (unsafe_configuration_error_should_have_been_raised, file_write_should_have_succeeded, file_write_should_have_been_blocked) after their assertions run, so the robot_unsafe_host_* temp dir is removed once the scenario no longer needs it. Verified no leaked directories after a full nox -s integration_tests run.

Nits

  • noxfile.py stale coverage threshold docstring — Not changed. Confirmed with the maintainer: the docstring's "97.0%" is an intentional rounded display value, while COVERAGE_THRESHOLD = 96.5 is the real enforced gate — not the inconsistency it appeared to be, so no edit was made.

Verification (via nox)

  • nox -s lint, nox -s typecheck — clean
  • nox -s unit_tests — 3079/3079 Behave scenarios pass (0 regressions; includes the new scenarios above)
  • nox -s integration_tests — 362/362 Robot scenarios pass (pabot), no leaked temp dirs
  • nox -s coverage_report — 96.9% (≥ 96.5% threshold); the only uncovered line touched by this round (runtime_dispatch.py, the elif actor_context: flat-context fallback in the new _execute_tool block) mirrors an already-untested, pre-existing defensive branch duplicated identically in _execute_llm/_execute_graph — not a new gap.

CHANGELOG.md's issue #115 entry was rewritten to a single concise, user-oriented sentence (previously a multi-paragraph technical write-up) to match this project's actual CONTRIBUTING convention rather than the file's pre-existing long-form style.

All fixes were amended into the existing single commit per this repo's one-issue/one-commit convention (no new commit added).

Thanks for the approval and the minor notes — addressed in the amended commit (`9b72919` -> `de12f77`, force-pushed). ### Minor Issues 1. **`_execute_tool()` does not enforce the host `unsafe` contract** — Confirmed against spec (§9.4: "An Actor MAY declare that it requires unsafe mode" is not graph/LLM-specific) and fixed. `_execute_tool` now builds `global_context` from the actor's `context.global` section and calls `_enforce_unsafe_flag()`, mirroring `_execute_graph`/`_execute_llm`/`_execute_multi_actor` exactly: refused with `UnsafeConfigurationError` on a safe host, and `_unsafe_mode: true` passed to `ToolAgent.process_message(message, context=...)` on an unsafe host (only when actually needed, so unrelated test doubles that never expected a second argument are unaffected). You noted this was "likely out of scope for the LLM-specific #115" and might warrant a separate PR/issue — kept it in this PR instead, since it closes the same host-unsafe-mode contract this PR already introduces for every other dispatch path, rather than leaving one path spec-non-compliant in the interim. Added 3 new Behave scenarios (refusal, propagation, no-regression) in `features/runtime_unsafe_host_mode.feature` for this path. 2. **Missing default single-LLM regression scenario** — Added: "Default single type:llm actor with no unsafe declaration keeps file_write blocked (no regression)" using `_single_llm_config(declare_unsafe=False)`, mirroring the existing graph-actor no-regression scenario. 3. **`robot/ToolCallingTestLib.py` temp directories not cleaned up** — Fixed: added `_cleanup_unsafe_host_tempdir()`, called from all three Then-keywords (`unsafe_configuration_error_should_have_been_raised`, `file_write_should_have_succeeded`, `file_write_should_have_been_blocked`) after their assertions run, so the `robot_unsafe_host_*` temp dir is removed once the scenario no longer needs it. Verified no leaked directories after a full `nox -s integration_tests` run. ### Nits - **`noxfile.py` stale coverage threshold docstring** — Not changed. Confirmed with the maintainer: the docstring's "97.0%" is an intentional rounded display value, while `COVERAGE_THRESHOLD = 96.5` is the real enforced gate — not the inconsistency it appeared to be, so no edit was made. ### Verification (via `nox`) - `nox -s lint`, `nox -s typecheck` — clean - `nox -s unit_tests` — 3079/3079 Behave scenarios pass (0 regressions; includes the new scenarios above) - `nox -s integration_tests` — 362/362 Robot scenarios pass (pabot), no leaked temp dirs - `nox -s coverage_report` — 96.9% (≥ 96.5% threshold); the only uncovered line touched by this round (`runtime_dispatch.py`, the `elif actor_context:` flat-context fallback in the new `_execute_tool` block) mirrors an already-untested, pre-existing defensive branch duplicated identically in `_execute_llm`/`_execute_graph` — not a new gap. CHANGELOG.md's issue #115 entry was rewritten to a single concise, user-oriented sentence (previously a multi-paragraph technical write-up) to match this project's actual CONTRIBUTING convention rather than the file's pre-existing long-form style. All fixes were amended into the existing single commit per this repo's one-issue/one-commit convention (no new commit added).
CoreRasurae force-pushed bugfix/m1-llm-tool-unsafe-mode from de12f778df
Some checks failed
CI / lint (pull_request) Failing after 1m8s
CI / security (pull_request) Successful in 1m48s
CI / unit_tests (pull_request) Has started running
CI / integration_tests (pull_request) Has started running
CI / typecheck (pull_request) Successful in 3m7s
CI / build (pull_request) Has started running
CI / benchmark (pull_request) Has started running
CI / quality (pull_request) Successful in 1m11s
CI / coverage (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
to 3e5cdee737
Some checks failed
CI / benchmark (pull_request) Has started running
CI / quality (pull_request) Successful in 1m4s
CI / build (pull_request) Successful in 57s
CI / lint (pull_request) Successful in 2m1s
CI / security (pull_request) Successful in 2m17s
CI / typecheck (pull_request) Successful in 2m28s
CI / integration_tests (pull_request) Successful in 3m3s
CI / unit_tests (pull_request) Successful in 4m26s
CI / coverage (pull_request) Successful in 5m40s
CI / status-check (pull_request) Successful in 6s
CI / lint (push) Successful in 1m9s
CI / security (push) Successful in 1m37s
CI / quality (push) Successful in 2m21s
CI / build (push) Successful in 2m32s
CI / unit_tests (push) Successful in 5m14s
CI / typecheck (push) Successful in 3m30s
CI / benchmark (push) Has been cancelled
CI / integration_tests (push) Successful in 3m15s
CI / coverage (push) Successful in 7m17s
CI / status-check (push) Successful in 4s
2026-08-20 17:15:56 +00:00
Compare
CoreRasurae deleted branch bugfix/m1-llm-tool-unsafe-mode 2026-08-20 17:32:42 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
cleveragents/cleveractors-core!129
No description provided.