Skip to content

feat(python): add external function handler support#394

Open
achicu wants to merge 1 commit intoeverruns:mainfrom
achicu:feat/python-external-fns
Open

feat(python): add external function handler support#394
achicu wants to merge 1 commit intoeverruns:mainfrom
achicu:feat/python-external-fns

Conversation

@achicu
Copy link

@achicu achicu commented Feb 28, 2026

Allow host applications to register async callback functions that Python code can call via monty's external_functions mechanism. The handler receives raw MontyObject args and returns ExternalResult directly, avoiding unnecessary serialization.

New public types: PythonExternalFnHandler, PythonExternalFns. Re-exports monty types (MontyObject, ExternalResult, etc.) for consumers. Adds BashBuilder::python_with_external_handler() builder method.

Allow host applications to register async callback functions that Python
code can call via monty's external_functions mechanism. The handler
receives raw MontyObject args and returns ExternalResult directly,
avoiding unnecessary serialization.

New public types: PythonExternalFnHandler, PythonExternalFns.
Re-exports monty types (MontyObject, ExternalResult, etc.) for consumers.
Adds BashBuilder::python_with_external_handler() builder method.
chaliy

This comment was marked as outdated.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support for host-registered async external function handlers that embedded Monty-backed Python code can call, enabling direct raw MontyObject argument handling and returning ExternalResult without extra serialization.

Changes:

  • Introduces PythonExternalFnHandler / PythonExternalFns and wires optional external function handling into the Python builtin execution loop.
  • Adds BashBuilder::python_with_external_handler() to register python/python3 builtins with external function support.
  • Re-exports handler-related monty types (MontyObject, ExternalResult, etc.) for downstream consumers under the python feature.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
crates/bashkit/src/lib.rs Re-exports new handler types and required monty types; adds builder method for configuring Python with external handlers.
crates/bashkit/src/builtins/python.rs Implements external function handler plumbing in the Monty execution loop; adds tests for handler behavior.
crates/bashkit/src/builtins/mod.rs Makes the python builtin module publicly accessible within the builtins module when the python feature is enabled.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

// No external functions registered; return error
ExternalResult::Error(MontyException::new(
ExcType::RuntimeError,
Some("external function not available in virtual mode".into()),
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RuntimeError message used when no external handler is configured is now misleading: external functions can be available in virtual mode when configured. Consider changing this to an error that clearly indicates the missing configuration (e.g., no external handler registered / external functions not enabled) so callers can diagnose it correctly.

Suggested change
Some("external function not available in virtual mode".into()),
Some("no external function handler configured (external functions not enabled)".into()),

Copilot uses AI. Check for mistakes.
Comment on lines +123 to +131
/// External function configuration for the Python builtin.
///
/// Groups function names and their async handler together.
pub struct PythonExternalFns {
/// Function names callable from Python (e.g., `"call_tool"`).
pub names: Vec<String>,
/// Async handler invoked when Python calls one of these functions.
pub handler: PythonExternalFnHandler,
}
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PythonExternalFns is a new public API type but doesn't derive common traits like Debug/Clone. In this codebase, similar public config structs (e.g., ExecutionLimits, GitConfig, NetworkAllowlist) derive these; adding #[derive(Debug, Clone)] here would improve ergonomics and keep the public surface consistent.

Copilot uses AI. Check for mistakes.
chaliy

This comment was marked as outdated.

@chaliy chaliy dismissed their stale review February 28, 2026 17:13

Revised: security concerns overstated. Posting corrected review.

Copy link
Contributor

@chaliy chaliy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revised Review: feat(python): add external function handler support

Note: Previous "Request Changes" review was dismissed — security concerns were overstated. External function handlers follow the same trust model as existing BashBuilder::builtin() and ScriptedTool callbacks: the host application registers trusted Rust code, untrusted scripts invoke it by name.


What's Good

  • Clean integration with Monty's RunProgress::FunctionCall pause/resume mechanism
  • Good test coverage (8 tests: return values, args, kwargs, errors, caught exceptions, multi-call, dispatch by name)
  • Follows existing builder pattern (with_external_handler)
  • Type-safe handler via PythonExternalFnHandler

Requested Changes

1. Keep mod python private (API surface)

-pub mod python;
+mod python;

The module is currently private and only PythonLimits is re-exported through mod.rs. Do the same here — re-export PythonExternalFnHandler and PythonExternalFns from mod.rs rather than making the entire module public. This avoids committing to the full module as public API.

2. Re-exporting raw monty types is risky (API stability)

pub use monty::{ExcType, ExternalResult, MontyException, MontyObject};

monty is a git-pinned dep (rev = "87f8f31"), not on crates.io. Re-exporting its types as bashkit public API means any monty change breaks bashkit consumers. Consider either:

  • Bashkit-native wrapper types (e.g., PythonValue enum mapping to/from MontyObject)
  • Or at minimum mark these re-exports as unstable/experimental in docs

3. Spec update needed (AGENTS.md compliance)

specs/011-python-builtin.md should document external function support — a new "External Functions" section covering the builder API, handler signature, and usage example.

4. PythonExternalFns field visibility (minor)

Fields are pub but the struct is only constructed internally via with_external_handler. Consider private fields for consistency.


Overall

Solid contribution. With the module visibility and spec items addressed, this is ready to merge.

@chaliy
Copy link
Contributor

chaliy commented Feb 28, 2026

Thank you for contribution. This is very intersting. This opens an ability for Agents to orchestrate custom things in Python, this is awesome idea. Let me know if you will be able to address comments and make CI green.

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.

3 participants