fix(agents): honor safe_mode and context.global.unsafe for LLM-agent tool calls #129
No reviewers
Labels
No labels
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Blocks
#115 LLM agent file_write calls are always blocked — safe_mode: false / context.global.unsafe: true never reach _unsafe_mode
cleveragents/cleveractors-core
Reference
cleveragents/cleveractors-core!129
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bugfix/m1-llm-tool-unsafe-mode"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Fixes two compounding bugs that unconditionally blocked every
file_write(and other tool) call issued by atype: llmagent's tool-call loop, regardless of actor configuration:LLMAgent._execute_tool_loop()(and its two synthesis-retry mirrors) derivedparent_unsafefrom a deadunsafe_modeconfig key that nothing in the codebase ever wrote, instead of the actualsafe_modefield an actor author sets (docs/index.md §4.5). Fixed via a newLLMAgent._resolve_parent_unsafe()helper, and by threading the invocationcontextthrough_execute_tool_loop()so it can also see a host-propagated_unsafe_mode: true.cleveractors.runtime_dispatch._execute_graph/_execute_graph_stream(theExecutor/create_executor()path) had no host-unsafe-flag concept at all, so an actor declaringcontext.global.unsafe: true(§9.4) was neither refused on an unsafe host nor granted_unsafe_modepropagation when the host was unsafe. Fixed by addingExecutor(unsafe=...)/create_executor(unsafe=...)and a newruntime_dispatch._enforce_unsafe_flag()helper mirroringApplication._enforce_unsafe_flag.Default
safe_mode: truesandboxing forfile_read/shell/directory traversal is unaffected.Test plan
features/llm_agent_tool_loop.feature—@tdd_issue_115scenario now passes without@tdd_expected_fail(LLM agentsafe_mode: falsepath)features/runtime_unsafe_host_mode.feature(new) —context.global.unsafehost-mode path: refusal, propagation, and no-regression scenarios, exercised end-to-end throughcreate_executor()→ real graph → realLLMAgent/ToolAgent(only the LangChain network boundary is mocked)robot/llm_tool_calling.robot(extended) — same three scenarios via Robot integration test, real file I/Onox -s unit_tests— 3021/3021 scenarios passingnox -s coverage_report— 96.6% (threshold 96.5%)nox -s integration_tests— 355/355 passingnox -s lint,nox -s format -- --check,nox -s typecheck,nox -s security_scan,nox -s dead_code— all cleanCloses #115
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
LLMAgenttool-call path. The code is well-typed, the new helpers are clearly documented, and the existing scenarios that relied on the deadunsafe_modekey have been updated appropriately.One major functional gap remains: the new host
unsafeflag 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
unsafeflag is lost across multi-actor sub-Executorssrc/cleveractors/runtime_dispatch.py(lines 736–743)_execute_multi_actorcreates a childExecutorbut does not passunsafe=executor.unsafe(orregistry_api_key=executor.registry_api_key). Consequently, a multi-actor bundle whose selected sub-actor is a graph declaringcontext.global.unsafe: truewill raiseUnsafeConfigurationErroreven when the parent host was placed in unsafe mode. This breaks the host-mode contract that the PR is introducing.unsafe=executor.unsafeandregistry_api_key=executor.registry_api_keywhen 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.unsafesrc/cleveractors/runtime_dispatch.py(_execute_llm,_execute_llm_stream)_enforce_unsafe_flagis only wired into_execute_graphand_execute_graph_stream. A singletype: llmactor config that declarescontext.global.unsafe: trueis neither refused on a safe host nor granted_unsafe_modeon an unsafe host, leaving a spec-compliance gap outside the graph path._execute_llmand_execute_llm_stream, or explicitly document thatcontext.global.unsafeis only honored by graph actors.Streaming graph path lacks unsafe-mode coverage
features/runtime_unsafe_host_mode.feature,robot/llm_tool_calling.robot_execute_graph_streamnow calls_enforce_unsafe_flag, but no new scenario exercises refusal or propagation throughExecutor.execute_stream().Nits
_execute_multi_actorsrc/cleveractors/runtime_dispatch.py(line 703)unsafe(andregistry_api_key) should also be propagated.Summary
The core fix is sound and the tests directly validate the reported
file_writeblocking bug. The missingunsafepropagation in_execute_multi_actoris the only blocker; once that is fixed and covered by a test, this PR should be ready to approve.ac50b16b095e27912e59Thanks 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
unsafeflag lost across multi-actor sub-Executors — Confirmed, fixed._execute_multi_actor(src/cleveractors/runtime_dispatch.py) now passesunsafe=executor.unsafeandregistry_api_key=executor.registry_api_keywhen constructing the sub-Executor. Checked againstExecutor.__init__, which defaults both toFalse/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 infeatures/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_llmand_execute_llm_streamnow buildglobal_contextfrom the actor'scontext.globalsection and call_enforce_unsafe_flag(), mirroring_execute_graph/_execute_graph_streamexactly. When the host is unsafe and the actor declares it,_unsafe_mode: trueis threaded into thellm_contextpassed toprocess_message()/stream_message(), whereLLMAgent._resolve_parent_unsafe()already knew how to read it. Added 4 Behave scenarios covering refusal and propagation for bothexecute()andexecute_stream()on a top-leveltype: llmactor.Streaming graph path lacks unsafe-mode coverage — Confirmed as a test-coverage gap only, not a code bug:
_execute_graph_streamalready called_enforce_unsafe_flag()before this review. Added 2 Behave scenarios (refusal + propagation viaexecute_stream()) plus 2 Robot scenarios inrobot/llm_tool_calling.robot("...Via Execute Stream" variants), backed by a newexecute_unsafe_host_message_streamkeyword inToolCallingTestLib.pythat reuses the existing fixture setup.Nits
Stale docstring in
_execute_multi_actor— Fixed; the docstring now listsregistry_api_keyand the hostunsafeflag 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— cleanunit_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 inllm_tool_calling.robotpass, including the 2 new streaming onesThe 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: falseagainst currentmaster, due to an unrelatedCHANGELOG.mdconflict with other PRs merged since this branch's base commit. Flagging for visibility only.PR Review: !129 (Ticket #115)
Verdict: Request Changes
The issue #115 fixes are implemented correctly:
LLMAgent._resolve_parent_unsafe()now readssafe_modeinstead of the deadunsafe_modekey, the invocationcontextis threaded through_execute_tool_loop()and its synthesis mirrors, andExecutor/create_executor()enforcecontext.global.unsafeacross graph, single-LLM, multi-actor, and streaming paths. The four findings from the previous review cycle have all been addressed (multi-actor forwarding ofunsafe/registry_api_key, single-LLM enforcement, streaming coverage, and the stale_execute_multi_actordocstring).However, the branch is currently stale against
masterand 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
masterchangessrc/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.mdmaster. Relative to currentmasterit removes theAgentReferenceResolver/RouteReferenceResolverwiring added by ADR-2037, strips thecredentialsdocumentation fromREADME.mdand the reasoning-aware guide, revertsdocs/index.mdfrom version 1.5.0 to 1.4.0, deletes ADR-2037, and removes the corresponding changelog entries. The author noted aCHANGELOG.mdconflict, but the stale state also affects source code and other docs. Merging would break agent/route package references and leave documentation inconsistent.origin/masterand resolve conflicts so that the newunsafe/registry_api_keyforwarding is layered on top of the ADR-2037 code. PreserveAgentReferenceResolver,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
_enforce_unsafe_flag()src/cleveractors/runtime_dispatch.py(line 95)type: llmactors in_execute_llm()and_execute_llm_stream(). Update the wording to reflect that it applies to any actor type.unsafehost 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.
5e27912e59e0b20a0165e0b20a01659b72919720Thanks — addressed in the amended commit (
5e27912→9b72919, force-pushed).Critical Issues
Stale branch reverts unrelated
masterchanges — Resolved by rebasing onto currentmaster.HEAD~1is now identical toorigin/master(single commit ahead), soAgentReferenceResolver/RouteReferenceResolver(ADR-2037), the README credentials docs, and the CHANGELOG entries for #121/#123/#131 are all preserved untouched — verifiedgit 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 theunsafehost 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— cleanunit_tests— 3075/3075 scenarios pass (full suite, 0 regressions)llm_agent_tool_calling.feature,llm_agent_tool_loop.feature,runtime_unsafe_host_mode.feature) — 88/88 scenarios passThis is a docstring-only change (no executable lines touched), so no coverage delta and no Robot/benchmark re-run were needed.
PR Review: !129 (Ticket #115)
Verdict: Approve
The implementation correctly fixes the two compounding bugs described in #115:
LLMAgent._execute_tool_loop()now derivesparent_unsafefrom the realsafe_modefield and/or a host-propagated_unsafe_mode, and theExecutor/create_executor()runtime path enforces thecontext.global.unsafehost 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
src/cleveractors/runtime_dispatch.py—_execute_tool()does not enforce the hostunsafecontractExecutor.unsafeflag and_enforce_unsafe_flag()helper are wired into_execute_graph,_execute_graph_stream,_execute_llm,_execute_llm_stream, and the_execute_multi_actorsub-Executor, but_execute_tool()(singletype: toolactor path) is skipped. A tool actor declaringcontext.global.unsafe: truewill therefore not be refused on a safe host, and_unsafe_modeis never injected into its invocation context (the call isagent.process_message(message)with no context)._enforce_unsafe_flag()in_execute_tool()and pass the resulting context toagent.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.features/runtime_unsafe_host_mode.feature— missing default single-LLM regression scenariotype: llmactor with nocontext.global.unsafedeclaration. The_execute_llmpath is distinct from_execute_graph, so the default safe-mode blocking behavior for that shape is not exercised end-to-end here._single_llm_config(declare_unsafe=False)and assert the write is blocked.robot/ToolCallingTestLib.py— temporary directories are not cleaned up_create_graph_executor_for_unsafe_host_test()createstarget_dir = Path(tempfile.mkdtemp(...))but never removes it, so each Robot scenario leaks a temp directory.self._unsafe_host_target_path.parentafter assertions, or use a context manager /tempfile.TemporaryDirectory.Nits
noxfile.py— stale coverage threshold docstringcoverage_report()docstring says "Coverage threshold is enforced at >=97.0%", butCOVERAGE_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%.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 invocationcontextthrough_execute_tool_loop()cleanly covers all three dispatch sites (main loop + budget/stuck-model synthesis mirrors). TheExecutor(unsafe=...)design mirrorsApplication's--unsafeflag consistently, and forwarding bothunsafeandregistry_api_keythrough 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.9b72919720757a607c81757a607c81de12f778dfThanks for the approval and the minor notes — addressed in the amended commit (
9b72919->de12f77, force-pushed).Minor Issues
_execute_tool()does not enforce the hostunsafecontract — Confirmed against spec (§9.4: "An Actor MAY declare that it requires unsafe mode" is not graph/LLM-specific) and fixed._execute_toolnow buildsglobal_contextfrom the actor'scontext.globalsection and calls_enforce_unsafe_flag(), mirroring_execute_graph/_execute_llm/_execute_multi_actorexactly: refused withUnsafeConfigurationErroron a safe host, and_unsafe_mode: truepassed toToolAgent.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) infeatures/runtime_unsafe_host_mode.featurefor this path.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.robot/ToolCallingTestLib.pytemp 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 therobot_unsafe_host_*temp dir is removed once the scenario no longer needs it. Verified no leaked directories after a fullnox -s integration_testsrun.Nits
noxfile.pystale coverage threshold docstring — Not changed. Confirmed with the maintainer: the docstring's "97.0%" is an intentional rounded display value, whileCOVERAGE_THRESHOLD = 96.5is 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— cleannox -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 dirsnox -s coverage_report— 96.9% (≥ 96.5% threshold); the only uncovered line touched by this round (runtime_dispatch.py, theelif actor_context:flat-context fallback in the new_execute_toolblock) 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).
de12f778df3e5cdee737