Skip to content

[AAASM-4658] 🐛 (adapters): Set raise_error so LangChain enforces policy deny#262

Merged
Chisanan232 merged 2 commits into
masterfrom
v0.0.1/AAASM-4658/enforce_callback_deny
Jul 15, 2026
Merged

[AAASM-4658] 🐛 (adapters): Set raise_error so LangChain enforces policy deny#262
Chisanan232 merged 2 commits into
masterfrom
v0.0.1/AAASM-4658/enforce_callback_deny

Conversation

@Chisanan232

@Chisanan232 Chisanan232 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Description

AssemblyCallbackHandler.on_tool_start raises ToolExecutionBlockedError on a policy DENY, but LangChain's CallbackManager only propagates an exception raised inside a callback when the handler's raise_error flag is True. The handler inherited LangChain's default of raise_error = False, so LangChain logged the exception and ran the tool anyway.

The consequence: a user who wires the handler the idiomatic LangChain way (callbacks=[handler]) had a policy DENY silently bypassed — the denied tool (execute_sql, run_shell_command, …) still executed and returned a result, with only a single log line as trace. This defeats the product's core promise ("every tool call … allowed, denied, or recorded") on the LangChain callback path. The shipped example avoided the bug only by calling on_tool_start directly, which a real user wrapping an AgentExecutor cannot do.

Fix: set the class attribute raise_error = True on AssemblyCallbackHandler (one line + rationale comment) so LangChain propagates the block instead of swallowing it. Minimal surface; distinct from the AAASM-4014/4034 __getattr__ delegation fix.

Type of Change

  • 🔧 Bug fix

Breaking Changes

  • No

Governance now enforces a DENY on the idiomatic callback path where it previously (incorrectly) did not — a security correction, not an API change.

Related Issues

  • Related JIRA ticket: AAASM-4658
  • Fix Version: agent-assembly v0.0.1-rc.6

Testing

  • Unit tests added/updated

New regression test test/unit/adapters/langchain/test_callback_raise_error_enforcement.py attaches the handler the idiomatic way (callbacks=[handler]) on a real @tool and asserts a denied tool raises ToolExecutionBlockedError and never runs its body — the exact AAASM-4658 path that a direct on_tool_start call does not exercise. It is guarded by importorskip and runs under the langchain-test dependency group.

Verified the reproduction: with raise_error=False (pre-fix) the denied tool executes and returns a result; with the fix it is blocked. The existing test_getattr_contract_with_langchain.py invariant (all contract flags resolve statically; raise_error now intentionally overridden to True) was reconciled in the fix commit to keep every commit bisectable.

Local runs (with uv sync --group langchain-test):

  • pytest test/unit/adapters/langchain/ -> 84 passed
  • pytest test/unit -> 728 passed, 4 skipped
  • pre-commit run (isort / autoflake / black / mypy) -> all passed

Checklist

  • Code follows project style guidelines
  • Self-review completed
  • Comments added for complex logic
  • Documentation updated if needed — docs quick-start copy is a separate concern; not touched here
  • All tests passing

🤖 Generated with Claude Code

https://claude.ai/code/session_019NafDDcehs2ez4Ax9kuWfi

Chisanan232 and others added 2 commits July 15, 2026 21:47
LangChain's CallbackManager logs-and-swallows an exception raised in a
callback when raise_error is False (its inherited default), then runs the
tool anyway. So AssemblyCallbackHandler wired the idiomatic way
(callbacks=[handler]) let a policy DENY be silently bypassed: on_tool_start
raised ToolExecutionBlockedError but the denied tool still executed, with
only a log line as trace. Setting raise_error=True makes LangChain
propagate the block so a DENY aborts the tool call.

Refs AAASM-4658
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_019NafDDcehs2ez4Ax9kuWfi
Attaches AssemblyCallbackHandler the idiomatic LangChain way
(callbacks=[handler]) on a real @tool and asserts a denied tool raises
ToolExecutionBlockedError and never runs its body — the AAASM-4658 path
that a direct on_tool_start call does not exercise. Guarded by
importorskip so it runs under the langchain-test dependency group.

Refs AAASM-4658
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_019NafDDcehs2ez4Ax9kuWfi
@sonarqubecloud

Copy link
Copy Markdown

@codecov

codecov Bot commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@Chisanan232

Copy link
Copy Markdown
Contributor Author

🤖 Claude Code — PR review

CI state: GREEN — 19/19 checks pass, including the full CI unit+integration runs, the LangChain-installed __getattr__ contract test, CodeQL, SonarCloud, and Codecov patch. mergeStateStatus: BLOCKED reflects required-review approval, not a CI failure. Nothing to fix. As an extra green-proxy for this HIGH-severity code change I also ran the adapter suite locally in the branch worktree (pytest test/unit/adapters/langchain, langchain-core 1.4.9) → 84 passed.

Scope coverage: Fully covers AAASM-4658's ## Fix. Sets the class attribute raise_error = True on AssemblyCallbackHandler so LangChain's CallbackManager propagates the DENY (ToolExecutionBlockedError) raised in on_tool_start instead of logging-and-swallowing it and running the tool anyway. Adds the requested regression test that attaches the handler the idiomatic way (callbacks=[handler]) on a real @tool and asserts a denied tool raises and never runs its body — the exact path a direct on_tool_start call does not exercise. The optional docs quick-start copy update is explicitly deferred (a separate concern), consistent with the ticket marking it "Optionally".

Side-effect assessment: Safe for existing callers.

  • raise_error is a single class-level flag that governs all of the handler's callbacks (on_tool_start, on_tool_end, on_llm_start, on_llm_end) — LangChain offers no per-method granularity, so this breadth is inherent and unavoidable.
  • The ALLOW path is unaffected: on_tool_start only raises on deny / unapproved pending; it returns None for allow, so no previously-allowed tool is now blocked (no false-positive blocking). observe/disabled postures fail open via _unknown_decision(), so they never raise here either.
  • The audit/scan callbacks (on_tool_end, on_llm_start→scan, on_llm_end) delegate to the interceptor only when the target method is callable and do not raise under normal operation. If one did raise (e.g. a gateway error during audit reporting), it would now propagate rather than be swallowed — which is consistent with the product's fail-closed-under-enforce design intent, and no existing test relied on that swallow (full unit suite green: 728 passed).
  • The one existing invariant test affected (test_getattr_contract_with_langchain.py) was reconciled in the fix so raise_error is recognized as an intentional override (True) rather than a delegated contract flag. Commits are granular and bisectable (fix commit separate from test commit).

FE: No FE-app change; adapter library code only. Playwright validation N/A.

Verdict: READY to merge (pending required review approval). Correctly closes the HIGH-severity governance-bypass on the idiomatic LangChain callback path.

@Chisanan232 Chisanan232 merged commit d727d8e into master Jul 15, 2026
26 checks passed
@Chisanan232 Chisanan232 deleted the v0.0.1/AAASM-4658/enforce_callback_deny branch July 15, 2026 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant