cleveragents/cleveractors-core

Fork 0

test(agents): capture LLM-agent file_write unsafe-mode regression (#115) #119

Merged

CoreRasurae merged 1 commit from tdd/m1-llm-tool-unsafe-mode into master

2026-08-09 22:08:22 +00:00

CoreRasurae commented

2026-08-06 22:09:45 +00:00

Member

Summary

Adds a failing-first Behave regression test proving issue #115: LLMAgent._execute_tool_loop() (cleveractors.agents.llm) and its two budget/stuck-model synthesis-retry mirrors all compute parent_unsafe = self.config.get("unsafe_mode", False) before building the ephemeral per-call ToolAgent's context via LLMAgent._build_tool_context(). No code anywhere in the codebase ever sets config["unsafe_mode"] — the field an actor author actually sets per docs/index.md §4.5 is safe_mode. Because the dispatch sites never read safe_mode, parent_unsafe is always False, _build_tool_context never includes _unsafe_mode in the tool-call context, and ToolAgent._file_write_tool (§4.5.4) unconditionally raises "File writing requires unsafe mode" regardless of the agent's safe_mode configuration.

Extends the shared features/llm_agent_tool_loop.feature / features/steps/llm_agent_tool_loop_steps.py pair (rather than adding a new feature/step file) with a scenario that configures an LLMAgent with tools: [file_write] and safe_mode: false, mocks a model-issued file_write tool call through the real (unmocked) ToolAgent dispatch path, and asserts the resulting ToolMessage reports success. Today it doesn't: the tool call fails with "File writing requires unsafe mode".

The scenario is tagged @tdd_issue, @tdd_issue_115, and @tdd_expected_fail per the TDD issue-capture workflow.

The actual fix (deriving parent_unsafe from the agent's own safe_mode field) lands separately on bugfix/m1-llm-tool-unsafe-mode per issue #115.

Test plan

nox -s lint green
nox -s format -- --check green
nox -s typecheck green
nox -s security_scan green
nox -s dead_code green
nox -s unit_tests green — confirmed the assertion fails via AssertionError without the @tdd_expected_fail tag (verified locally by temporarily removing it), and TddExpectedFailPolicy inverts it to a pass with the tag present
nox -s coverage_report — 96.6% (>= 96.5% threshold; test-only change, features/ excluded from coverage)
nox -s integration_tests — green (345/345)
nox -s benchmark_regression — skipped; this is a test-only change with no new/affected ASV benchmarks

Closes #116

## Summary Adds a failing-first Behave regression test proving issue #115: `LLMAgent._execute_tool_loop()` (`cleveractors.agents.llm`) and its two budget/stuck-model synthesis-retry mirrors all compute `parent_unsafe = self.config.get("unsafe_mode", False)` before building the ephemeral per-call `ToolAgent`'s context via `LLMAgent._build_tool_context()`. No code anywhere in the codebase ever sets `config["unsafe_mode"]` — the field an actor author actually sets per `docs/index.md` §4.5 is `safe_mode`. Because the dispatch sites never read `safe_mode`, `parent_unsafe` is always `False`, `_build_tool_context` never includes `_unsafe_mode` in the tool-call context, and `ToolAgent._file_write_tool` (§4.5.4) unconditionally raises "File writing requires unsafe mode" regardless of the agent's `safe_mode` configuration. Extends the shared `features/llm_agent_tool_loop.feature` / `features/steps/llm_agent_tool_loop_steps.py` pair (rather than adding a new feature/step file) with a scenario that configures an `LLMAgent` with `tools: [file_write]` and `safe_mode: false`, mocks a model-issued `file_write` tool call through the real (unmocked) `ToolAgent` dispatch path, and asserts the resulting `ToolMessage` reports success. Today it doesn't: the tool call fails with "File writing requires unsafe mode". The scenario is tagged `@tdd_issue`, `@tdd_issue_115`, and `@tdd_expected_fail` per the TDD issue-capture workflow. The actual fix (deriving `parent_unsafe` from the agent's own `safe_mode` field) lands separately on `bugfix/m1-llm-tool-unsafe-mode` per issue #115. ## Test plan - [x] `nox -s lint` green - [x] `nox -s format -- --check` green - [x] `nox -s typecheck` green - [x] `nox -s security_scan` green - [x] `nox -s dead_code` green - [x] `nox -s unit_tests` green — confirmed the assertion fails via `AssertionError` without the `@tdd_expected_fail` tag (verified locally by temporarily removing it), and `TddExpectedFailPolicy` inverts it to a pass with the tag present - [x] `nox -s coverage_report` — 96.6% (>= 96.5% threshold; test-only change, features/ excluded from coverage) - [x] `nox -s integration_tests` — green (345/345) - [x] `nox -s benchmark_regression` — skipped; this is a test-only change with no new/affected ASV benchmarks Closes #116

CoreRasurae added the

Type

Testing

label

2026-08-06 22:10:51 +00:00

CoreRasurae added this to the v2.1.0 milestone

2026-08-06 22:11:04 +00:00

CoreRasurae added a new dependency

2026-08-06 22:12:19 +00:00

#116 TDD: LLM agent file_write calls are always blocked — safe_mode: false / context.global.unsafe: true never reach _unsafe_mode

hurui200320 requested changes

2026-08-08 07:06:41 +00:00

Dismissed

hurui200320 left a comment

PR Review: !119 (Ticket #116 / underlying #115)

Verdict: Request Changes

The PR is not in a mergeable state: Gitea reports mergeable: false, and the branch carries stale versions of issue #107 changes that are already on master. If merged as-is, it would regress the _sandboxed_import security boundary and duplicate/conflict with already-merged work. The intended #116 TDD regression test for #115 is structurally sound, but it cannot land until the branch is cleanly rebased onto current master and the unrelated #107 artifacts are removed.

Critical Issues

PR is unmergeable and based on stale master
- The API reports mergeable: false for this PR. The branch appears to have diverged from master before the final version of the issue #107 fix landed.
- This must be resolved by rebasing tdd/m1-llm-tool-unsafe-mode onto current origin/master (or recutting the branch from master) so that only the intended #116 changes remain in the diff.
_sandboxed_import would lose the relative-import guard (regression of #107)
- File: src/cleveractors/agents/tool.py (_sandboxed_import)
- The PR's version of _sandboxed_import only checks if name not in _SANDBOX_ALLOWED_IMPORTS and ignores the level argument. Current master already has the reviewed #107 fix that also rejects level != 0 (from .json import dumps and other relative-import forms) per docs/index.md §13.2.1.
- Merging this branch would re-open the relative-import bypass that was explicitly closed in response to review feedback on PR !118.
Stale #107 test collateral would regress master
- File: features/inline_sandbox_import_restriction.feature
- The PR's version is missing the relative-import-of-json scenario that was added during #107 review.
- File: features/steps/inline_sandbox_import_restriction_steps.py
- The PR's assertion only checks isinstance(context.error, ExecutionError). The reviewed master version additionally verifies that the rejected module name appears in the error message and that the statement after the import never ran (result stays unset), preventing the test from passing on an unrelated error.

Major Issues

Scope contamination: PR bundles unrelated #107 work
- The PR title, linked issue (#116), and description all describe only the TDD regression test for #115. However, the diff also includes source changes for issue #107 (src/cleveractors/agents/tool.py, CHANGELOG.md, features/inline_sandbox_import_restriction.feature, robot/InlineSandboxImportRestrictionLib.py, robot/inline_sandbox_import_restriction.robot), rewrites of tests/fixtures/email_graph/*.yaml, and deletion of features/stderr_suppression.feature / features/steps/stderr_suppression_steps.py.
- All of that #107 work is already on master (commit c515470 and its predecessors). It should not appear in a PR whose sole purpose is #116.
Misleading PR description
- The description states "test-only change" and "The actual fix ... lands separately," but the diff includes non-test source changes (src/cleveractors/agents/tool.py). Once the branch is rebased, only the intended Behave/Robot test additions for #116 should remain.

Minor Issues

Fixture rewrites are copy-pasted rather than shared
- Files: tests/fixtures/email_graph/components/business_email.yaml, tests/fixtures/email_graph/components/component_orders.yaml, tests/fixtures/email_graph/local_graph.yaml, tests/fixtures/email_graph/mixed_graph.yaml, tests/fixtures/email_graph/namespaced/*.yaml, tests/fixtures/email_graph/remote_graph.yaml
- The PR's diff repeats the same hand-rolled parsing logic in multiple fixtures. master already deduplicates this via constants in robot/EmailGraphLib.py (_expand_shared_snippets). If any fixture rewrite is genuinely needed, it should follow the existing shared-snippet pattern, not re-introduce duplication.

Nits

None worth blocking on once the critical/major issues above are fixed.

Summary

The #116 TDD test itself — the new @tdd_issue @tdd_issue_115 @tdd_expected_fail scenario in features/llm_agent_tool_loop.feature and its step definitions — is well-targeted: it configures an LLMAgent with safe_mode: false, mocks a file_write tool call, and asserts success, which will fail today because LLMAgent reads the nonexistent unsafe_mode key instead of safe_mode. However, the branch as it stands is not rebased on current master and carries stale issue #107 changes that would regress the repository. Please rebase (not merge) onto origin/master, drop the unrelated #107 artifacts, and verify the diff contains only the #116 test additions before resubmitting.

## PR Review: !119 (Ticket #116 / underlying #115) ### Verdict: Request Changes The PR is not in a mergeable state: Gitea reports `mergeable: false`, and the branch carries stale versions of issue #107 changes that are already on `master`. If merged as-is, it would regress the `_sandboxed_import` security boundary and duplicate/conflict with already-merged work. The intended #116 TDD regression test for #115 is structurally sound, but it cannot land until the branch is cleanly rebased onto current `master` and the unrelated #107 artifacts are removed. ### Critical Issues 1. **PR is unmergeable and based on stale `master`** - The API reports `mergeable: false` for this PR. The branch appears to have diverged from `master` before the final version of the issue #107 fix landed. - This must be resolved by rebasing `tdd/m1-llm-tool-unsafe-mode` onto current `origin/master` (or recutting the branch from `master`) so that only the intended #116 changes remain in the diff. 2. **`_sandboxed_import` would lose the relative-import guard (regression of #107)** - **File:** `src/cleveractors/agents/tool.py` (`_sandboxed_import`) - The PR's version of `_sandboxed_import` only checks `if name not in _SANDBOX_ALLOWED_IMPORTS` and ignores the `level` argument. Current `master` already has the reviewed #107 fix that also rejects `level != 0` (`from .json import dumps` and other relative-import forms) per docs/index.md §13.2.1. - Merging this branch would re-open the relative-import bypass that was explicitly closed in response to review feedback on PR !118. 3. **Stale #107 test collateral would regress `master`** - **File:** `features/inline_sandbox_import_restriction.feature` - The PR's version is missing the relative-import-of-`json` scenario that was added during #107 review. - **File:** `features/steps/inline_sandbox_import_restriction_steps.py` - The PR's assertion only checks `isinstance(context.error, ExecutionError)`. The reviewed `master` version additionally verifies that the rejected module name appears in the error message and that the statement after the import never ran (`result` stays unset), preventing the test from passing on an unrelated error. ### Major Issues 1. **Scope contamination: PR bundles unrelated #107 work** - The PR title, linked issue (#116), and description all describe only the TDD regression test for #115. However, the diff also includes source changes for issue #107 (`src/cleveractors/agents/tool.py`, `CHANGELOG.md`, `features/inline_sandbox_import_restriction.feature`, `robot/InlineSandboxImportRestrictionLib.py`, `robot/inline_sandbox_import_restriction.robot`), rewrites of `tests/fixtures/email_graph/*.yaml`, and deletion of `features/stderr_suppression.feature` / `features/steps/stderr_suppression_steps.py`. - All of that #107 work is already on `master` (commit `c515470` and its predecessors). It should not appear in a PR whose sole purpose is #116. 2. **Misleading PR description** - The description states "test-only change" and "The actual fix ... lands separately," but the diff includes non-test source changes (`src/cleveractors/agents/tool.py`). Once the branch is rebased, only the intended Behave/Robot test additions for #116 should remain. ### Minor Issues 1. **Fixture rewrites are copy-pasted rather than shared** - **Files:** `tests/fixtures/email_graph/components/business_email.yaml`, `tests/fixtures/email_graph/components/component_orders.yaml`, `tests/fixtures/email_graph/local_graph.yaml`, `tests/fixtures/email_graph/mixed_graph.yaml`, `tests/fixtures/email_graph/namespaced/*.yaml`, `tests/fixtures/email_graph/remote_graph.yaml` - The PR's diff repeats the same hand-rolled parsing logic in multiple fixtures. `master` already deduplicates this via constants in `robot/EmailGraphLib.py` (`_expand_shared_snippets`). If any fixture rewrite is genuinely needed, it should follow the existing shared-snippet pattern, not re-introduce duplication. ### Nits - None worth blocking on once the critical/major issues above are fixed. ### Summary The #116 TDD test itself — the new `@tdd_issue @tdd_issue_115 @tdd_expected_fail` scenario in `features/llm_agent_tool_loop.feature` and its step definitions — is well-targeted: it configures an `LLMAgent` with `safe_mode: false`, mocks a `file_write` tool call, and asserts success, which will fail today because `LLMAgent` reads the nonexistent `unsafe_mode` key instead of `safe_mode`. However, the branch as it stands is not rebased on current `master` and carries stale issue #107 changes that would regress the repository. **Please rebase (not merge) onto `origin/master`, drop the unrelated #107 artifacts, and verify the diff contains only the #116 test additions before resubmitting.**

CoreRasurae added 1 commit

2026-08-08 16:54:37 +00:00

test(agents): capture LLM-agent file_write unsafe-mode regression (#115 )

CI / lint (pull_request) Successful in 34s

Details

CI / typecheck (pull_request) Successful in 1m3s

Details

CI / security (pull_request) Successful in 58s

Details

CI / quality (pull_request) Successful in 38s

Details

CI / build (pull_request) Successful in 1m32s

Details

CI / integration_tests (pull_request) Successful in 3m13s

Details

CI / unit_tests (pull_request) Successful in 5m3s

Details

CI / coverage (pull_request) Failing after 11m4s

Details

CI / benchmark (pull_request) Failing after 16m8s

Details

CI / status-check (pull_request) Failing after 5s

Details

ede57c02b4

Adds a failing-first Behave regression test proving issue #115:
LLMAgent._execute_tool_loop() (cleveractors.agents.llm) and its two
budget/stuck-model synthesis-retry mirrors all compute
`parent_unsafe = self.config.get("unsafe_mode", False)` before building
the ephemeral per-call ToolAgent's context via
LLMAgent._build_tool_context(). No code anywhere in the codebase ever
sets config["unsafe_mode"] -- the field an actor author actually sets
per docs/index.md §4.5 is `safe_mode`. Because the dispatch sites never
read `safe_mode`, `parent_unsafe` is always False, `_build_tool_context`
never includes `_unsafe_mode` in the tool-call context, and
ToolAgent._file_write_tool (§4.5.4) unconditionally raises
"File writing requires unsafe mode" regardless of the agent's
`safe_mode` configuration.

Extends the shared features/llm_agent_tool_loop.feature /
features/steps/llm_agent_tool_loop_steps.py pair (rather than adding a
new feature/step file) with a scenario that configures an LLMAgent with
`tools: [file_write]` and `safe_mode: false`, mocks a model-issued
file_write tool call through the real (unmocked) ToolAgent dispatch
path, and asserts the resulting ToolMessage reports success. Confirmed
the assertion fails via a plain AssertionError when @tdd_expected_fail
is removed (today it reports the "File writing requires unsafe mode"
error instead), and passes CI via TddExpectedFailPolicy's inversion
with the tag present.

The scenario is tagged @tdd_issue, @tdd_issue_115, and
@tdd_expected_fail per the TDD issue-capture workflow.

The actual fix (deriving parent_unsafe from the agent's own safe_mode
field) lands separately on bugfix/m1-llm-tool-unsafe-mode per issue
#115.

Refs: #115, #116

CoreRasurae force-pushed tdd/m1-llm-tool-unsafe-mode from d2979e1ab6

CI / lint (pull_request) Successful in 1m2s

Details

CI / typecheck (pull_request) Successful in 1m29s

Details

CI / security (pull_request) Successful in 1m24s

Details

CI / build (pull_request) Successful in 54s

Details

CI / quality (pull_request) Successful in 1m43s

Details

CI / integration_tests (pull_request) Successful in 3m20s

Details

CI / unit_tests (pull_request) Successful in 5m46s

Details

CI / coverage (pull_request) Successful in 3m43s

Details

CI / status-check (pull_request) Successful in 6s

Details

CI / benchmark (pull_request) Failing after 16m1s

Details

to ede57c02b4

CI / lint (pull_request) Successful in 34s

Details

CI / typecheck (pull_request) Successful in 1m3s

Details

CI / security (pull_request) Successful in 58s

Details

CI / quality (pull_request) Successful in 38s

Details

CI / build (pull_request) Successful in 1m32s

Details

CI / integration_tests (pull_request) Successful in 3m13s

Details

CI / unit_tests (pull_request) Successful in 5m3s

Details

CI / coverage (pull_request) Failing after 11m4s

Details

CI / benchmark (pull_request) Failing after 16m8s

Details

CI / status-check (pull_request) Failing after 5s

Details

2026-08-08 16:54:38 +00:00

Compare

hurui200320 approved these changes

2026-08-09 12:23:04 +00:00

hurui200320 left a comment

PR Review: !119 (Ticket #116)

Verdict: Approve

The PR now contains only the intended TDD regression test for issue #115. The previous critical concerns (unmergeable branch carrying stale #107 artifacts) have been resolved: the diff is limited to the two expected test files, and Gitea reports the PR as mergeable. The scenario correctly reproduces the bug described in #115: with safe_mode: false, the LLM-agent tool-call dispatch still reads the dead unsafe_mode key, so _unsafe_mode never reaches the ephemeral ToolAgent's context and file_write fails. The assertion is an AssertionError, which TddExpectedFailPolicy inverts under @tdd_expected_fail, matching the TDD workflow.

Critical Issues

None

Major Issues

None

Minor Issues

None

Nits

None

Summary

Clean, focused TDD capture PR. The new scenario in features/llm_agent_tool_loop.feature and its step definitions in features/steps/llm_agent_tool_loop_steps.py extend existing shared BDD files rather than adding unnecessary new ones, consistent with the project's conventions. The tags @tdd_issue @tdd_issue_115 @tdd_expected_fail are well-formed, the assertion type is correct, and the test will flip to a real pass (and trigger stale-tag detection) once #115's fix lands on bugfix/m1-llm-tool-unsafe-mode. Approved.

## PR Review: !119 (Ticket #116) ### Verdict: Approve The PR now contains only the intended TDD regression test for issue #115. The previous critical concerns (unmergeable branch carrying stale #107 artifacts) have been resolved: the diff is limited to the two expected test files, and Gitea reports the PR as mergeable. The scenario correctly reproduces the bug described in #115: with `safe_mode: false`, the LLM-agent tool-call dispatch still reads the dead `unsafe_mode` key, so `_unsafe_mode` never reaches the ephemeral ToolAgent's context and `file_write` fails. The assertion is an `AssertionError`, which `TddExpectedFailPolicy` inverts under `@tdd_expected_fail`, matching the TDD workflow. ### Critical Issues None ### Major Issues None ### Minor Issues None ### Nits None ### Summary Clean, focused TDD capture PR. The new scenario in `features/llm_agent_tool_loop.feature` and its step definitions in `features/steps/llm_agent_tool_loop_steps.py` extend existing shared BDD files rather than adding unnecessary new ones, consistent with the project's conventions. The tags `@tdd_issue @tdd_issue_115 @tdd_expected_fail` are well-formed, the assertion type is correct, and the test will flip to a real pass (and trigger stale-tag detection) once #115's fix lands on `bugfix/m1-llm-tool-unsafe-mode`. Approved.

CoreRasurae merged commit b98bcf7b1d into master

2026-08-09 22:08:21 +00:00

CoreRasurae deleted branch tdd/m1-llm-tool-unsafe-mode

2026-08-09 22:08:34 +00:00