fix(safety): non-blocking review must not flip OUTCOME_FAILED; lazy get_tool_var

Warning:
- _unsafe_local_code_executor.execute_code: non-blocking
  NEEDS_HUMAN_REVIEW previously appended the warning to error_parts,
  which became stderr, which made create_code_execution_result set
  outcome=OUTCOME_FAILED. _code_execution_processor then treated it as
  an execution error and retried the already-allowed code up to
  error_retry_attempts (default 2) times. This also was inconsistent
  with ToolSafetyFilter._after (which keeps success=True and only
  merges safety_warning). Now the warning goes through the SDK logger
  (matching _filter._before), stderr stays empty, outcome stays
  OUTCOME_OK, and the agent does not retry. The audit log still
  records the NEEDS_HUMAN_REVIEW decision. Removed the now-unused
  _pending_review_warning variable.

Suggestion:
- _filter.ToolSafetyFilter.__init__: the comment claimed "never pull
  trpc_agent_sdk.tools package", but `from trpc_agent_sdk.tools.
  _context_var import get_tool_var` actually executes
  trpc_agent_sdk/tools/__init__.py, which imports mcp_tool /
  file_tools / webfetch_tool and their optional deps (mcp, anthropic).
  Replaced the eager import with a sys.modules lookup: we only attach
  get_tool_var when the host application has already loaded
  trpc_agent_sdk.tools._context_var, so the safety package never
  triggers the heavy tools/__init__.py itself. When tools are loaded,
  we reuse the exact same ContextVar instance so tool names resolve
  correctly.

Tests:
- test_unsafe_local_code_executor: add
  TestUnsafeLocalCodeExecutorSafetyGuardNonBlocking with
  test_nonblocking_review_keeps_outcome_ok — asserts that
  enable_safety_guard=True + block_on_review=False + NEEDS_HUMAN_REVIEW
  script keeps outcome=OUTCOME_OK and executes exactly once (no retry).
- Clean up two pre-existing unused imports (AsyncMock,
  create_code_execution_result) in the same test file so flake8 stays
  clean on changed files.

Author: Violet2314

tests/code_executors/local/test_unsafe_local_code_executor.py

@@ -8,15 +8,13 @@
 import shutil
 import tempfile
 from pathlib import Path
-from unittest.mock import AsyncMock
 from unittest.mock import Mock
 from unittest.mock import patch
 
 import pytest
 from trpc_agent_sdk.code_executors._types import CodeBlock
 from trpc_agent_sdk.code_executors._types import CodeBlockDelimiter
 from trpc_agent_sdk.code_executors._types import CodeExecutionInput
-from trpc_agent_sdk.code_executors._types import create_code_execution_result
 from trpc_agent_sdk.code_executors.local import UnsafeLocalCodeExecutor
 from trpc_agent_sdk.context import InvocationContext
 from trpc_agent_sdk.types import Outcome
@@ -439,3 +437,49 @@ async def test_error_with_empty_stderr(self, mock_exec):
         inp = CodeExecutionInput(code_blocks=[CodeBlock(language="python", code="bad")])
         result = await executor.execute_code(self.mock_ctx, inp)
         assert result.outcome == Outcome.OUTCOME_FAILED
+
+
+class TestUnsafeLocalCodeExecutorSafetyGuardNonBlocking:
+    """Tests for non-blocking NEEDS_HUMAN_REVIEW path in safety guard.
+
+    Regression for CongkeChen review: previously the review warning was
+    appended to stderr, which made create_code_execution_result set
+    outcome=OUTCOME_FAILED, triggering _code_execution_processor retries
+    of already-allowed code. Now the warning goes through logging and
+    outcome stays OUTCOME_OK.
+    """
+
+    def setup_method(self):
+        self.tmp_dir = tempfile.mkdtemp(prefix="safety_nonblocking_")
+        self.mock_ctx = Mock(spec=InvocationContext)
+
+    def teardown_method(self):
+        shutil.rmtree(self.tmp_dir, ignore_errors=True)
+
+    @patch('trpc_agent_sdk.code_executors.local._unsafe_local_code_executor.async_execute_command')
+    @pytest.mark.asyncio
+    async def test_nonblocking_review_keeps_outcome_ok(self, mock_exec):
+        """enable_safety_guard=True, block_on_review=False + NEEDS_HUMAN_REVIEW
+        script must keep outcome=OUTCOME_OK so the agent does not retry.
+
+        'sleep 100 &' is a non-network background process → MEDIUM →
+        NEEDS_HUMAN_REVIEW under default policy. With block_on_review=False
+        the code is allowed to run; the warning must NOT leak into stderr
+        (which would flip outcome to OUTCOME_FAILED).
+        """
+        mock_exec.return_value = CommandExecResult(stdout="done", stderr="", exit_code=0, is_timeout=False)
+        audit_path = os.path.join(self.tmp_dir, "audit.jsonl")
+        executor = UnsafeLocalCodeExecutor(
+            enable_safety_guard=True,
+            safety_audit_path=audit_path,
+            safety_block_on_review=False,
+        )
+        inp = CodeExecutionInput(code_blocks=[CodeBlock(language="bash", code="sleep 100 &")])
+        result = await executor.execute_code(self.mock_ctx, inp)
+        # outcome must remain OK so _code_execution_processor does not retry.
+        assert result.outcome == Outcome.OUTCOME_OK, (
+            f"non-blocking review must not flip outcome to FAILED (would trigger retry); got {result.outcome}")
+        # The code should have executed exactly once (no retry).
+        assert mock_exec.call_count == 1
+        # Audit log records the NEEDS_HUMAN_REVIEW decision.
+        assert os.path.exists(audit_path)

trpc_agent_sdk/code_executors/local/_unsafe_local_code_executor.py

@@ -105,12 +105,6 @@ async def execute_code(self, invocation_context: InvocationContext,
         Returns:
             CodeExecutionResult with combined output
         """
-        # Initialize at the top so the variable always has a defined value
-        # regardless of which branch (scanner on/off, report None/not) runs.
-        # Previously it was only assigned inside the `if self._safety_scanner`
-        # branch and the `else`, making later `if _pending_review_warning`
-        # depend on branch ordering — fragile for refactors.
-        _pending_review_warning = None
         if self._safety_scanner is not None:
             from trpc_agent_sdk.safety import Decision
             from trpc_agent_sdk.safety._wrapper import _scan_code_input
@@ -139,15 +133,20 @@ async def execute_code(self, invocation_context: InvocationContext,
                 # from "real execution error" so the agent does not retry.
                 code_label = ("TOOL_SAFETY_DENY" if report.decision == Decision.DENY else "TOOL_SAFETY_NEEDS_REVIEW")
                 return create_code_execution_result(stderr=f"{code_label}: {report.rule_ids}")
-            # Non-blocking NEEDS_HUMAN_REVIEW: do NOT block execution, but
-            # surface the warning so the caller is not silently unaware
-            # (matches _filter._after's merge semantics). Stash the warning
-            # and prepend it to stderr after execution completes below.
-            # Use logging (not sys.stderr.write) to avoid polluting captured
-            # stderr streams in CI / subprocess stdout/stderr separation.
+            # Non-blocking NEEDS_HUMAN_REVIEW: do NOT block execution. Surface
+            # the warning via the SDK logger (matches ToolSafetyFilter._before)
+            # instead of appending to stderr. Writing to stderr would make
+            # create_code_execution_result set outcome=OUTCOME_FAILED, which
+            # _code_execution_processor treats as an execution error and
+            # retries via error_retry_attempts — re-running already-allowed
+            # code. Logging keeps outcome=OUTCOME_OK so the agent does not
+            # retry, while operators still see the warning and the audit log
+            # records the decision.
             if (report is not None and report.decision == Decision.NEEDS_HUMAN_REVIEW and not should_block):
-                _pending_review_warning = (f"TOOL_SAFETY_NEEDS_REVIEW: {list(report.rule_ids)} "
-                                           f"(risk={report.risk_level.value})")
+                import logging
+                logging.getLogger("trpc_agent_sdk.safety").warning(
+                    "TOOL_SAFETY_NEEDS_REVIEW: %s (risk=%s)",
+                    list(report.rule_ids), report.risk_level.value)
 
         output_parts = []
         error_parts = []
@@ -172,8 +171,6 @@ async def execute_code(self, invocation_context: InvocationContext,
             if should_cleanup:
                 shutil.rmtree(work_dir, ignore_errors=True)
 
-        if _pending_review_warning:
-            error_parts.append(_pending_review_warning)
         return create_code_execution_result(stdout="\n".join(output_parts) if output_parts else "",
                                             stderr="\n".join(error_parts) if error_parts else "")
 

trpc_agent_sdk/safety/_filter.py

@@ -66,14 +66,20 @@ def __init__(
         self.audit = AuditLogger(audit_path)
         self._configured_tool_name = tool_name
         self._block_on_review = (policy.block_on_review if block_on_review is None else block_on_review)
-        # Prefer lightweight context-var import; never pull trpc_agent_sdk.tools
-        # package (heavy optional deps like anthropic) on every scan.
+        # Resolve the tool name from the async context var set by the SDK tool
+        # runner, so audit records show "Bash" instead of the generic configured
+        # name. We only attach get_tool_var when the host application has
+        # already imported trpc_agent_sdk.tools._context_var — importing it
+        # ourselves would execute trpc_agent_sdk/tools/__init__.py, which pulls
+        # mcp_tool / file_tools / webfetch_tool and their optional deps (mcp,
+        # anthropic, ...). Checking sys.modules keeps the safety package
+        # lightweight when tools are not in use; when they are, we reuse the
+        # exact same ContextVar instance so names resolve correctly.
+        import sys
         self._get_tool_var = None
-        try:
-            from trpc_agent_sdk.tools._context_var import get_tool_var
-            self._get_tool_var = get_tool_var
-        except Exception:  # pylint: disable=broad-except
-            self._get_tool_var = None
+        ctx_mod = sys.modules.get("trpc_agent_sdk.tools._context_var")
+        if ctx_mod is not None:
+            self._get_tool_var = getattr(ctx_mod, "get_tool_var", None)
 
     async def _before(self, ctx: Any, req: Any, rsp: FilterResult) -> None:
         """Scan the tool args; block execution when decision is DENY."""