diff --git a/.config/rustqual/baseline-dry.json b/.config/rustqual/baseline-dry.json index b844dc085..cc499df38 100644 --- a/.config/rustqual/baseline-dry.json +++ b/.config/rustqual/baseline-dry.json @@ -1,9 +1,9 @@ { "version": 2, - "quality_score": 0.6506185173027932, - "iosp_score": 0.8116445796219857, - "violations": 867, - "total": 6459, + "quality_score": 0.6519302292023524, + "iosp_score": 0.8120950323974082, + "violations": 870, + "total": 6496, "complexity_warnings": 0, "magic_number_warnings": 0, "nesting_depth_warnings": 0, @@ -11,7 +11,7 @@ "unsafe_warnings": 0, "error_handling_warnings": 0, "duplicate_groups": 29, - "dead_code_warnings": 264, + "dead_code_warnings": 262, "dead_type_warnings": 28, "fragment_groups": 58, "boilerplate_warnings": 0, @@ -28,7 +28,7 @@ "tq_untested_logic_warnings": 0, "structural_srp_warnings": 0, "structural_coupling_warnings": 0, - "total_findings": 1291, + "total_findings": 1292, "violation_details": [ { "name": "ConversationStream::config_unsets", @@ -858,7 +858,7 @@ { "name": "PartialPluginsConfig::assign", "file": "crates/jp_config/src/plugins.rs", - "line": 44 + "line": 63 }, { "name": "PartialCodeConfig::assign", @@ -1068,27 +1068,27 @@ { "name": "PartialToolsDefaultsConfig::assign", "file": "crates/jp_config/src/conversation/tool.rs", - "line": 377 + "line": 421 }, { "name": "PartialToolConfig::assign", "file": "crates/jp_config/src/conversation/tool.rs", - "line": 639 + "line": 686 }, { "name": "PartialToolParameterConfig::assign", "file": "crates/jp_config/src/conversation/tool.rs", - "line": 882 + "line": 929 }, { "name": "PartialEnableConfig::assign", "file": "crates/jp_config/src/conversation/tool.rs", - "line": 1897 + "line": 1998 }, { "name": "PartialFanOutConfig::assign", "file": "crates/jp_config/src/conversation/tool.rs", - "line": 2117 + "line": 2218 }, { "name": "PartialTitleConfig::assign", @@ -1163,17 +1163,17 @@ { "name": "PartialDisplayStyleConfig::assign", "file": "crates/jp_config/src/conversation/tool/style.rs", - "line": 153 + "line": 155 }, { "name": "PartialErrorStyleConfig::assign", "file": "crates/jp_config/src/conversation/tool/style.rs", - "line": 274 + "line": 276 }, { "name": "InlineResults::deserialize", "file": "crates/jp_config/src/conversation/tool/style.rs", - "line": 370 + "line": 372 }, { "name": "PartialAttachmentConfig::assign", @@ -2308,82 +2308,87 @@ { "name": "Query::run_locked", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 452 + "line": 457 }, { "name": "Query::conversation_load_request", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 881 + "line": 886 }, { "name": "Query::build_conversation", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 898 + "line": 903 }, { "name": "Query::create_new_conversation", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 996 + "line": 1001 }, { "name": "Query::run_turn", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 1169 + "line": 1174 }, { "name": "Query::acquire_lock", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 1383 + "line": 1388 }, { "name": "persist_config_reset", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 1992 + "line": 2020 }, { "name": "turn_config_delta", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 2021 + "line": 2049 }, { "name": "Query::apply_cli_config", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 2042 + "line": 2070 }, { "name": "build_sections", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 2142 + "line": 2170 }, { "name": "build_thread", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 2170 + "line": 2198 }, { "name": "apply_mounts", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 2566 + "line": 2594 + }, + { + "name": "inject_mounts", + "file": "crates/jp_cli/src/cmd/query.rs", + "line": 2612 }, { "name": "create_mount_effects", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 2639 + "line": 2678 }, { "name": "PendingStreamTrim::apply", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 2818 + "line": 2857 }, { "name": "mount_access_seed", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 2977 + "line": 3016 }, { "name": "read_piped_stdin", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 3081 + "line": 3121 }, { "name": "Login::run_with_auth", @@ -2438,72 +2443,72 @@ { "name": "run_plugin", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 361 + "line": 352 }, { "name": "spawn_plugin", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 546 + "line": 537 }, { "name": "stop_plugin", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 643 + "line": 634 }, { "name": "join_within", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 666 + "line": 657 }, { "name": "message_loop", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 741 + "line": 732 }, { "name": "run_query", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 891 + "line": 882 }, { "name": "prepare_turn", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 1261 + "line": 1252 }, { "name": "handle_request", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 1762 + "line": 1753 }, { "name": "handle_write_draft", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 2099 + "line": 2096 }, { "name": "handle_list_configs", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 2197 + "line": 2194 }, { "name": "describe", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 2480 + "line": 2477 }, { "name": "manifest_differences", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 2516 + "line": 2513 }, { "name": "describe_plugin", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 2541 + "line": 2538 }, { "name": "run_external", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 2900 + "line": 2897 }, { "name": "render", @@ -2525,6 +2530,16 @@ "file": "crates/jp_cli/src/cmd/plugin/update.rs", "line": 26 }, + { + "name": "TurnPlugins::admit", + "file": "crates/jp_cli/src/cmd/plugin/tool.rs", + "line": 109 + }, + { + "name": "TurnPlugins::admit_from", + "file": "crates/jp_cli/src/cmd/plugin/tool.rs", + "line": 137 + }, { "name": "not_approved", "file": "crates/jp_cli/src/cmd/plugin/admission.rs", @@ -2533,7 +2548,7 @@ { "name": "admit", "file": "crates/jp_cli/src/cmd/plugin/admission.rs", - "line": 280 + "line": 283 }, { "name": "Install::run", @@ -2783,137 +2798,137 @@ { "name": "ToolCoordinator::pre_render_for_prompt", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 671 + "line": 672 }, { "name": "ToolCoordinator::render_executor", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 694 + "line": 692 }, { "name": "ToolCoordinator::abandon", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 792 + "line": 790 }, { "name": "ToolCoordinator::decide_permission", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 887 + "line": 885 }, { "name": "ToolCoordinator::submit", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 976 + "line": 974 }, { "name": "ToolCoordinator::add_call", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1017 + "line": 1015 }, { "name": "ToolCoordinator::finish", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1094 + "line": 1092 }, { "name": "ToolCoordinator::dispatch", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1334 + "line": 1332 }, { "name": "ToolCoordinator::on_step", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1443 + "line": 1441 }, { "name": "ToolCoordinator::on_permission", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1493 + "line": 1491 }, { "name": "ToolCoordinator::pump", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1527 + "line": 1525 }, { "name": "ToolCoordinator::decide", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1585 + "line": 1583 }, { "name": "ToolCoordinator::announce", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1638 + "line": 1636 }, { "name": "ToolCoordinator::release", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1660 + "line": 1658 }, { "name": "ToolCoordinator::route", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1734 + "line": 1732 }, { "name": "ToolCoordinator::answer", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1832 + "line": 1830 }, { "name": "ToolCoordinator::serve", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1846 + "line": 1844 }, { "name": "ToolCoordinator::deliver", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1934 + "line": 1932 }, { "name": "ToolCoordinator::close", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1985 + "line": 1983 }, { "name": "ToolCoordinator::render_result", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 2227 + "line": 2225 }, { "name": "ToolCoordinator::preset_answer", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 2321 + "line": 2319 }, { "name": "route", "file": "crates/jp_cli/src/cmd/query/tool/mcp_executor.rs", - "line": 489 + "line": 493 }, { "name": "TerminalExecutorSource::create", "file": "crates/jp_cli/src/cmd/query/tool/mcp_executor.rs", - "line": 654 + "line": 658 }, { "name": "TerminalExecutorSource::fan_out_executors", "file": "crates/jp_cli/src/cmd/query/tool/mcp_executor.rs", - "line": 723 + "line": 727 }, { "name": "PendingCall::next", "file": "crates/jp_cli/src/cmd/query/tool/mcp_executor.rs", - "line": 1080 + "line": 1082 }, { "name": "PendingCall::receive", "file": "crates/jp_cli/src/cmd/query/tool/mcp_executor.rs", - "line": 1091 + "line": 1093 }, { "name": "PendingCall::receive_agent", "file": "crates/jp_cli/src/cmd/query/tool/mcp_executor.rs", - "line": 1123 + "line": 1125 }, { "name": "Grep::run", @@ -3208,17 +3223,17 @@ { "name": "TurnRenderer::new", "file": "crates/jp_cli/src/render/turn.rs", - "line": 107 + "line": 104 }, { "name": "TurnRenderer::render_turn", "file": "crates/jp_cli/src/render/turn.rs", - "line": 157 + "line": 154 }, { "name": "TurnRenderer::reconfigure", "file": "crates/jp_cli/src/render/turn.rs", - "line": 268 + "line": 264 }, { "name": "ToolRenderer::new", @@ -3448,7 +3463,7 @@ { "name": "register", "file": "crates/jp_mcp/src/server/service.rs", - "line": 527 + "line": 525 }, { "name": "Service::new", @@ -3458,42 +3473,42 @@ { "name": "validate_arguments", "file": "crates/jp_mcp/src/server/service.rs", - "line": 885 + "line": 887 }, { "name": "run_call", "file": "crates/jp_mcp/src/server/service.rs", - "line": 919 + "line": 921 }, { "name": "describe", "file": "crates/jp_mcp/src/server/service.rs", - "line": 1048 + "line": 1063 }, { "name": "conclude", "file": "crates/jp_mcp/src/server/service.rs", - "line": 1100 + "line": 1115 }, { "name": "run_fan_out", "file": "crates/jp_mcp/src/server/service.rs", - "line": 1138 + "line": 1153 }, { "name": "deliver_result", "file": "crates/jp_mcp/src/server/service.rs", - "line": 1388 + "line": 1403 }, { "name": "attempt", "file": "crates/jp_mcp/src/server/service.rs", - "line": 1448 + "line": 1463 }, { "name": "ask_for_input", "file": "crates/jp_mcp/src/server/service.rs", - "line": 1521 + "line": 1537 }, { "name": "split", @@ -3548,37 +3563,37 @@ { "name": "run_tool_command", "file": "crates/jp_mcp/src/server.rs", - "line": 430 + "line": 433 }, { "name": "parse_command_output", "file": "crates/jp_mcp/src/server.rs", - "line": 552 + "line": 555 }, { - "name": "execute", + "name": "run_plugin", "file": "crates/jp_mcp/src/server.rs", - "line": 701 + "line": 901 }, { - "name": "execute_local", + "name": "plugin_result", "file": "crates/jp_mcp/src/server.rs", - "line": 748 + "line": 961 }, { "name": "tool_definitions", "file": "crates/jp_mcp/src/server.rs", - "line": 913 + "line": 1102 }, { "name": "resolve_tool", "file": "crates/jp_mcp/src/server.rs", - "line": 966 + "line": 1153 }, { "name": "resolve_mcp_tool", "file": "crates/jp_mcp/src/server.rs", - "line": 990 + "line": 1179 }, { "name": "MockEditorBackend::edit_text", diff --git a/.config/rustqual/baseline.json b/.config/rustqual/baseline.json index 0cc3efa14..b5255b458 100644 --- a/.config/rustqual/baseline.json +++ b/.config/rustqual/baseline.json @@ -1,13 +1,13 @@ { "version": 2, - "quality_score": 0.7028860816848206, - "iosp_score": 0.9137309847878302, - "violations": 862, - "total": 13804, + "quality_score": 0.705261627659666, + "iosp_score": 0.9144580696202531, + "violations": 865, + "total": 13949, "complexity_warnings": 163, "magic_number_warnings": 238, "nesting_depth_warnings": 27, - "function_length_warnings": 428, + "function_length_warnings": 430, "unsafe_warnings": 74, "error_handling_warnings": 194, "duplicate_groups": 0, @@ -16,7 +16,7 @@ "fragment_groups": 0, "boilerplate_warnings": 286, "srp_struct_warnings": 46, - "srp_module_warnings": 208, + "srp_module_warnings": 210, "wildcard_import_warnings": 0, "sdp_violations": 0, "coupling_warnings": 0, @@ -26,9 +26,9 @@ "tq_untested_warnings": 95, "tq_uncovered_warnings": 0, "tq_untested_logic_warnings": 0, - "structural_srp_warnings": 47, + "structural_srp_warnings": 48, "structural_coupling_warnings": 8, - "total_findings": 2808, + "total_findings": 2816, "violation_details": [ { "name": "ConversationStream::config_unsets", @@ -858,7 +858,7 @@ { "name": "PartialPluginsConfig::assign", "file": "crates/jp_config/src/plugins.rs", - "line": 44 + "line": 63 }, { "name": "PartialCodeConfig::assign", @@ -1068,27 +1068,27 @@ { "name": "PartialToolsDefaultsConfig::assign", "file": "crates/jp_config/src/conversation/tool.rs", - "line": 377 + "line": 421 }, { "name": "PartialToolConfig::assign", "file": "crates/jp_config/src/conversation/tool.rs", - "line": 639 + "line": 686 }, { "name": "PartialToolParameterConfig::assign", "file": "crates/jp_config/src/conversation/tool.rs", - "line": 882 + "line": 929 }, { "name": "PartialEnableConfig::assign", "file": "crates/jp_config/src/conversation/tool.rs", - "line": 1897 + "line": 1998 }, { "name": "PartialFanOutConfig::assign", "file": "crates/jp_config/src/conversation/tool.rs", - "line": 2117 + "line": 2218 }, { "name": "PartialTitleConfig::assign", @@ -1163,17 +1163,17 @@ { "name": "PartialDisplayStyleConfig::assign", "file": "crates/jp_config/src/conversation/tool/style.rs", - "line": 153 + "line": 155 }, { "name": "PartialErrorStyleConfig::assign", "file": "crates/jp_config/src/conversation/tool/style.rs", - "line": 274 + "line": 276 }, { "name": "InlineResults::deserialize", "file": "crates/jp_config/src/conversation/tool/style.rs", - "line": 370 + "line": 372 }, { "name": "PartialAttachmentConfig::assign", @@ -2308,82 +2308,87 @@ { "name": "Query::run_locked", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 452 + "line": 457 }, { "name": "Query::conversation_load_request", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 881 + "line": 886 }, { "name": "Query::build_conversation", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 898 + "line": 903 }, { "name": "Query::create_new_conversation", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 996 + "line": 1001 }, { "name": "Query::run_turn", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 1169 + "line": 1174 }, { "name": "Query::acquire_lock", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 1383 + "line": 1388 }, { "name": "persist_config_reset", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 1992 + "line": 2020 }, { "name": "turn_config_delta", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 2021 + "line": 2049 }, { "name": "Query::apply_cli_config", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 2042 + "line": 2070 }, { "name": "build_sections", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 2142 + "line": 2170 }, { "name": "build_thread", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 2170 + "line": 2198 }, { "name": "apply_mounts", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 2566 + "line": 2594 + }, + { + "name": "inject_mounts", + "file": "crates/jp_cli/src/cmd/query.rs", + "line": 2612 }, { "name": "create_mount_effects", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 2639 + "line": 2678 }, { "name": "PendingStreamTrim::apply", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 2818 + "line": 2857 }, { "name": "mount_access_seed", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 2977 + "line": 3016 }, { "name": "read_piped_stdin", "file": "crates/jp_cli/src/cmd/query.rs", - "line": 3081 + "line": 3121 }, { "name": "Login::run_with_auth", @@ -2438,72 +2443,72 @@ { "name": "run_plugin", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 361 + "line": 352 }, { "name": "spawn_plugin", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 546 + "line": 537 }, { "name": "stop_plugin", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 643 + "line": 634 }, { "name": "join_within", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 666 + "line": 657 }, { "name": "message_loop", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 741 + "line": 732 }, { "name": "run_query", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 891 + "line": 882 }, { "name": "prepare_turn", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 1261 + "line": 1252 }, { "name": "handle_request", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 1762 + "line": 1753 }, { "name": "handle_write_draft", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 2099 + "line": 2096 }, { "name": "handle_list_configs", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 2197 + "line": 2194 }, { "name": "describe", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 2480 + "line": 2477 }, { "name": "manifest_differences", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 2516 + "line": 2513 }, { "name": "describe_plugin", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 2541 + "line": 2538 }, { "name": "run_external", "file": "crates/jp_cli/src/cmd/plugin/dispatch.rs", - "line": 2900 + "line": 2897 }, { "name": "render", @@ -2525,6 +2530,16 @@ "file": "crates/jp_cli/src/cmd/plugin/update.rs", "line": 26 }, + { + "name": "TurnPlugins::admit", + "file": "crates/jp_cli/src/cmd/plugin/tool.rs", + "line": 109 + }, + { + "name": "TurnPlugins::admit_from", + "file": "crates/jp_cli/src/cmd/plugin/tool.rs", + "line": 137 + }, { "name": "not_approved", "file": "crates/jp_cli/src/cmd/plugin/admission.rs", @@ -2533,7 +2548,7 @@ { "name": "admit", "file": "crates/jp_cli/src/cmd/plugin/admission.rs", - "line": 280 + "line": 283 }, { "name": "Install::run", @@ -2778,132 +2793,132 @@ { "name": "ToolCoordinator::pre_render_for_prompt", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 671 + "line": 672 }, { "name": "ToolCoordinator::render_executor", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 694 + "line": 692 }, { "name": "ToolCoordinator::abandon", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 792 + "line": 790 }, { "name": "ToolCoordinator::decide_permission", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 887 + "line": 885 }, { "name": "ToolCoordinator::submit", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 976 + "line": 974 }, { "name": "ToolCoordinator::add_call", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1017 + "line": 1015 }, { "name": "ToolCoordinator::finish", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1094 + "line": 1092 }, { "name": "ToolCoordinator::dispatch", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1334 + "line": 1332 }, { "name": "ToolCoordinator::on_step", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1443 + "line": 1441 }, { "name": "ToolCoordinator::on_permission", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1493 + "line": 1491 }, { "name": "ToolCoordinator::pump", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1527 + "line": 1525 }, { "name": "ToolCoordinator::decide", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1585 + "line": 1583 }, { "name": "ToolCoordinator::announce", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1638 + "line": 1636 }, { "name": "ToolCoordinator::release", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1660 + "line": 1658 }, { "name": "ToolCoordinator::route", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1734 + "line": 1732 }, { "name": "ToolCoordinator::answer", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1832 + "line": 1830 }, { "name": "ToolCoordinator::serve", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1846 + "line": 1844 }, { "name": "ToolCoordinator::deliver", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1934 + "line": 1932 }, { "name": "ToolCoordinator::close", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 1985 + "line": 1983 }, { "name": "ToolCoordinator::render_result", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 2227 + "line": 2225 }, { "name": "ToolCoordinator::preset_answer", "file": "crates/jp_cli/src/cmd/query/tool/coordinator.rs", - "line": 2321 + "line": 2319 }, { "name": "route", "file": "crates/jp_cli/src/cmd/query/tool/mcp_executor.rs", - "line": 489 + "line": 493 }, { "name": "TerminalExecutorSource::create", "file": "crates/jp_cli/src/cmd/query/tool/mcp_executor.rs", - "line": 654 + "line": 658 }, { "name": "TerminalExecutorSource::fan_out_executors", "file": "crates/jp_cli/src/cmd/query/tool/mcp_executor.rs", - "line": 723 + "line": 727 }, { "name": "PendingCall::receive", "file": "crates/jp_cli/src/cmd/query/tool/mcp_executor.rs", - "line": 1091 + "line": 1093 }, { "name": "PendingCall::receive_agent", "file": "crates/jp_cli/src/cmd/query/tool/mcp_executor.rs", - "line": 1123 + "line": 1125 }, { "name": "Grep::run", @@ -3198,17 +3213,17 @@ { "name": "TurnRenderer::new", "file": "crates/jp_cli/src/render/turn.rs", - "line": 107 + "line": 104 }, { "name": "TurnRenderer::render_turn", "file": "crates/jp_cli/src/render/turn.rs", - "line": 157 + "line": 154 }, { "name": "TurnRenderer::reconfigure", "file": "crates/jp_cli/src/render/turn.rs", - "line": 268 + "line": 264 }, { "name": "ToolRenderer::new", @@ -3433,7 +3448,7 @@ { "name": "register", "file": "crates/jp_mcp/src/server/service.rs", - "line": 527 + "line": 525 }, { "name": "Service::new", @@ -3443,37 +3458,37 @@ { "name": "validate_arguments", "file": "crates/jp_mcp/src/server/service.rs", - "line": 885 + "line": 887 }, { "name": "run_call", "file": "crates/jp_mcp/src/server/service.rs", - "line": 919 + "line": 921 }, { "name": "conclude", "file": "crates/jp_mcp/src/server/service.rs", - "line": 1100 + "line": 1115 }, { "name": "run_fan_out", "file": "crates/jp_mcp/src/server/service.rs", - "line": 1138 + "line": 1153 }, { "name": "deliver_result", "file": "crates/jp_mcp/src/server/service.rs", - "line": 1388 + "line": 1403 }, { "name": "attempt", "file": "crates/jp_mcp/src/server/service.rs", - "line": 1448 + "line": 1463 }, { "name": "ask_for_input", "file": "crates/jp_mcp/src/server/service.rs", - "line": 1521 + "line": 1537 }, { "name": "split", @@ -3523,37 +3538,37 @@ { "name": "run_tool_command", "file": "crates/jp_mcp/src/server.rs", - "line": 430 + "line": 433 }, { "name": "parse_command_output", "file": "crates/jp_mcp/src/server.rs", - "line": 552 + "line": 555 }, { - "name": "execute", + "name": "run_plugin", "file": "crates/jp_mcp/src/server.rs", - "line": 701 + "line": 901 }, { - "name": "execute_local", + "name": "plugin_result", "file": "crates/jp_mcp/src/server.rs", - "line": 748 + "line": 961 }, { "name": "tool_definitions", "file": "crates/jp_mcp/src/server.rs", - "line": 913 + "line": 1102 }, { "name": "resolve_tool", "file": "crates/jp_mcp/src/server.rs", - "line": 966 + "line": 1153 }, { "name": "resolve_mcp_tool", "file": "crates/jp_mcp/src/server.rs", - "line": 990 + "line": 1179 }, { "name": "MockEditorBackend::edit_text", diff --git a/Cargo.lock b/Cargo.lock index 6c4e4f3ca..f4b04bf4b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2622,6 +2622,7 @@ dependencies = [ "futures", "indexmap", "jp_config", + "jp_plugin", "jp_process", "jp_tool", "minijinja", diff --git a/crates/jp_cli/src/access/mount.rs b/crates/jp_cli/src/access/mount.rs index 37b31dd5e..c495864e5 100644 --- a/crates/jp_cli/src/access/mount.rs +++ b/crates/jp_cli/src/access/mount.rs @@ -7,7 +7,7 @@ //! - `MODE` is `ro` (default) or `rw`. //! `rw` requires an explicit `TOOL:` prefix. //! - `TOOL:` scopes the grant to a single tool; without it the grant expands to -//! all enabled local tools. +//! all enabled local and command plugin tools. use camino::{Utf8Component, Utf8Path, Utf8PathBuf}; use jp_config::conversation::tool::access::FsRuleConfig; @@ -24,7 +24,8 @@ pub enum MountMode { /// A parsed `--mount` specification. #[derive(Debug, Clone, PartialEq, Eq)] pub struct MountSpec { - /// Single tool the grant applies to, or `None` for all enabled local tools. + /// Single tool the grant applies to, or `None` for all enabled local and + /// command plugin tools. pub tool: Option, /// Workspace-relative symlink location, as typed (relative to CWD). pub name: String, diff --git a/crates/jp_cli/src/cmd/plugin.rs b/crates/jp_cli/src/cmd/plugin.rs index e3d3196de..aaac9f414 100644 --- a/crates/jp_cli/src/cmd/plugin.rs +++ b/crates/jp_cli/src/cmd/plugin.rs @@ -6,6 +6,7 @@ //! subcommands, which need no workspace //! - External plugin dispatch: routing an unknown subcommand to the plugin //! whose manifest claims it, and spawning it +//! - Tool calls served by plugins (`source = "plugin.command."`) //! //! See: `docs/rfd/072-command-plugin-system.md` @@ -22,6 +23,7 @@ mod process_tree; pub(crate) mod registry; mod revoke; pub(crate) mod routing; +pub(crate) mod tool; mod uninstall; mod update; diff --git a/crates/jp_cli/src/cmd/plugin/admission.rs b/crates/jp_cli/src/cmd/plugin/admission.rs index 3ab044fa2..9b9dbafab 100644 --- a/crates/jp_cli/src/cmd/plugin/admission.rs +++ b/crates/jp_cli/src/cmd/plugin/admission.rs @@ -273,6 +273,9 @@ pub(crate) struct Official<'a> { /// Decide whether `plugin` may run, asking the user where the policy says to. /// /// Records an approval when the user answers `Y`. +/// Returns the SHA-256 of the contents the decision was made on, read once +/// before deciding: a binary replaced while the prompt is open is not the one +/// that was admitted. /// /// # Errors /// @@ -284,7 +287,7 @@ pub(crate) fn admit( approvals: &mut ApprovalStore, interactive: bool, printer: &Printer, -) -> cmd::Output { +) -> Result { check_protocol(plugin)?; let config = plugins_config.command.get(&plugin.name); @@ -307,7 +310,7 @@ pub(crate) fn admit( let reason = match decide(&candidate, config) { Verdict::Run => { debug!(name = plugin.name, path = %plugin.path, "Plugin admitted."); - return Ok(()); + return Ok(sha256); } Verdict::Refuse(reason) => return Err(reason.into()), Verdict::Ask(reason) => reason, @@ -333,14 +336,17 @@ pub(crate) fn admit( .map_err(|e| cmd::Error::from(format!("prompt failed: {e}")))?; match answer { - 'y' => Ok(()), - 'Y' => approvals.record(&plugin.name, ApprovedPlugin { - path: plugin.path.clone(), - sha256, - approved_at: Utc::now(), - installed: false, - manifest: None, - }), + 'y' => Ok(sha256), + 'Y' => { + approvals.record(&plugin.name, ApprovedPlugin { + path: plugin.path.clone(), + sha256: sha256.clone(), + approved_at: Utc::now(), + installed: false, + manifest: None, + })?; + Ok(sha256) + } _ => Err("plugin execution denied".into()), } } diff --git a/crates/jp_cli/src/cmd/plugin/admission_tests.rs b/crates/jp_cli/src/cmd/plugin/admission_tests.rs index f1e906869..8561317b6 100644 --- a/crates/jp_cli/src/cmd/plugin/admission_tests.rs +++ b/crates/jp_cli/src/cmd/plugin/admission_tests.rs @@ -136,6 +136,7 @@ fn a_sha1_pin_is_compared_with_the_binarys_sha1() { false, &printer ) + .map(drop) .map_err(|error| error.message.unwrap_or_default()), Ok(()) ); @@ -150,6 +151,7 @@ fn a_sha1_pin_is_compared_with_the_binarys_sha1() { false, &printer ) + .map(drop) .map_err(|error| error.message.unwrap_or_default()), Err(format!( "plugin `webui` binary checksum mismatch.\nexpected: bbb\nactual: \ @@ -327,6 +329,7 @@ fn without_a_terminal_only_an_approved_binary_runs() { false, &printer, ) + .map(drop) .map_err(|error| error.message.unwrap_or_default()) }; @@ -360,6 +363,44 @@ fn without_a_terminal_only_an_approved_binary_runs() { ); } +/// Admission hands back the digest it decided on, so a caller that pins the +/// binary for later checks pins the approved contents, not whatever is on disk +/// by the time it reads the file again. +#[test] +fn admission_returns_the_digest_it_decided_on() { + let tmp = tempdir().unwrap(); + let path = tmp.path().join("jp-webui"); + std::fs::write(&path, "v1").unwrap(); + let plugin = LocalPlugin { + path: path.clone(), + ..plugin("webui") + }; + let approved = registry::sha256_file(&path).unwrap(); + let mut approvals = ApprovalStore::load_from(Some(tmp.path().join("approvals.json"))); + approvals + .record("webui", ApprovedPlugin { + path: path.clone(), + sha256: approved.clone(), + approved_at: Utc::now(), + installed: false, + manifest: None, + }) + .unwrap(); + let (printer, _out, _err) = Printer::memory(jp_printer::OutputFormat::Text); + + let decided = admit( + &plugin, + Official::default(), + &AppConfig::new_test().plugins, + &mut approvals, + false, + &printer, + ) + .map_err(|error| error.message.unwrap_or_default()); + + assert_eq!(decided, Ok(approved)); +} + #[test] fn without_a_terminal_the_refusal_says_how_to_approve() { let plugin = plugin("webui"); diff --git a/crates/jp_cli/src/cmd/plugin/dispatch.rs b/crates/jp_cli/src/cmd/plugin/dispatch.rs index d001fb429..3c5cf1b1a 100644 --- a/crates/jp_cli/src/cmd/plugin/dispatch.rs +++ b/crates/jp_cli/src/cmd/plugin/dispatch.rs @@ -273,21 +273,11 @@ fn init_message( ) -> Result<(HostToPlugin, Value), cmd::Error> { let config_json = config_value(config)?; - let options: serde_json::Map = config - .plugins - .command - .get(name) - .map(|c| { - c.options - .iter() - .map(|(k, v)| (k.clone(), v.0.clone())) - .collect() - }) - .unwrap_or_default(); + let options = config.plugins.command_options(name); let storage = paths.storage.ok_or("workspace has no storage configured")?; - let init = HostToPlugin::Init(InitMessage { + let init = HostToPlugin::Init(Box::new(InitMessage { version: PROTOCOL_VERSION, workspace: WorkspaceInfo { root: workspace.root().to_owned(), @@ -300,7 +290,8 @@ fn init_message( args: args.to_vec(), log_level, output_format: output_format(format), - }); + tool: None, + })); Ok((init, config_json)) } @@ -691,7 +682,7 @@ fn output_format(format: OutputFormat) -> PluginOutputFormat { /// The JP directories a plugin is told about, so it needs no platform logic of /// its own. -fn well_known_paths(user_storage_path: Option<&Utf8Path>) -> PathsInfo { +pub(crate) fn well_known_paths(user_storage_path: Option<&Utf8Path>) -> PathsInfo { let home = std::env::home_dir().and_then(|p| camino::Utf8PathBuf::from_path_buf(p).ok()); PathsInfo { @@ -1835,6 +1826,12 @@ fn handle_request( debug!("Ignoring describe in message loop."); } + // Only meaningful when `init` carried a tool call, which a command run + // never does. + PluginToHost::ToolOutcome(_) => { + debug!("Ignoring a tool outcome from a plugin running a command."); + } + // Answered by the caller, before the lock this runs under is taken. PluginToHost::Compose(_) | PluginToHost::Query(_) diff --git a/crates/jp_cli/src/cmd/plugin/routing.rs b/crates/jp_cli/src/cmd/plugin/routing.rs index 6b52d50b9..1f1fef13f 100644 --- a/crates/jp_cli/src/cmd/plugin/routing.rs +++ b/crates/jp_cli/src/cmd/plugin/routing.rs @@ -292,6 +292,45 @@ pub(crate) fn route<'a>( }) } +/// A plugin found by its name, for a tool whose source names it. +#[derive(Debug, PartialEq)] +pub(crate) struct Named<'a> { + /// The binary. + pub plugin: &'a LocalPlugin, + + /// The official registry entry whose `id` is the plugin's name, when it is + /// one. + pub official: Option<&'a RegistryPlugin>, +} + +/// Find the plugin named `name`, the way a tool's `plugin.command.` +/// source names it. +/// +/// A name, not a command path: a tool reaches a plugin by its identity, which +/// keys its configuration and its approval, whatever command it claims. +/// Returns `None` when no binary on this machine has that name. +/// +/// # Errors +/// +/// [`RouteError::SameName`] when two binaries share it. +pub(crate) fn by_name<'a>( + name: &str, + local: &'a [LocalPlugin], + registry: Option<&'a Registry>, +) -> Result>, RouteError> { + check_unique_name(local, name)?; + + let Some(plugin) = local.iter().find(|plugin| plugin.name == name) else { + return Ok(None); + }; + + let official = official_entries(registry) + .into_values() + .find(|entry| entry.kind.is_command() && entry.id == name); + + Ok(Some(Named { plugin, official })) +} + /// The registry's official entries, by key. /// /// Only these claim commands; a third-party entry is a catalog entry. diff --git a/crates/jp_cli/src/cmd/plugin/routing_tests.rs b/crates/jp_cli/src/cmd/plugin/routing_tests.rs index 5cdc7ec6a..54b3a2658 100644 --- a/crates/jp_cli/src/cmd/plugin/routing_tests.rs +++ b/crates/jp_cli/src/cmd/plugin/routing_tests.rs @@ -30,6 +30,71 @@ fn plugin(name: &str, dir: &str, command: &[&str]) -> LocalPlugin { } } +/// A tool names its plugin by name, and the lookup finds that binary and +/// nothing else. +#[test] +fn by_name_finds_the_plugin_with_that_name() { + let local = [ + plugin("ticket", "/bin", &["ticket"]), + plugin("metrics", "/bin", &["metrics"]), + ]; + + let named = by_name("ticket", &local, None).unwrap().unwrap(); + + assert_eq!(named.plugin.path, Utf8PathBuf::from("/bin/jp-ticket")); + assert_eq!(named.official, None); +} + +#[test] +fn by_name_finds_nothing_for_an_absent_plugin() { + let local = [plugin("metrics", "/bin", &["metrics"])]; + + assert_eq!(by_name("ticket", &local, None).unwrap(), None); +} + +/// Two binaries with one name would share one approval; a tool call refuses +/// both, as a command does. +#[test] +fn by_name_refuses_a_name_two_binaries_share() { + let local = [ + plugin("ticket", "/install", &["ticket"]), + plugin("ticket", "/bin", &["tickets"]), + ]; + + assert_eq!( + by_name("ticket", &local, None).unwrap_err(), + RouteError::SameName { + name: "ticket".to_owned(), + paths: vec!["/install/jp-ticket".into(), "/bin/jp-ticket".into()], + } + ); +} + +/// The official entry decides whether the binary is the published release, so +/// it travels with the plugin; a third-party catalog entry with the same id +/// does not make a binary official. +#[test] +fn by_name_carries_the_official_entry() { + let local = [ + plugin("serve-web", "/install", &["serve", "web"]), + plugin("metrics", "/bin", &["metrics"]), + ]; + let registry = registry(); + + let official = by_name("serve-web", &local, Some(®istry)) + .unwrap() + .unwrap(); + let third_party = by_name("metrics", &local, Some(®istry)) + .unwrap() + .unwrap(); + + assert_eq!( + official.official.map(|entry| entry.id.as_str()), + Some("serve-web") + ); + assert_eq!(third_party.official, None); +} + fn without_manifest(name: &str, dir: &str) -> LocalPlugin { LocalPlugin { manifest: ManifestState::Missing, diff --git a/crates/jp_cli/src/cmd/plugin/tool.rs b/crates/jp_cli/src/cmd/plugin/tool.rs new file mode 100644 index 000000000..e9ff36778 --- /dev/null +++ b/crates/jp_cli/src/cmd/plugin/tool.rs @@ -0,0 +1,321 @@ +//! Admitting the command plugins a turn's tools run through. +//! +//! A turn whose tools run through command plugins (`source = +//! "plugin.command."`) admits each of those plugins once, before the +//! turn starts, with [`admit_turn_plugins`]: the same trust decision `jp +//! ` makes, asked at a point where a prompt cannot open inside a tool +//! call. +//! The turn says which plugins were refused and why +//! ([`report_refused_plugins`]), and fails when the tool `--tool` names is one +//! of theirs ([`refuse_forced_tool`]). +//! +//! The admitted plugins go to the tool service as [`CommandPlugins`], which +//! runs each call: it starts the admitted binary with an `init` carrying the +//! call, the turn's configuration, and the plugin's options from it, and reads +//! back one `tool_outcome`. +//! A tool on any plugin not handed over is left out of the turn +//! ([`without_unadmitted_plugins`]). +//! The plugin resolves no configuration of its own, so what it sees is what the +//! query making the call sees: nested `.jp.toml` files, conversation config, +//! and `--cfg` overrides included. +//! +//! See: `docs/rfd/072-command-plugin-system.md`, "Tool Calls". + +use std::collections::{BTreeSet, HashMap}; + +use crossterm::style::Stylize as _; +use indexmap::IndexMap; +use jp_config::{ + AppConfig, + conversation::tool::{ToolConfigWithDefaults, ToolSource, ToolsConfig}, + plugins::PluginsConfig, +}; +use jp_mcp::server::{AdmittedPlugin, CommandPlugins, PluginInit, is_offered}; +use jp_plugin::registry::Registry; +use jp_printer::{PrintableExt as _, Printer}; +use jp_tool::Error as ToolError; +use tracing::{debug, warn}; + +use super::{ + admission::{Official, admit}, + approvals::ApprovalStore, + discovery::LocalPlugin, + dispatch::local_plugins, + registry, routing, +}; + +/// Admit the command plugins a turn's tools run through, and hand the admitted +/// ones over with `init`. +/// +/// Says which plugins were refused, and why, before the turn starts. +/// +/// # Errors +/// +/// [`ToolError::CommandPluginUnavailable`] when the tool `--tool` names runs +/// through a refused plugin. +pub(crate) fn admit_turn_plugins( + config: &AppConfig, + interactive: bool, + printer: &Printer, + init: PluginInit, +) -> Result { + let tools = &config.conversation.tools; + let forced_tool = config.assistant.tool_choice.function_name(); + + let plugins = TurnPlugins::admit( + &plugins_needed(tools, forced_tool), + &config.plugins, + interactive, + printer, + ); + report_refused_plugins(printer, tools, plugins.refused(), forced_tool); + refuse_forced_tool(tools, plugins.refused(), forced_tool)?; + + Ok(plugins.into_command_plugins(init)) +} + +/// The plugins the offered tools of a turn run through. +/// +/// Mirrors which tools the turn offers the assistant: an enabled tool, or the +/// tool `forced_tool` names unless it is locked off. +pub(crate) fn plugins_needed(tools: &ToolsConfig, forced_tool: Option<&str>) -> BTreeSet { + tools + .iter() + .filter(|(name, config)| is_offered(name, config, forced_tool)) + .filter_map(|(_, config)| match config.source() { + ToolSource::CommandPlugin { plugin, .. } => Some(plugin.clone()), + _ => None, + }) + .collect() +} + +/// What admission decided about each plugin a turn's tools run through. +#[derive(Debug, Default)] +pub(crate) struct TurnPlugins { + admitted: HashMap, + + /// Plugins that may not run this turn, and why, in the order they were + /// decided. + refused: IndexMap, +} + +impl TurnPlugins { + /// Admit each plugin in `needed`, the way `jp ` admits one. + /// + /// With `interactive`, a binary nobody approved is asked about; without, it + /// is refused. + /// Nothing is installed: an official plugin that is missing is refused, not + /// fetched as the turn starts. + pub(crate) fn admit( + needed: &BTreeSet, + plugins_config: &PluginsConfig, + interactive: bool, + printer: &Printer, + ) -> Self { + // Discovery reads every `jp-*` binary on `$PATH`; a turn with no + // plugin tools has no reason to. + if needed.is_empty() { + return Self::default(); + } + + let mut approvals = ApprovalStore::load(); + let local = local_plugins(&approvals); + let registry = registry::load_cached(); + + Self::admit_from(needed, Admitter { + local: &local, + registry: registry.as_ref(), + plugins_config, + approvals: &mut approvals, + interactive, + printer, + }) + } + + /// [`Self::admit`], with the binaries, the registry, and the approvals + /// passed in. + fn admit_from(needed: &BTreeSet, mut admitter: Admitter<'_>) -> Self { + let mut plugins = Self::default(); + + for name in needed { + match admitter.admit(name) { + Ok(admitted) => { + debug!(plugin = name, binary = %admitted.binary, "Plugin admitted for the turn."); + plugins.admitted.insert(name.clone(), admitted); + } + Err(reason) => { + warn!(plugin = name, %reason, "Plugin not admitted; its tools are left out."); + plugins.refused.insert(name.clone(), reason); + } + } + } + + plugins + } + + /// The plugins that may not run this turn, and why. + pub(crate) fn refused(&self) -> &IndexMap { + &self.refused + } + + /// The admitted plugins, as the tool service runs them, with `init` shared + /// by every call. + pub(crate) fn into_command_plugins(self, init: PluginInit) -> CommandPlugins { + self.admitted + .into_iter() + .fold(CommandPlugins::new(init), |plugins, (name, admitted)| { + plugins.with(name, admitted) + }) + } +} + +/// What admitting a plugin by name decides against: the binaries on this +/// machine, the official registry, the run policy, and the saved approvals. +struct Admitter<'a> { + local: &'a [LocalPlugin], + registry: Option<&'a Registry>, + plugins_config: &'a PluginsConfig, + approvals: &'a mut ApprovalStore, + + /// Whether a binary nobody approved is asked about, rather than refused. + interactive: bool, + printer: &'a Printer, +} + +impl Admitter<'_> { + /// Admit one plugin by name. + /// + /// The error is the reason, written for the user. + fn admit(&mut self, name: &str) -> Result { + let named = routing::by_name(name, self.local, self.registry) + .map_err(|error| error.to_string())? + .ok_or_else(|| { + format!("no `jp-{name}` binary in the plugin install directory or on $PATH") + })?; + + let official = Official { + official: named.official.is_some(), + sha256: named + .official + .and_then(registry::release) + .map(|release| release.sha256.as_str()), + replaces: None, + }; + + // The digest admission decided on, not a second read of the file: a + // binary replaced while the prompt was open must fail the per-call + // check, not become the turn's baseline. + let sha256 = admit( + named.plugin, + official, + self.plugins_config, + self.approvals, + self.interactive, + self.printer, + ) + .map_err(error_message)?; + + Ok(AdmittedPlugin { + binary: named.plugin.path.clone(), + sha256, + }) + } +} + +/// The sentence a command error carries, without its exit code. +fn error_message(error: crate::cmd::Error) -> String { + error + .message + .unwrap_or_else(|| format!("failed with exit code {}", error.code)) +} + +/// The turn's tools, without those on a plugin `plugins` does not hold. +pub(crate) fn without_unadmitted_plugins<'a>( + tools: impl Iterator, + plugins: &CommandPlugins, +) -> Vec<(&'a str, ToolConfigWithDefaults)> { + tools + .filter(|(_, config)| match config.source() { + ToolSource::CommandPlugin { plugin, .. } => plugins.is_admitted(plugin), + _ => true, + }) + .collect() +} + +/// Fail when `forced_tool` runs through a refused plugin. +/// +/// # Errors +/// +/// [`ToolError::CommandPluginUnavailable`], with the refusal's reason: the user +/// asked for that tool, and a turn without it is not what they asked for. +pub(crate) fn refuse_forced_tool( + tools: &ToolsConfig, + refused: &IndexMap, + forced_tool: Option<&str>, +) -> Result<(), ToolError> { + let Some(config) = forced_tool.and_then(|name| tools.get(name)) else { + return Ok(()); + }; + let ToolSource::CommandPlugin { plugin, .. } = config.source() else { + return Ok(()); + }; + let Some(reason) = refused.get(plugin) else { + return Ok(()); + }; + + Err(ToolError::CommandPluginUnavailable { + plugin: plugin.clone(), + reason: reason.clone(), + }) +} + +/// Report the plugins a turn left out, and the tools that went with them. +/// +/// Emitted whatever the progress settings say, as a skipped MCP server is: a +/// tool silently missing from the turn is the failure this reports. +pub(crate) fn report_refused_plugins( + printer: &Printer, + tools: &ToolsConfig, + refused: &IndexMap, + forced_tool: Option<&str>, +) { + for (plugin, reason) in refused { + let names: Vec = tools + .iter() + .filter(|(name, config)| is_offered(name, config, forced_tool)) + .filter(|(_, config)| { + matches!(config.source(), ToolSource::CommandPlugin { plugin: p, .. } if p == plugin) + }) + .map(|(name, _)| name.to_owned()) + .collect(); + + if printer.format().is_json() { + let record = serde_json::json!({ + "event": "plugin_unavailable", + "plugin": plugin, + "reason": reason, + "tools": names, + }); + let line = if printer.format().is_json_pretty() { + serde_json::to_string_pretty(&record) + } else { + serde_json::to_string(&record) + } + .unwrap_or_else(|_| record.to_string()); + printer.println_raw(line.to_err()); + continue; + } + + let mut line = format!("Plugin '{plugin}' did not run"); + if !names.is_empty() { + line.push_str("; unavailable tools: "); + line.push_str(&names.join(", ")); + } + printer.eprintln(line.yellow().to_string()); + printer.eprintln(format!(" {reason}")); + } +} + +#[cfg(test)] +#[path = "tool_tests.rs"] +mod tests; diff --git a/crates/jp_cli/src/cmd/plugin/tool_tests.rs b/crates/jp_cli/src/cmd/plugin/tool_tests.rs new file mode 100644 index 000000000..dc8c61c6e --- /dev/null +++ b/crates/jp_cli/src/cmd/plugin/tool_tests.rs @@ -0,0 +1,404 @@ +use std::{fs, sync::Arc}; + +use camino::Utf8PathBuf; +use camino_tempfile::{Utf8TempDir, tempdir}; +use chrono::Utc; +use jp_config::{ + AppConfig, PartialAppConfig, + assignment::{AssignKeyValue as _, KvAssignment}, +}; +use jp_plugin::{message::PathsInfo, registry::ApprovedPlugin}; +use jp_printer::OutputFormat; +use pretty_assertions::assert_eq; +use serde_json::{Value, json}; + +use super::*; +use crate::cmd::plugin::discovery::{Location, ManifestState}; + +/// The configuration a turn resolved, with `assignments` applied as `--cfg` +/// would apply them. +fn turn_config(assignments: &[&str]) -> Arc { + let mut partial = PartialAppConfig::new_test(); + for assignment in assignments { + partial + .assign(assignment.parse::().unwrap()) + .unwrap(); + } + + Arc::new(jp_config::util::build(partial).unwrap()) +} + +/// What every plugin call of a turn under `config` is told. +fn plugin_init(config: Arc) -> PluginInit { + PluginInit { + workspace_id: "ws-abc".to_owned(), + storage: Some("/ws/.jp".into()), + paths: PathsInfo::default(), + config, + log_level: 0, + } +} + +// --- Which plugins a turn needs --- + +/// A turn's tools: two on `ticket`, one disabled on `metrics`, one locked off +/// on `secrets`, and a local tool. +fn turn_tools() -> Arc { + turn_config(&[ + "conversation.tools.ticket_create.source=plugin.command.ticket.create", + "conversation.tools.ticket_show.source=plugin.command.ticket.show", + "conversation.tools.metrics_dump.source=plugin.command.metrics", + "conversation.tools.metrics_dump.enable=false", + "conversation.tools.secrets_read.source=plugin.command.secrets", + "conversation.tools.secrets_read.enable.state=false", + "conversation.tools.secrets_read.enable.allow_toggle=never", + "conversation.tools.word_count.source=local", + "conversation.tools.word_count.command=wc", + ]) +} + +#[test] +fn a_turn_needs_the_plugins_its_enabled_tools_run_through() { + let config = turn_tools(); + + let needed = plugins_needed(&config.conversation.tools, None); + + assert_eq!(needed, BTreeSet::from(["ticket".to_owned()])); +} + +/// A forced tool is offered even when disabled, so its plugin is needed too; a +/// locked-off one is never offered, forced or not. +#[test] +fn a_forced_tool_adds_its_plugin_unless_it_is_locked_off() { + let config = turn_tools(); + + let forced = plugins_needed(&config.conversation.tools, Some("metrics_dump")); + let locked = plugins_needed(&config.conversation.tools, Some("secrets_read")); + + assert_eq!( + forced, + BTreeSet::from(["metrics".to_owned(), "ticket".to_owned()]) + ); + assert_eq!(locked, BTreeSet::from(["ticket".to_owned()])); +} + +// --- Admission --- + +/// A plugin binary in a temporary directory, and an approval store beside it. +struct Machine { + dir: Utf8TempDir, + plugins: Vec, +} + +impl Machine { + fn new() -> Self { + Self { + dir: tempdir().unwrap(), + plugins: vec![], + } + } + + /// Put a `jp-{name}` binary on this machine, with `contents`. + fn with_plugin(mut self, name: &str, contents: &str) -> Self { + let path = self.dir.path().join(format!("jp-{name}")); + fs::write(&path, contents).unwrap(); + self.plugins.push(LocalPlugin { + name: name.to_owned(), + path: path.canonicalize_utf8().unwrap(), + location: Location::Path, + manifest: ManifestState::Missing, + }); + self + } + + fn path(&self, name: &str) -> Utf8PathBuf { + self.dir + .path() + .join(format!("jp-{name}")) + .canonicalize_utf8() + .unwrap() + } + + fn approvals(&self) -> ApprovalStore { + ApprovalStore::load_from(Some(self.dir.path().join("approvals.json"))) + } + + /// Record an approval for the binary `name` as it is now. + fn approve(&self, approvals: &mut ApprovalStore, name: &str) { + let path = self.path(name); + approvals + .record(name, ApprovedPlugin { + sha256: registry::sha256_file(&path).unwrap(), + path, + approved_at: Utc::now(), + installed: false, + manifest: None, + }) + .unwrap(); + } + + /// Admit `needed` without a terminal, under `config`. + fn admit(&self, needed: &[&str], config: &AppConfig) -> TurnPlugins { + self.admit_with(needed, config, &mut self.approvals()) + } + + fn admit_with( + &self, + needed: &[&str], + config: &AppConfig, + approvals: &mut ApprovalStore, + ) -> TurnPlugins { + let needed = needed.iter().map(|name| (*name).to_owned()).collect(); + + TurnPlugins::admit_from(&needed, Admitter { + local: &self.plugins, + registry: None, + plugins_config: &config.plugins, + approvals, + interactive: false, + printer: &Printer::sink(), + }) + } +} + +#[test] +fn a_plugin_allowed_by_configuration_is_admitted() { + let machine = Machine::new().with_plugin("ticket", "v1"); + + let plugins = machine.admit( + &["ticket"], + &turn_config(&["plugins.command.ticket.run=allow"]), + ); + + assert!(plugins.refused().is_empty(), "{:?}", plugins.refused()); + assert_eq!(plugins.admitted["ticket"].binary, machine.path("ticket")); +} + +/// What admission recorded is what the tool service checks before each call, so +/// an admitted binary that has not changed runs. +#[test] +fn an_admitted_plugin_passes_the_check_before_each_call() { + let machine = Machine::new().with_plugin("ticket", "v1"); + let plugins = machine.admit( + &["ticket"], + &turn_config(&["plugins.command.ticket.run=allow"]), + ); + + let command_plugins = plugins.into_command_plugins(plugin_init(turn_config(&[]))); + + assert_eq!( + command_plugins.verify("ticket").map(|p| p.binary.clone()), + Ok(machine.path("ticket")) + ); +} + +/// Without a terminal, a binary nobody approved does not run, exactly as `jp +/// ticket` without a terminal refuses it. +#[test] +fn an_unapproved_plugin_is_refused_without_a_terminal() { + let machine = Machine::new().with_plugin("ticket", "v1"); + let path = machine.path("ticket"); + + let plugins = machine.admit(&["ticket"], &turn_config(&[])); + + assert!(plugins.admitted.is_empty()); + assert_eq!( + plugins.refused(), + &IndexMap::from([( + "ticket".to_owned(), + format!( + "plugin `ticket` at {path} is not approved. Approve it with `jp plugin approve \ + {path}`, or set plugins.command.ticket.run = \"allow\" in config." + ) + )]) + ); +} + +/// An approval recorded with `jp plugin approve`, or `Y` at the prompt, admits +/// the binary while its contents are the approved ones. +#[test] +fn an_approved_plugin_is_admitted() { + let machine = Machine::new().with_plugin("ticket", "v1"); + let mut approvals = machine.approvals(); + machine.approve(&mut approvals, "ticket"); + + let plugins = machine.admit_with(&["ticket"], &turn_config(&[]), &mut approvals); + + assert!(plugins.refused().is_empty(), "{:?}", plugins.refused()); + assert!(plugins.admitted.contains_key("ticket")); +} + +/// The turn's baseline is the digest admission approved. +/// A binary replaced after that is refused at its next call rather than trusted +/// for the rest of the turn. +#[test] +fn the_turn_pins_the_approved_contents() { + let machine = Machine::new().with_plugin("ticket", "v1"); + let mut approvals = machine.approvals(); + machine.approve(&mut approvals, "ticket"); + let approved = registry::sha256_file(&machine.path("ticket")).unwrap(); + + let plugins = machine.admit_with(&["ticket"], &turn_config(&[]), &mut approvals); + assert_eq!(plugins.admitted["ticket"].sha256, approved); + + fs::write(machine.path("ticket"), "v2").unwrap(); + let command_plugins = plugins.into_command_plugins(plugin_init(turn_config(&[]))); + + assert_eq!( + command_plugins.verify("ticket").map(|p| p.binary.clone()), + Err(format!( + "{} changed since it was admitted at the start of this turn", + machine.path("ticket") + )) + ); +} + +#[test] +fn a_denied_plugin_is_refused_even_when_approved() { + let machine = Machine::new().with_plugin("ticket", "v1"); + let mut approvals = machine.approvals(); + machine.approve(&mut approvals, "ticket"); + + let plugins = machine.admit_with( + &["ticket"], + &turn_config(&["plugins.command.ticket.run=deny"]), + &mut approvals, + ); + + assert!(plugins.admitted.is_empty()); + assert_eq!( + plugins.refused()["ticket"], + "plugin `ticket` is denied by configuration (plugins.command.ticket.run = \"deny\")" + ); +} + +#[test] +fn a_missing_plugin_is_refused() { + let machine = Machine::new(); + + let plugins = machine.admit(&["ticket"], &turn_config(&[])); + + assert_eq!( + plugins.refused()["ticket"], + "no `jp-ticket` binary in the plugin install directory or on $PATH" + ); +} + +/// A refused plugin is not handed to the tool service, so its tools cannot run +/// even if one were offered. +#[test] +fn a_refused_plugin_is_not_handed_to_the_tool_service() { + let machine = Machine::new().with_plugin("ticket", "v1"); + + let command_plugins = machine + .admit(&["ticket"], &turn_config(&[])) + .into_command_plugins(plugin_init(turn_config(&[]))); + + assert_eq!( + command_plugins.verify("ticket").map(|p| p.binary.clone()), + Err("it was not admitted for this turn".to_owned()) + ); +} + +// --- Leaving refused plugins out of the turn --- + +fn refused_ticket() -> IndexMap { + IndexMap::from([("ticket".to_owned(), "it is not approved".to_owned())]) +} + +/// The tool service is handed only admitted plugins, so a tool on any other +/// plugin is left out of the turn: refused, or never needed because the tool is +/// not offered. +#[test] +fn a_tool_on_a_plugin_not_admitted_is_left_out_of_the_turn() { + let config = turn_tools(); + let names = |plugins: &CommandPlugins| { + let mut names: Vec<&str> = + without_unadmitted_plugins(config.conversation.tools.iter(), plugins) + .into_iter() + .map(|(name, _)| name) + .collect(); + names.sort_unstable(); + names + }; + let ticket = + CommandPlugins::new(plugin_init(turn_config(&[]))).with("ticket", AdmittedPlugin { + binary: "/bin/jp-ticket".into(), + sha256: "aaa".to_owned(), + }); + + assert_eq!(names(&CommandPlugins::default()), ["word_count"]); + assert_eq!(names(&ticket), [ + "ticket_create", + "ticket_show", + "word_count" + ]); +} + +/// Naming a tool with `--tool` asks for it; a turn without it is not what was +/// asked for, so the query fails with the reason. +#[test] +fn forcing_a_tool_of_a_refused_plugin_fails_the_turn() { + let config = turn_tools(); + let tools = &config.conversation.tools; + + let error = refuse_forced_tool(tools, &refused_ticket(), Some("ticket_show")).unwrap_err(); + + assert_eq!( + error.to_string(), + "Command plugin `ticket` cannot be run from here: it is not approved" + ); +} + +/// Only the forced tool's own plugin matters: a refused plugin the forced tool +/// does not run through leaves the turn to start without it. +#[test] +fn forcing_a_tool_of_another_source_starts_the_turn() { + let config = turn_tools(); + let tools = &config.conversation.tools; + + assert!(refuse_forced_tool(tools, &refused_ticket(), Some("word_count")).is_ok()); + assert!(refuse_forced_tool(tools, &refused_ticket(), None).is_ok()); +} + +#[test] +fn refused_plugin_report_names_the_tools_and_the_reason() { + let (printer, _out, err) = Printer::memory(OutputFormat::Text); + + report_refused_plugins( + &printer, + &turn_tools().conversation.tools, + &refused_ticket(), + None, + ); + printer.flush(); + + assert_eq!( + *err.lock(), + "Plugin 'ticket' did not run; unavailable tools: ticket_create, ticket_show\n it is not \ + approved\n" + ); +} + +#[test] +fn refused_plugin_report_is_ndjson_under_json_format() { + let (printer, _out, err) = Printer::memory(OutputFormat::Json); + + report_refused_plugins( + &printer, + &turn_tools().conversation.tools, + &refused_ticket(), + None, + ); + printer.flush(); + + assert_eq!( + serde_json::from_str::(err.lock().trim()).unwrap(), + json!({ + "event": "plugin_unavailable", + "plugin": "ticket", + "reason": "it is not approved", + "tools": ["ticket_create", "ticket_show"], + }) + ); +} diff --git a/crates/jp_cli/src/cmd/query.rs b/crates/jp_cli/src/cmd/query.rs index 5bacafcb5..26e9e80cd 100644 --- a/crates/jp_cli/src/cmd/query.rs +++ b/crates/jp_cli/src/cmd/query.rs @@ -107,6 +107,7 @@ use jp_llm::{event::NoticeSink, provider}; use jp_mcp::{ StartupSet, server::{ + CommandPlugins, PluginInit, builtin::{BuiltinExecutors, describe_tools::DescribeTools}, tool_definitions, }, @@ -147,6 +148,10 @@ use crate::{ conversation::fork, label::resolve::{Resolver, Trigger}, lock::{LockRequest, acquire_lock}, + plugin::{ + dispatch::well_known_paths, + tool::{admit_turn_plugins, without_unadmitted_plugins}, + }, }, config_pipeline::{self, ConfigReset, ConfigResetEvents}, ctx::{IntoPartialAppConfig, McpServerScope}, @@ -385,7 +390,7 @@ pub(crate) struct Query { /// `NAME` is the workspace-relative location for the symlink, `PATH` is the /// external target, and `MODE` is `ro` (default) or `rw`. /// `rw` requires a `TOOL:` prefix; without a `TOOL:` prefix the grant - /// applies to all enabled local tools. + /// applies to all enabled local and command plugin tools. /// Repeat the flag to mount several paths. #[arg(long = "mount", value_name = "[TOOL:]NAME=PATH[:MODE]", action = ArgAction::Append)] mount: Vec, @@ -419,7 +424,7 @@ impl Query { staged, } = self.acquire_lock(ctx, handle, start_new).await?; - let result = self.run_locked(ctx, &lock, query, fresh, staged).await; + let result = Box::pin(self.run_locked(ctx, &lock, query, fresh, staged)).await; // A run that never started a turn wrote nothing, so a directory the // editor created to compose in is all that is left of the conversation. @@ -1178,6 +1183,7 @@ impl Query { tools: &[ToolDefinition], printer: Arc, approvals: Arc, + command_plugins: CommandPlugins, chat_request: ChatRequest, invocation: InvocationContext, pending_trim: PendingStreamTrim, @@ -1226,6 +1232,7 @@ impl Query { let (executor_source, execution_owner) = TerminalExecutorSource::start_with_metadata( builtin_executors, Arc::new(SystemProcessRunner), + command_plugins, tools, &cfg.conversation.tools, approvals, @@ -1243,8 +1250,6 @@ impl Query { .with_interrupt(cfg.interrupt.tool_call.clone()); let prompt_backend = Arc::new(TerminalPromptBackend); - // Boxed because the loop's state machine is large, and inlining it - // would put the whole of it in every future that awaits this one. let result = Box::pin(run_turn_loop( provider, &model, @@ -1517,6 +1522,11 @@ pub(crate) struct TurnInputs { /// Standing decisions about which tool calls may run unattended. approvals: Arc, + /// The command plugins admitted for this turn, sharing `config` with it. + /// + /// A tool on any other plugin is left out of the turn. + command_plugins: CommandPlugins, + /// The request that starts the turn. chat_request: ChatRequest, @@ -1583,9 +1593,24 @@ impl TurnInputs { debug!(count = attachments.len(), "Attachments loaded."); + // Admitted here, before the turn starts, so a prompt asking whether to + // trust a plugin binary never opens inside a tool call. + // + // The turn's config, not the context's: a plugin serving a tool sees + // the options this query resolved, including conversation config and + // `--cfg`, rather than resolving its own from the root. + let command_plugins = admit_turn_plugins(&config, interactive, &printer, PluginInit { + workspace_id: ctx.workspace.id().to_string(), + storage: ctx.storage_path().map(ToOwned::to_owned), + paths: well_known_paths(ctx.user_storage_path()), + config: config.clone(), + log_level: ctx.term.args.verbose, + })?; + Ok(Self { workspace_root: ctx.workspace.root().to_path_buf(), approvals: Arc::new(load_approval_store(ctx.fs_backend.as_deref())), + command_plugins, workspace_id: ctx.workspace.id().clone(), signals: ctx.signals.clone(), mcp_client: ctx.mcp_client.clone(), @@ -1632,8 +1657,10 @@ impl TurnInputs { ); let forced_tool = cfg.assistant.tool_choice.function_name(); + let offered = + without_unadmitted_plugins(cfg.conversation.tools.iter(), &self.command_plugins); let tools = tool_definitions( - cfg.conversation.tools.iter(), + offered.into_iter(), &self.mcp_client, forced_tool, ) @@ -1687,6 +1714,7 @@ impl TurnInputs { &tools, self.printer, self.approvals, + self.command_plugins, self.chat_request, // Built from the lock the turn actually runs against, so it cannot // name one conversation while the events land in another. @@ -2574,9 +2602,20 @@ fn apply_mounts( } let workspace = workspace.ok_or("`--mount` requires a workspace")?; - let root = workspace.root().to_owned(); let cwd = current_dir_utf8()?; + inject_mounts(partial, mounts, workspace.root(), &cwd, merged_config) +} + +/// The pure half of [`apply_mounts`]: add the grants `mounts` name to +/// `partial`, resolving each `NAME` against `cwd` inside `root`. +fn inject_mounts( + partial: &mut PartialAppConfig, + mounts: &[String], + root: &Utf8Path, + cwd: &Utf8Path, + merged_config: Option<&PartialAppConfig>, +) -> BoxedResult<()> { // Resolve the tool set and the global enable default from the merged // config (the fully-layered view) so a bare mount expands over the tools // actually enabled in the resolved config, honoring `*` defaults. @@ -2586,14 +2625,14 @@ fn apply_mounts( let mut plans = Vec::new(); for spec in mounts { let spec = MountSpec::parse(spec)?; - let rule_path = spec.resolve_name(&cwd, &root)?.as_str().to_owned(); + let rule_path = spec.resolve_name(cwd, root)?.as_str().to_owned(); let targets = match &spec.tool { Some(tool) => vec![(tool.clone(), mount_access_seed(tools_config, tool))], None => tools_config .tools .iter() - .filter(|(_, cfg)| is_enabled_local(cfg, &default_enable)) + .filter(|(_, cfg)| is_enabled_subprocess(cfg, &default_enable)) .map(|(name, _)| (name.clone(), mount_access_seed(tools_config, name))) .collect(), }; @@ -3012,13 +3051,14 @@ fn declares_rules(access: &PartialAccessConfig) -> bool { !access.fs.is_empty() || !access.env.is_empty() } -/// Whether a partial tool config is an enabled local tool. +/// Whether a partial tool config is an enabled tool that runs as a subprocess +/// JP starts: a local command or a command plugin. /// /// The tool's own `enable` takes precedence over the global `*` default; a tool /// whose effective `state` resolves to `false` is not part of a bare mount's /// scope. -fn is_enabled_local(cfg: &PartialToolConfig, default_enable: &PartialEnableConfig) -> bool { - matches!(cfg.source, Some(ToolSource::Local { .. })) +fn is_enabled_subprocess(cfg: &PartialToolConfig, default_enable: &PartialEnableConfig) -> bool { + cfg.source.as_ref().is_some_and(ToolSource::is_subprocess) && effective_enable(cfg.enable.as_ref(), default_enable).state } diff --git a/crates/jp_cli/src/cmd/query/tool/coordinator.rs b/crates/jp_cli/src/cmd/query/tool/coordinator.rs index 88479d769..f2cf51f07 100644 --- a/crates/jp_cli/src/cmd/query/tool/coordinator.rs +++ b/crates/jp_cli/src/cmd/query/tool/coordinator.rs @@ -424,7 +424,7 @@ enum Wait { Admission, /// Its approval prompt is open. - Approval(PermissionInfo), + Approval(Box), /// Parked at release, for the batch to be released. Release, @@ -664,18 +664,17 @@ impl ToolCoordinator { /// user needs to see the rendered call to make an informed approval /// decision. /// - /// [`ParametersStyle::Custom`] renders whatever the execution service - /// produced. + /// A style that runs a formatter ([`ParametersStyle::runs_formatter`]: a + /// configured command, or the tool itself) renders whatever the execution + /// service produced. /// A formatter configured with `format = "ask"` has not run yet at this /// point, which is [`PreRender::Deferred`]. fn pre_render_for_prompt(&self, executor: &dyn Executor, renderer: &ToolRenderer) -> PreRender { let name = executor.tool_name(); - if matches!(self.parameter_style(name), ParametersStyle::Custom(_)) - && executor.formatted_arguments().is_none() - { - // Running a user-configured shell command before the user okays - // the tool would be surprising, so `format = "ask"` holds the - // formatter back until admission. Built-in styles are pure and + if self.parameter_style(name).runs_formatter() && executor.formatted_arguments().is_none() { + // Running a program before the user okays the tool would be + // surprising, so `format = "ask"` holds the formatter back until + // admission. Built-in styles are pure and // have no side effects, so they always render before the prompt. return PreRender::Deferred; } @@ -686,20 +685,19 @@ impl ToolCoordinator { /// Render one tool call's arguments for display, returning the content to /// persist for replay. /// - /// A `Custom` parameter style shows what the execution service's formatter - /// produced. - /// The formatter is a user-configured command, so it runs once, there, - /// under the call's access policy and cancellation token, and never a - /// second time here. + /// A parameter style that runs a formatter shows what the execution + /// service's formatter produced. + /// The formatter is a program, so it runs once, there, under the call's + /// access policy and cancellation token, and never a second time here. fn render_executor(&self, executor: &dyn Executor, renderer: &ToolRenderer) -> Option { let name = executor.tool_name(); if self.is_hidden(name) { return None; } - let ParametersStyle::Custom(_) = self.parameter_style(name) else { + if !self.parameter_style(name).runs_formatter() { self.render_approved_tool(name, &executor.arguments(), renderer); return None; - }; + } // The service formats a call's arguments before releasing it, unless // the call is hidden or configured not to run. Both of those are // already handled, so no output here means there is no call to @@ -1615,7 +1613,7 @@ impl ToolCoordinator { }), PreRender::Deferred => None, }; - call.wait = Wait::Approval(info.clone()); + call.wait = Wait::Approval(Box::new(info.clone())); batch.prompting = true; let prompter = Arc::clone(host.prompter); diff --git a/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs b/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs index c4f634867..bf3f0297b 100644 --- a/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs @@ -417,6 +417,31 @@ fn test_pre_render_for_prompt_custom_defers_until_the_service_formats() { assert_eq!(strip_ansi(&stderr.lock()), ""); } +/// A tool that formats its own arguments runs a program to do so, exactly as a +/// configured command does, so it defers the same way. +#[test] +fn test_pre_render_for_prompt_tool_style_defers_until_the_service_formats() { + let coordinator = coordinator_with_style("ticket_create", ParametersStyle::Tool); + + let (printer, _stdout, stderr) = Printer::memory(OutputFormat::TextPretty); + let printer = Arc::new(printer); + let tool_renderer = ToolRenderer::new( + ErrChannel::new(printer.clone()), + jp_config::AppConfig::new_test().style, + ); + + let executor = MockExecutor::completed("call-1", "ticket_create", "done"); + let result = coordinator.pre_render_for_prompt(&executor, &tool_renderer); + + assert!( + matches!(result, PreRender::Deferred), + "an unformatted tool style should defer rendering, got: {result:?}" + ); + + printer.flush(); + assert_eq!(strip_ansi(&stderr.lock()), ""); +} + /// Take `requests` from arrival to their responses through `coordinator`, as /// the turn loop does once the response has finished streaming. /// diff --git a/crates/jp_cli/src/cmd/query/tool/mcp_executor.rs b/crates/jp_cli/src/cmd/query/tool/mcp_executor.rs index 52b67e5de..211765c96 100644 --- a/crates/jp_cli/src/cmd/query/tool/mcp_executor.rs +++ b/crates/jp_cli/src/cmd/query/tool/mcp_executor.rs @@ -28,7 +28,7 @@ use jp_llm::query::ToolExecution; use jp_mcp::{ Client, server::{ - StderrSink, + CommandPlugins, StderrSink, builtin::BuiltinExecutors, fan_out::{self, Split}, http::{Endpoint, EndpointError}, @@ -398,6 +398,7 @@ impl TerminalExecutorSource { Self::start_with_metadata( builtins, runner, + CommandPlugins::default(), definitions, tools, approvals, @@ -414,11 +415,13 @@ impl TerminalExecutorSource { /// `metadata` is advertised on every tool's MCP description, for a caller /// that reads vendor hints there. /// It changes no execution policy. - /// `runner` runs local commands: `local` tools, and every tool's argument - /// formatter. + /// `runner` runs every subprocess: `local` tools, argument formatters, and + /// the binaries of `command_plugins`, the plugins admitted for the tools + /// whose source is `plugin.command.`. pub(crate) async fn start_with_metadata( builtins: BuiltinExecutors, runner: Arc, + command_plugins: CommandPlugins, definitions: &[ToolDefinition], tools: &ToolsConfig, approvals: Arc, @@ -451,6 +454,7 @@ impl TerminalExecutorSource { upstream.clone(), builtins, runner, + command_plugins, root, invocation, )?; @@ -1061,8 +1065,6 @@ impl PendingCall { } enum Received { - /// Boxed because a call holds one of these only while dispatching it, and - /// the largest variant is several times the size of the rest. Interaction(Box), /// The MCP call returned its final result. diff --git a/crates/jp_cli/src/cmd/query/tool/prompter.rs b/crates/jp_cli/src/cmd/query/tool/prompter.rs index f404f8dc0..dfcaf2907 100644 --- a/crates/jp_cli/src/cmd/query/tool/prompter.rs +++ b/crates/jp_cli/src/cmd/query/tool/prompter.rs @@ -577,18 +577,27 @@ fn build_permission_question(tool_name: &str, tool_source: &ToolSource) -> Strin ToolSource::Builtin { .. } => "built-in", ToolSource::Local { .. } => "local", ToolSource::Mcp { .. } => "mcp", + ToolSource::CommandPlugin { .. } => "plugin", }; let mut question = format!("Run {} {} tool", source_type, tool_name.yellow().bold()); - if let ToolSource::Mcp { server, .. } = tool_source { - question = format!( - "{} from {} server?", - question, - server.as_str().blue().bold() - ); - } else { - question.push('?'); + match tool_source { + ToolSource::Mcp { server, .. } => { + question = format!( + "{} from {} server?", + question, + server.as_str().blue().bold() + ); + } + ToolSource::CommandPlugin { plugin, .. } => { + question = format!( + "{} from {} plugin?", + question, + plugin.as_str().blue().bold() + ); + } + ToolSource::Builtin { .. } | ToolSource::Local { .. } => question.push('?'), } question diff --git a/crates/jp_cli/src/cmd/query_tests.rs b/crates/jp_cli/src/cmd/query_tests.rs index f6698a605..eee303241 100644 --- a/crates/jp_cli/src/cmd/query_tests.rs +++ b/crates/jp_cli/src/cmd/query_tests.rs @@ -242,6 +242,66 @@ fn mount_seeds_an_unconfigured_tool_from_the_defaults() { assert_eq!(seed.fs[0].path.as_deref(), Some("src")); } +/// Which tools a bare `--mount` (no `TOOL:` prefix) grants the mounted path to. +/// +/// Every tool that runs as a subprocess JP starts gets it, and nothing else: a +/// local or command plugin tool left out would lose access the user mounted for +/// it, and a builtin or MCP tool given a rule would carry a grant nothing +/// enforces. +#[test] +fn a_bare_mount_reaches_every_enabled_subprocess_tool() { + let mut partial = PartialAppConfig::new_test(); + for (name, source) in [ + ("local_tool", ToolSource::Local { tool: None }), + ("plugin_tool", ToolSource::CommandPlugin { + plugin: "ticket".to_owned(), + tool: None, + }), + ("mcp_tool", ToolSource::Mcp { + server: "server".to_owned(), + tool: None, + }), + ("builtin_tool", ToolSource::Builtin { tool: None }), + ] { + partial + .conversation + .tools + .tools + .insert(name.to_owned(), PartialToolConfig { + source: Some(source), + ..Default::default() + }); + } + + inject_mounts( + &mut partial, + &["notes=/elsewhere/notes".to_owned()], + Utf8Path::new("/ws"), + Utf8Path::new("/ws"), + None, + ) + .unwrap(); + + let mut granted: Vec<&str> = partial + .conversation + .tools + .tools + .iter() + .filter(|(_, cfg)| { + cfg.access.as_ref().is_some_and(|access| { + access + .fs + .iter() + .any(|rule| rule.path.as_deref() == Some("notes")) + }) + }) + .map(|(name, _)| name.as_str()) + .collect(); + granted.sort_unstable(); + + assert_eq!(granted, ["local_tool", "plugin_tool"]); +} + fn make_id(secs: u64) -> ConversationId { ConversationId::try_from(DateTime::::UNIX_EPOCH + std::time::Duration::from_secs(secs)) .unwrap() @@ -348,6 +408,7 @@ async fn an_interrupt_during_mcp_startup_stops_the_turn_before_it_runs() { attachments: vec![], printer: Arc::new(printer), approvals: Arc::new(crate::access::approvals::ApprovalStore::default()), + command_plugins: CommandPlugins::default(), chat_request: ChatRequest::from("hello"), workspace_id: workspace.id().clone(), pending_trim: PendingStreamTrim::default(), @@ -435,6 +496,7 @@ async fn a_client_stop_during_mcp_startup_stops_the_turn_before_it_runs() { attachments: vec![], printer: Arc::new(printer), approvals: Arc::new(crate::access::approvals::ApprovalStore::default()), + command_plugins: CommandPlugins::default(), chat_request: ChatRequest::from("hello"), workspace_id: workspace.id().clone(), pending_trim: PendingStreamTrim::default(), diff --git a/crates/jp_cli/src/render/tool.rs b/crates/jp_cli/src/render/tool.rs index 8ee7d7f71..588883407 100644 --- a/crates/jp_cli/src/render/tool.rs +++ b/crates/jp_cli/src/render/tool.rs @@ -683,9 +683,9 @@ fn format_args(arguments: &Map, style: &ParametersStyle) -> Strin } match style { - // Off and Custom produce no inline output. - // Custom content is rendered separately via render_approved. - ParametersStyle::Off | ParametersStyle::Custom(_) => String::new(), + // Off produces no inline output, and a formatter's content is rendered + // separately via render_approved. + ParametersStyle::Off | ParametersStyle::Custom(_) | ParametersStyle::Tool => String::new(), ParametersStyle::Json => format_args_json(filtered), diff --git a/crates/jp_cli/src/render/turn.rs b/crates/jp_cli/src/render/turn.rs index ed1d88d0c..bcb445932 100644 --- a/crates/jp_cli/src/render/turn.rs +++ b/crates/jp_cli/src/render/turn.rs @@ -11,10 +11,7 @@ use std::{collections::HashMap, sync::Arc}; use chrono::Utc; use jp_config::{ PartialAppConfig, - conversation::tool::{ - ToolConfigWithDefaults, ToolsConfig, - style::{DisplayStyleConfig, ParametersStyle}, - }, + conversation::tool::{ToolConfigWithDefaults, ToolsConfig, style::DisplayStyleConfig}, model::id::PartialModelIdOrAliasConfig, style::{StyleConfig, reasoning::ReasoningDisplayConfig, typewriter::DelayDuration}, }; @@ -224,10 +221,9 @@ impl TurnRenderer { .render_tool_call(&req.name, arguments, &style.parameters); } - // Show stored custom-formatter output when replaying - // a tool call that was originally rendered with a - // Custom parameters style. - if matches!(style.parameters, ParametersStyle::Custom(_)) + // Show stored formatter output when replaying a tool + // call whose parameters style ran a formatter. + if style.parameters.runs_formatter() && let Some(rendered) = get_rendered_arguments(event_with_cfg.event) { self.tool.render_formatted_arguments(&rendered); diff --git a/crates/jp_config/src/conversation/tool.rs b/crates/jp_config/src/conversation/tool.rs index d3b39c3d0..271713e63 100644 --- a/crates/jp_config/src/conversation/tool.rs +++ b/crates/jp_config/src/conversation/tool.rs @@ -226,7 +226,8 @@ impl Validator for ToolsConfig { /// Validate cross-field invariants on the tools configuration. fn validate(&self) -> Result<(), ConfigError> { reject_comma_in_tool_names(self)?; - reject_access_on_non_local_tools(self) + reject_access_on_non_local_tools(self)?; + reject_tool_formatter_on_non_subprocess_tools(self) } } @@ -248,11 +249,11 @@ fn reject_comma_in_tool_names(tools: &ToolsConfig) -> Result<(), ConfigError> { Ok(()) } -/// Reject `access` declared on a tool whose finalized source is `builtin` or -/// `mcp`. +/// Reject `access` declared on a tool that does not run as a subprocess JP +/// starts. /// -/// `access` is the local-subprocess contract: it is serialized into the -/// `Context` that local tool binaries self-check. +/// `access` is the subprocess contract: it is compiled and handed to the local +/// command or command plugin, which self-checks it. /// Builtin tools run in-process and MCP tools run on external servers, so /// neither consumes `access` — accepting it there would create false /// confidence in a security-relevant field. @@ -267,19 +268,63 @@ fn reject_access_on_non_local_tools(tools: &ToolsConfig) -> Result<(), ConfigErr continue; } let kind = match tool.source { - ToolSource::Local { .. } => continue, + ToolSource::Local { .. } | ToolSource::CommandPlugin { .. } => continue, ToolSource::Builtin { .. } => "builtin", ToolSource::Mcp { .. } => "mcp", }; return Err(HandlerError::new(format!( - "conversation.tools.{name}: `access` is only supported on local tools, but '{name}' \ - is a {kind} tool" + "conversation.tools.{name}: `access` is only supported on local and command plugin \ + tools, but '{name}' is a {kind} tool" )) .into()); } Ok(()) } +/// Reject `style.parameters = "tool"` where it would reach a tool that does not +/// run as a subprocess JP starts. +/// +/// The style asks the tool's command or plugin to format its own arguments. +/// A builtin or MCP tool has neither, so the call would be announced with no +/// arguments at all. +/// +/// The `'*'` style reaches every tool that sets no style of its own, so it is +/// rejected only when such a tool is a builtin or MCP one. +fn reject_tool_formatter_on_non_subprocess_tools(tools: &ToolsConfig) -> Result<(), ConfigError> { + let kind = |source: &ToolSource| match source { + ToolSource::Local { .. } | ToolSource::CommandPlugin { .. } => None, + ToolSource::Builtin { .. } => Some("builtin"), + ToolSource::Mcp { .. } => Some("mcp"), + }; + let defaults_format = tools.defaults.style.parameters == style::ParametersStyle::Tool; + + for (name, tool) in tools.tools.iter() { + let Some(kind) = kind(&tool.source) else { + continue; + }; + + match &tool.style { + Some(style) if style.parameters == style::ParametersStyle::Tool => { + return Err(HandlerError::new(format!( + "conversation.tools.{name}: `style.parameters = \"tool\"` is only supported \ + on local and command plugin tools, but '{name}' is a {kind} tool" + )) + .into()); + } + None if defaults_format => { + return Err(HandlerError::new(format!( + "conversation.tools.'*'.style.parameters: `tool` is only supported on local \ + and command plugin tools, but '{name}' is a {kind} tool that inherits it; \ + set it on each tool instead" + )) + .into()); + } + _ => {} + } + } + Ok(()) +} + /// Tools defaults configuration. #[derive(Debug, Clone, PartialEq, Config)] #[config(rename_all = "snake_case")] @@ -304,19 +349,18 @@ pub struct ToolsDefaultsConfig { #[setting(required)] pub run: RunMode, - /// When to run a tool's custom argument formatter relative to the approval - /// prompt. + /// When to run a tool's argument formatter relative to the approval prompt. /// - /// Only affects [`style::ParametersStyle::Custom`] (a user-configured shell - /// command). + /// Only affects `style.parameters` set to a command or to `tool`, both of + /// which run a program to format the call. /// Built-in parameter styles (`json`, `function_call`, `off`) are pure /// transformations and always render before the prompt regardless of this /// field. /// - /// - `ask`: Defer the custom formatter until after approval (safe default - /// — keeps an untrusted shell command from running unprompted). - /// - `allow`: Run the custom formatter ahead of the approval prompt so the - /// user sees the rendered call before deciding. + /// - `ask`: Defer the formatter until after approval (safe default — keeps + /// an untrusted program from running unprompted). + /// - `allow`: Run the formatter ahead of the approval prompt so the user + /// sees the rendered call before deciding. /// /// If unset, derives from `run`: `ask`, `edit` and `skip` map to `ask`; /// `allow` maps to `allow`. @@ -346,8 +390,8 @@ pub struct ToolsDefaultsConfig { #[setting(nested)] pub style: DisplayStyleConfig, - /// Resource access grants for every local tool that declares no `access` of - /// its own. + /// Resource access grants for every local and command plugin tool that + /// declares no `access` of its own. /// /// When absent, those tools keep unrestricted (but workspace-confined) /// access; declaring any rule here switches all of them to default-deny at @@ -478,7 +522,10 @@ pub struct ToolConfig { /// /// - `builtin`: Use a built-in tool. /// - `local`: Use a locally defined tool (shell command). - /// - `mcp`: Use a tool from an MCP server. + /// - `mcp.[.]`: Use a tool from an MCP server. + /// - `plugin.command.[.]`: Use a tool served by the command + /// plugin configured under `plugins.command.`. + /// The plugin receives its `options` as this query resolved them. #[setting(required)] pub source: ToolSource, @@ -553,11 +600,11 @@ pub struct ToolConfig { /// Overrides the global default. pub run: Option, - /// When to run the tool's custom argument formatter relative to the - /// approval prompt. + /// When to run the tool's argument formatter relative to the approval + /// prompt. /// - /// Only affects [`style::ParametersStyle::Custom`]; see - /// [`ToolsDefaultsConfig::format`] for details. + /// Only affects `style.parameters` set to a command or to `tool`; see + /// `conversation.tools.'*'.format` for details. /// /// Overrides the global default. /// If unset, derives from `run`: `ask`, `edit` and `skip` map to `ask`; @@ -1093,6 +1140,22 @@ pub enum ToolSource { /// [`super::ConversationConfig::tools`] map. tool: Option, }, + + /// Use a tool served by a command plugin: `plugin.command.`. + /// + /// The host runs the plugin itself and hands it the plugin's options from + /// the query's resolved configuration, so the tool sees the same + /// `plugins.command..options` the query does. + CommandPlugin { + /// The plugin serving the tool, as named under `plugins.command`. + plugin: String, + + /// The name the plugin knows the tool by. + /// + /// If not specified, it is inferred from the key in the + /// [`super::ConversationConfig::tools`] map. + tool: Option, + }, } impl<'de> Deserialize<'de> for ToolSource { @@ -1126,6 +1189,10 @@ impl Serialize for ToolSource { } s } + Self::CommandPlugin { plugin, tool } => tool.as_ref().map_or_else( + || format!("plugin.command.{plugin}"), + |tool| format!("plugin.command.{plugin}.{tool}"), + ), }; serializer.serialize_str(&s) } @@ -1163,8 +1230,28 @@ impl FromStr for ToolSource { Ok(Self::Mcp { server, tool }) } + "plugin" => { + let rest = tool.unwrap_or_default(); + let (kind, rest) = rest.split_once('.').unwrap_or((rest.as_str(), "")); + match kind { + "" | "command" => {} + kind => return Err(format!("Unknown plugin kind: {kind}, must be: command")), + } + + let (plugin, tool) = match rest.split_once('.') { + Some((plugin, tool)) => (plugin.to_owned(), Some(tool.to_owned())), + None => (rest.to_owned(), None), + }; + if plugin.is_empty() { + return Err("A plugin tool source must name the plugin: use \ + `plugin.command.` or `plugin.command..`." + .to_owned()); + } + + Ok(Self::CommandPlugin { plugin, tool }) + } _ => Err(format!( - "Unknown tool source: {source}, must be one of: builtin, local, mcp" + "Unknown tool source: {source}, must be one of: builtin, local, mcp, plugin" )), } } @@ -1177,13 +1264,24 @@ impl ToolSource { matches!(self, Self::Mcp { .. }) } + /// Return whether the tool runs as a subprocess JP starts: a local command + /// or a command plugin. + /// + /// These are the tools `access` grants apply to, because they receive the + /// compiled policy and enforce it. + #[must_use] + pub const fn is_subprocess(&self) -> bool { + matches!(self, Self::Local { .. } | Self::CommandPlugin { .. }) + } + /// Return the custom name of the tool, if any. #[must_use] pub fn tool_name(&self) -> Option<&str> { match self { - Self::Builtin { tool } | Self::Local { tool } | Self::Mcp { tool, .. } => { - tool.as_deref() - } + Self::Builtin { tool } + | Self::Local { tool } + | Self::Mcp { tool, .. } + | Self::CommandPlugin { tool, .. } => tool.as_deref(), } } } @@ -1194,16 +1292,18 @@ impl schematic::Schematic for ToolSource { } fn build_schema(mut schema: schematic::SchemaBuilder) -> schematic::Schema { - // The three prefixes are the whole vocabulary, and `mcp` is the only - // one that requires a name after it. A bare string would accept + // The four prefixes are the whole vocabulary, and `mcp` and `plugin` + // require a name after them. A bare string would accept // `source = "nonsense"`, which `FromStr` rejects. let mut schema = schema.string(schematic::schema::StringType { - pattern: Some(r"^(builtin|local)(\..+)?$|^mcp\.[^.]+(\..+)?$".to_owned()), + pattern: Some( + r"^(builtin|local)(\..+)?$|^(mcp|plugin\.command)\.[^.]+(\..+)?$".to_owned(), + ), ..schematic::schema::StringType::default() }); schema.set_description( - "Where a tool comes from: `builtin[.]`, `local[.]`, or \ - `mcp.[.]`.", + "Where a tool comes from: `builtin[.]`, `local[.]`, \ + `mcp.[.]`, or `plugin.command.[.]`.", ); schema } @@ -1387,8 +1487,9 @@ impl ToolConfigWithDefaults { /// Return the format mode of the tool. /// - /// Only affects [`style::ParametersStyle::Custom`] (a user-configured shell - /// command). + /// Only affects a parameters style that runs a formatter: a configured + /// command or the tool itself (see + /// [`style::ParametersStyle::runs_formatter`]). /// Built-in parameter styles (`json`, `function_call`, `off`) are pure /// transformations and always render before the approval prompt regardless /// of this value — see [`ToolsDefaultsConfig::format`] for the full @@ -1464,14 +1565,14 @@ impl ToolConfigWithDefaults { /// Return the resource access grants that apply to the tool. /// - /// A tool declaring its own rules uses them whole; a local tool declaring - /// none inherits `conversation.tools.'*'.access`. + /// A tool declaring its own rules uses them whole; a local or command + /// plugin tool declaring none inherits `conversation.tools.'*'.access`. /// The two are never combined, and the block moves as a unit: a tool /// declaring only `fs` rules also drops the `'*'` block's `env` rules. /// - /// Builtin and MCP tools inherit nothing: access grants are the - /// local-subprocess contract, and a `'*'` block says nothing about tools - /// that cannot consume it. + /// Builtin and MCP tools inherit nothing: access grants are the subprocess + /// contract, and a `'*'` block says nothing about tools that cannot consume + /// it. /// Returns `None` when no scope applies, meaning unrestricted (but /// workspace-confined) access. #[must_use] @@ -1480,7 +1581,7 @@ impl ToolConfigWithDefaults { return Some(access); } - if matches!(self.tool.source, ToolSource::Local { .. }) { + if self.tool.source.is_subprocess() { return self.defaults.access.as_ref().filter(|v| !v.is_empty()); } diff --git a/crates/jp_config/src/conversation/tool/access.rs b/crates/jp_config/src/conversation/tool/access.rs index 94337aa9f..65df7a32d 100644 --- a/crates/jp_config/src/conversation/tool/access.rs +++ b/crates/jp_config/src/conversation/tool/access.rs @@ -1,7 +1,7 @@ //! Resource access grants for a tool. //! //! Grants are declared either on one tool or on `conversation.tools.'*'`, which -//! covers every local tool that declares none of its own. +//! covers every local and command plugin tool that declares none of its own. //! A tool declaring any rules of its own ignores the `'*'` block entirely //! rather than adding to it, and the block moves as a unit: a tool declaring //! only `fs` rules also drops the `'*'` block's `env` rules. diff --git a/crates/jp_config/src/conversation/tool/style.rs b/crates/jp_config/src/conversation/tool/style.rs index 1373da62a..5198a5340 100644 --- a/crates/jp_config/src/conversation/tool/style.rs +++ b/crates/jp_config/src/conversation/tool/style.rs @@ -83,6 +83,8 @@ pub struct DisplayStyleConfig { /// - `json`: Show as JSON. /// - `function_call`: Show as a function call (e.g. `tool_name(arg=val)`). /// - `off`: Do not show parameters. + /// - `tool`: Ask the tool itself to format them. + /// Only local and command plugin tools support it. /// - ``: Use a custom command to format the parameters. #[setting(default)] pub parameters: ParametersStyle, @@ -543,6 +545,7 @@ mod strings { named_unit_variant!(json); named_unit_variant!(function_call); named_unit_variant!(off); + named_unit_variant!(tool); } /// How to display the link to the file containing the tool call results. @@ -567,6 +570,16 @@ pub enum ParametersStyle { #[schema(literal = "off")] Off, + /// The tool formats its own arguments. + /// + /// A local tool's command, or a command plugin, is run with the action + /// `format_arguments` instead of `run`, and what it answers is shown + /// verbatim. + /// Only local and command plugin tools support it. + #[serde(with = "strings::tool")] + #[schema(literal = "tool")] + Tool, + /// A custom command to format how the tool call parameters are displayed in /// the terminal. /// @@ -577,6 +590,15 @@ pub enum ParametersStyle { } impl ParametersStyle { + /// Whether a formatter runs to produce what is shown: a configured command, + /// or the tool itself. + /// + /// The other styles are rendered by JP from the arguments alone. + #[must_use] + pub const fn runs_formatter(&self) -> bool { + matches!(self, Self::Custom(_) | Self::Tool) + } + /// Return the custom command if this is a custom style. #[must_use] pub fn into_custom(self) -> Option { @@ -595,6 +617,7 @@ impl FromStr for ParametersStyle { "off" => Self::Off, "json" => Self::Json, "function_call" => Self::FunctionCall, + "tool" => Self::Tool, _ => Self::Custom(CommandConfigOrString::String(s.to_owned())), }) } diff --git a/crates/jp_config/src/conversation/tool/style_tests.rs b/crates/jp_config/src/conversation/tool/style_tests.rs index 5b035d4df..098166593 100644 --- a/crates/jp_config/src/conversation/tool/style_tests.rs +++ b/crates/jp_config/src/conversation/tool/style_tests.rs @@ -12,6 +12,41 @@ fn test_link_style_deserialization() { assert_eq!(from_str::("\"osc8\"").unwrap(), LinkStyle::Osc8); } +/// `tool` asks the tool itself to format its arguments; any other string that +/// is not a keyword is a command. +#[test] +fn parameters_style_reads_tool_as_its_own_keyword() { + assert_eq!( + from_str::("\"tool\"").unwrap(), + ParametersStyle::Tool + ); + assert_eq!( + "tool".parse::().unwrap(), + ParametersStyle::Tool + ); + assert_eq!( + serde_json::to_string(&ParametersStyle::Tool).unwrap(), + "\"tool\"" + ); + assert_eq!( + from_str::("\"tools fmt\"").unwrap(), + ParametersStyle::Custom(CommandConfigOrString::String("tools fmt".to_owned())) + ); +} + +/// A formatter runs for a command and for the tool itself; the built-in styles +/// are rendered by JP. +#[test] +fn parameters_style_runs_a_formatter_for_a_command_or_the_tool() { + assert!(ParametersStyle::Tool.runs_formatter()); + assert!( + ParametersStyle::Custom(CommandConfigOrString::String("fmt".to_owned())).runs_formatter() + ); + assert!(!ParametersStyle::Json.runs_formatter()); + assert!(!ParametersStyle::FunctionCall.runs_formatter()); + assert!(!ParametersStyle::Off.runs_formatter()); +} + #[test] fn test_inline_results_deserialization() { assert_eq!( diff --git a/crates/jp_config/src/conversation/tool_tests.rs b/crates/jp_config/src/conversation/tool_tests.rs index 8f3c0af6a..487cc9950 100644 --- a/crates/jp_config/src/conversation/tool_tests.rs +++ b/crates/jp_config/src/conversation/tool_tests.rs @@ -8,6 +8,94 @@ use crate::{ util::build, }; +/// Build a config from `assignments`, each written as `--cfg` takes it. +fn build_assigned(assignments: &[&str]) -> Result { + use crate::assignment::{AssignKeyValue as _, KvAssignment}; + + let mut partial = PartialAppConfig::new_test(); + for assignment in assignments { + partial + .assign(assignment.parse::().unwrap()) + .unwrap(); + } + build(partial).map_err(|error| error.to_string()) +} + +/// A tool that runs as a subprocess can be asked to format its own arguments. +#[test] +fn parameters_tool_is_accepted_on_local_and_command_tools() { + let config = build_assigned(&[ + "conversation.tools.ticket_create.source=plugin.command.ticket.create", + "conversation.tools.ticket_create.style.parameters=tool", + "conversation.tools.word_count.source=local", + "conversation.tools.word_count.command=wc", + "conversation.tools.word_count.style.parameters=tool", + ]) + .unwrap(); + + for name in ["ticket_create", "word_count"] { + assert_eq!( + config + .conversation + .tools + .get(name) + .unwrap() + .style() + .parameters, + style::ParametersStyle::Tool + ); + } +} + +/// A builtin or MCP tool has nothing JP can ask to format its arguments, and a +/// call announced by an empty formatter would show no arguments at all. +#[test] +fn parameters_tool_is_rejected_on_an_mcp_tool() { + let error = build_assigned(&[ + "conversation.tools.notes.source=mcp.grizzly", + "conversation.tools.notes.style.parameters=tool", + ]) + .unwrap_err(); + + assert!( + error.contains( + "conversation.tools.notes: `style.parameters = \"tool\"` is only supported on local \ + and command plugin tools, but 'notes' is a mcp tool" + ), + "unexpected error: {error}" + ); +} + +/// The `'*'` block reaches every tool without a style of its own, so setting +/// `tool` there is rejected when a builtin or MCP tool would inherit it. +#[test] +fn parameters_tool_in_the_defaults_is_rejected_when_an_mcp_tool_inherits_it() { + let error = build_assigned(&[ + "conversation.tools.*.style.parameters=tool", + "conversation.tools.notes.source=mcp.grizzly", + ]) + .unwrap_err(); + + assert!( + error.contains( + "conversation.tools.'*'.style.parameters: `tool` is only supported on local and \ + command plugin tools, but 'notes' is a mcp tool that inherits it; set it on each \ + tool instead" + ), + "unexpected error: {error}" + ); +} + +#[test] +fn parameters_tool_in_the_defaults_is_accepted_when_only_subprocess_tools_inherit_it() { + let config = build_assigned(&[ + "conversation.tools.*.style.parameters=tool", + "conversation.tools.ticket_create.source=plugin.command.ticket.create", + ]); + + assert!(config.is_ok(), "{config:?}"); +} + /// A tool opting out of the progress window keeps that answer through the whole /// loader, not just when the style is built in memory. /// @@ -191,11 +279,108 @@ fn access_on_mcp_tool_is_rejected_by_validation() { let err = build(partial).unwrap_err().to_string(); assert!( - err.contains("only supported on local tools"), + err.contains( + "conversation.tools.my_mcp: `access` is only supported on local and command plugin \ + tools, but 'my_mcp' is a mcp tool" + ), "unexpected error: {err}" ); } +/// A command plugin tool is a subprocess like a local tool, so it takes the +/// same grants. +#[test] +fn access_on_command_tool_is_accepted_by_validation() { + use crate::{ + PartialAppConfig, + conversation::tool::access::{PartialAccessConfig, PartialFsRuleConfig}, + util::build, + }; + + let mut partial = PartialAppConfig::new_test(); + partial + .conversation + .tools + .tools + .insert("ticket_create".to_owned(), PartialToolConfig { + source: Some(ToolSource::CommandPlugin { + plugin: "ticket".to_owned(), + tool: Some("create".to_owned()), + }), + access: Some(PartialAccessConfig { + fs: vec![PartialFsRuleConfig { + path: Some(".".to_owned()), + read: Some(true), + ..Default::default() + }] + .into(), + ..Default::default() + }), + ..Default::default() + }); + + let config = build(partial).unwrap(); + let access = config + .conversation + .tools + .get("ticket_create") + .unwrap() + .access() + .expect("the tool's own grants") + .clone(); + + assert_eq!(access.fs.len(), 1); + assert_eq!(access.fs[0].path, "."); +} + +/// A `'*'` block restricts every subprocess-backed tool, command plugin tools +/// included: one left out would run unrestricted under a policy the user +/// believes covers it. +#[test] +fn defaults_access_applies_to_a_command_tool_without_its_own() { + use crate::{ + PartialAppConfig, + conversation::tool::access::{PartialAccessConfig, PartialFsRuleConfig}, + util::build, + }; + + let mut partial = PartialAppConfig::new_test(); + partial.conversation.tools.defaults.access = Some(PartialAccessConfig { + fs: vec![PartialFsRuleConfig { + path: Some("src".to_owned()), + read: Some(true), + ..Default::default() + }] + .into(), + ..Default::default() + }); + partial + .conversation + .tools + .tools + .insert("ticket_create".to_owned(), PartialToolConfig { + source: Some(ToolSource::CommandPlugin { + plugin: "ticket".to_owned(), + tool: Some("create".to_owned()), + }), + ..Default::default() + }); + + let config = build(partial).unwrap(); + let access = config + .conversation + .tools + .get("ticket_create") + .unwrap() + .access() + .expect("inherited from the '*' block") + .clone(); + + assert_eq!(access.fs.len(), 1); + assert_eq!(access.fs[0].path, "src"); + assert_eq!(access.fs[0].read, Some(true)); +} + #[test] fn access_on_local_tool_is_accepted_by_validation() { use crate::{ @@ -1440,11 +1625,21 @@ fn the_tool_source_pattern_agrees_with_the_parser() { "mcp.bookworm", "mcp.bookworm.crate_readme", "mcp.bookworm.a.b", - // Rejected by the parser: no server named, an empty server, or a - // prefix that is not a source at all. + "plugin.command.ticket", + "plugin.command.ticket.create", + // Rejected by the parser: no server or plugin named, an empty one, + // a plugin kind JP does not have, or a prefix that is not a source at + // all. "mcp", "mcp.", "mcp..tool", + "plugin", + "plugin.", + "plugin.command", + "plugin.command.", + "plugin.command..create", + "plugin.wasm.ticket", + "command.ticket", "nonsense", "", "builtinx", @@ -1513,6 +1708,75 @@ fn test_tool_source_mcp_roundtrip_without_tool() { assert_eq!(parsed, original); } +#[test] +fn test_tool_source_command_plugin_parses_plugin_only() { + let parsed: ToolSource = "plugin.command.ticket".parse().unwrap(); + assert_eq!(parsed, ToolSource::CommandPlugin { + plugin: "ticket".to_owned(), + tool: None, + }); +} + +#[test] +fn test_tool_source_command_plugin_parses_plugin_and_tool() { + let parsed: ToolSource = "plugin.command.ticket.create".parse().unwrap(); + assert_eq!(parsed, ToolSource::CommandPlugin { + plugin: "ticket".to_owned(), + tool: Some("create".to_owned()), + }); +} + +#[test] +fn test_tool_source_command_plugin_rejects_missing_plugin() { + for input in [ + "plugin", + "plugin.", + "plugin.command", + "plugin.command.", + "plugin.command..create", + ] { + let err = input.parse::().unwrap_err(); + assert_eq!( + err, + "A plugin tool source must name the plugin: use `plugin.command.` or \ + `plugin.command..`.", + "for {input:?}" + ); + } +} + +/// Only command plugins serve tools; another kind is named as unknown rather +/// than read as a plugin called `wasm`. +#[test] +fn test_tool_source_rejects_an_unknown_plugin_kind() { + let err = "plugin.wasm.ticket".parse::().unwrap_err(); + assert_eq!(err, "Unknown plugin kind: wasm, must be: command"); +} + +#[test] +fn test_tool_source_command_plugin_roundtrip() { + for (original, serialized) in [ + ( + ToolSource::CommandPlugin { + plugin: "ticket".to_owned(), + tool: Some("create".to_owned()), + }, + r#""plugin.command.ticket.create""#, + ), + ( + ToolSource::CommandPlugin { + plugin: "ticket".to_owned(), + tool: None, + }, + r#""plugin.command.ticket""#, + ), + ] { + assert_eq!(serde_json::to_string(&original).unwrap(), serialized); + let parsed: ToolSource = serde_json::from_str(serialized).unwrap(); + assert_eq!(parsed, original); + } +} + #[test] fn test_tool_options_assign_flat() { let mut p = PartialToolConfig::default(); diff --git a/crates/jp_config/src/plugins.rs b/crates/jp_config/src/plugins.rs index a7c8b9392..27de7ba98 100644 --- a/crates/jp_config/src/plugins.rs +++ b/crates/jp_config/src/plugins.rs @@ -10,6 +10,7 @@ pub mod command; use schematic::Config; +use serde_json::{Map, Value}; use crate::{ FillDefaults, @@ -40,6 +41,24 @@ pub struct PluginsConfig { pub command: MergeableMap, } +impl PluginsConfig { + /// The `options` the command plugin `name` is configured with, empty when + /// it has none. + #[must_use] + pub fn command_options(&self, name: &str) -> Map { + self.command + .get(name) + .map(|plugin| { + plugin + .options + .iter() + .map(|(key, value)| (key.clone(), value.0.clone())) + .collect() + }) + .unwrap_or_default() + } +} + impl AssignKeyValue for PartialPluginsConfig { fn assign(&mut self, mut kv: KvAssignment) -> AssignResult { match kv.key_string().as_str() { diff --git a/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap b/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap index 161ea695b..b450e3e28 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap @@ -485,6 +485,7 @@ conversation: ConversationConfig |: "json" |: "function_call" |: "off" + |: "tool" |: CommandConfigOrString |: string | (expanded): CommandConfig @@ -857,7 +858,7 @@ conversation: ConversationConfig tool_choice?: "auto" | "none" | "required" | string | null result: "allow" | "ask" | "edit" | "skip" | null run: "ask" | "allow" | "edit" | "skip" | null - source: string(/^(builtin|local)(\..+)?$|^mcp\.[^.]+(\..+)?$/) + source: string(/^(builtin|local)(\..+)?$|^(mcp|plugin\.command)\.[^.]+(\..+)?$/) style: |: DisplayStyleConfig error: ErrorStyleConfig @@ -870,6 +871,7 @@ conversation: ConversationConfig |: "json" |: "function_call" |: "off" + |: "tool" |: CommandConfigOrString |: string | (expanded): CommandConfig @@ -1246,7 +1248,7 @@ conversation: ConversationConfig tool_choice?: "auto" | "none" | "required" | string | null result: "allow" | "ask" | "edit" | "skip" | null run: "ask" | "allow" | "edit" | "skip" | null - source: string(/^(builtin|local)(\..+)?$|^mcp\.[^.]+(\..+)?$/) + source: string(/^(builtin|local)(\..+)?$|^(mcp|plugin\.command)\.[^.]+(\..+)?$/) style: |: DisplayStyleConfig error: ErrorStyleConfig @@ -1259,6 +1261,7 @@ conversation: ConversationConfig |: "json" |: "function_call" |: "off" + |: "tool" |: CommandConfigOrString |: string | (expanded): CommandConfig diff --git a/crates/jp_conversation/src/event.rs b/crates/jp_conversation/src/event.rs index f1fe7e6db..0e0b70603 100644 --- a/crates/jp_conversation/src/event.rs +++ b/crates/jp_conversation/src/event.rs @@ -27,11 +27,12 @@ pub use self::{ pub const CACHE_BREAKPOINT_KEY: &str = "cache_breakpoint"; /// Key used in `ConversationEvent::metadata` to store base64-encoded rendered -/// output from a custom tool argument formatter. +/// output from a tool argument formatter. /// -/// When a tool uses `ParametersStyle::Custom`, the formatter's output is -/// captured at execution time and stored here so that conversation replay can -/// reproduce the display without re-running the formatter. +/// When a tool's parameters style runs a formatter (a configured command, or +/// the tool itself), the formatter's output is captured at execution time and +/// stored here so that conversation replay can reproduce the display without +/// re-running the formatter. pub const RENDERED_ARGUMENTS_KEY: &str = "rendered_arguments"; /// A single event in a conversation. diff --git a/crates/jp_llm/src/provider/anthropic/acp/rpc.rs b/crates/jp_llm/src/provider/anthropic/acp/rpc.rs index db18ba211..6085e409e 100644 --- a/crates/jp_llm/src/provider/anthropic/acp/rpc.rs +++ b/crates/jp_llm/src/provider/anthropic/acp/rpc.rs @@ -124,9 +124,6 @@ pub(super) enum Inbound { } /// Handles everything the agent initiates. -/// -/// Boxed rather than generic because one connection has exactly one handler and -/// it captures the whole translation state. pub(super) type Handler = Box BoxFuture<'static, Result> + Send + Sync>; diff --git a/crates/jp_mcp/Cargo.toml b/crates/jp_mcp/Cargo.toml index 30a5579ca..df42148e1 100644 --- a/crates/jp_mcp/Cargo.toml +++ b/crates/jp_mcp/Cargo.toml @@ -27,6 +27,7 @@ server = [ "dep:base64", "dep:camino", "dep:futures", + "dep:jp_plugin", "dep:jp_process", "dep:jp_tool", "dep:minijinja", @@ -42,6 +43,7 @@ server = [ [dependencies] jp_config = { workspace = true } jp_process = { workspace = true, optional = true } +jp_plugin = { workspace = true, optional = true } jp_tool = { workspace = true, optional = true } async-trait = { workspace = true, optional = true } diff --git a/crates/jp_mcp/src/client_protocol_tests.rs b/crates/jp_mcp/src/client_protocol_tests.rs index 7a31a461a..ce22738bc 100644 --- a/crates/jp_mcp/src/client_protocol_tests.rs +++ b/crates/jp_mcp/src/client_protocol_tests.rs @@ -31,7 +31,7 @@ use super::{Client, McpServerId}; use crate::{ Content, server::{ - Answers, Execution, ExecutionOutcome, + Answers, CommandPlugins, Execution, ExecutionOutcome, builtin::BuiltinExecutors, execute, http::Endpoint, @@ -113,6 +113,7 @@ async fn upstream_receives_context_options_and_accumulated_answers() { conversation_id: "conversation-1".into(), }; let builtins = BuiltinExecutors::new(); + let command_plugins = CommandPlugins::default(); // One context for both attempts, which is what the service does: the second // attempt differs only by the answer it carries. let execution = Execution { @@ -126,6 +127,7 @@ async fn upstream_receives_context_options_and_accumulated_answers() { invocation: &invocation, builtins: &builtins, runner: &no_commands(), + command_plugins: &command_plugins, upstream: &client, cancellation: CancellationToken::new(), stderr: None, @@ -182,6 +184,7 @@ async fn native_service(upstream: &Client) -> (Service, HostReceiver) { upstream.clone(), BuiltinExecutors::new(), no_commands(), + CommandPlugins::default(), "/work".into(), InvocationContext::default(), ) @@ -357,6 +360,7 @@ async fn native_upstream_result_survives_host_projection_and_http_delivery() { upstream, BuiltinExecutors::new(), no_commands(), + CommandPlugins::default(), "/work".into(), InvocationContext::default(), ) diff --git a/crates/jp_mcp/src/server.rs b/crates/jp_mcp/src/server.rs index 58e1731f9..0697d1bad 100644 --- a/crates/jp_mcp/src/server.rs +++ b/crates/jp_mcp/src/server.rs @@ -5,11 +5,12 @@ //! [`http::Endpoint`] exposes that service over loopback Streamable HTTP. //! //! [`tool_definitions`] resolves the configured catalog. -//! [`execute`] runs one attempt of a local command, built-in implementation, or -//! upstream stdio MCP tool; the service handles input-driven re-execution and -//! delivery barriers. +//! [`execute`] runs one attempt of a local command, built-in implementation, +//! command plugin, or upstream stdio MCP tool; the service handles input-driven +//! re-execution and delivery barriers. pub mod builtin; +pub mod command; pub mod fan_out; pub mod http; mod http_client; @@ -21,6 +22,8 @@ use std::{fmt, sync::Arc}; pub use builtin::BuiltinTool; use camino::Utf8Path; +pub use command::{AdmittedPlugin, CommandPlugins, PluginInit}; +use command::{CommandToolCall, init_line, parse_plugin_output, plugin_stderr}; use indexmap::IndexMap; use jp_config::conversation::tool::{ CommandConfig, ToolConfigWithDefaults, ToolSource, style::ParametersStyle, @@ -37,7 +40,7 @@ use minijinja::{Environment, ErrorKind as MinijinjaErrorKind, value::ValueKind}; use result::from_mcp; use serde_json::{Error as JsonError, Map, Value, json}; use tokio_util::sync::CancellationToken; -use tracing::{error, info, trace, warn}; +use tracing::{debug, error, info, trace, warn}; use upstream::{UpstreamResult, decode_result, replace_envelope}; use crate::{ @@ -618,9 +621,10 @@ pub struct Execution<'a> { /// What the tool is run for. /// - /// [`Action::FormatArguments`] runs the tool's argument formatter, a local + /// [`Action::FormatArguments`] runs the tool's argument formatter: the /// command configured in `style.parameters`, whatever the tool's own source - /// is. + /// is, or, for `style.parameters = "tool"`, the tool itself, asked to + /// describe the call instead of making it. pub action: Action, /// Where the tool comes from, and how it is configured to run. @@ -639,9 +643,15 @@ pub struct Execution<'a> { /// Rust implementations, reached by a `builtin` source. pub builtins: &'a builtin::BuiltinExecutors, - /// Runs a local command: a `local` tool, or any tool's argument formatter. + /// Runs a subprocess: a `local` tool, a command plugin's binary, or any + /// tool's argument formatter. pub runner: &'a Arc, + /// The command plugins admitted for this turn, reached by a `command` + /// source. + /// A plugin missing from it does not run. + pub command_plugins: &'a CommandPlugins, + /// Upstream connections, reached by an `mcp` source. pub upstream: &'a Client, @@ -713,15 +723,23 @@ pub async fn execute( "Executing tool." ); + // A formatter command runs as a local command whatever the tool's own + // source is. A tool styled `parameters = "tool"` is its own formatter, so + // it falls through to its source, which sees the action. if execution.action.is_format_arguments() { - let (ToolSource::Local { tool } - | ToolSource::Builtin { tool } - | ToolSource::Mcp { tool, .. }) = execution.config.source(); - let ParametersStyle::Custom(command) = &execution.config.style().parameters else { - return Err(ToolError::MissingCommand); - }; - let command = command.clone().command(); - return execute_local(execution, arguments, answers, tool.as_deref(), command).await; + match &execution.config.style().parameters { + ParametersStyle::Custom(command) => { + let (ToolSource::Local { tool } + | ToolSource::Builtin { tool } + | ToolSource::Mcp { tool, .. } + | ToolSource::CommandPlugin { tool, .. }) = execution.config.source(); + let command = command.clone().command(); + return execute_local(execution, arguments, answers, tool.as_deref(), command) + .await; + } + ParametersStyle::Tool if execution.config.source().is_subprocess() => {} + _ => return Err(ToolError::MissingCommand), + } } match execution.config.source() { @@ -737,6 +755,52 @@ pub async fn execute( ToolSource::Builtin { tool } => { execute_builtin(execution, &arguments, answers, tool.as_deref()).await } + ToolSource::CommandPlugin { plugin, tool } => { + execute_command(execution, arguments, answers, plugin, tool.as_deref()).await + } + } +} + +/// The name the tool's implementation answers to, and `arguments` with +/// configured defaults applied for missing parameters, then validated. +/// +/// The error is the result to hand back to the model, which can correct its +/// arguments and call again. +fn prepare_arguments<'a>( + execution: &'a Execution<'_>, + mut arguments: Value, + tool: Option<&'a str>, +) -> Result<(&'a str, Value), ToolResult> { + let name = execution.invoked_name(tool); + let Some(args) = arguments.as_object_mut() else { + return Ok((name, arguments)); + }; + + apply_parameter_defaults(args, &execution.definition.parameters); + + validate_tool_arguments(args, &execution.definition.parameters).map_err(|error| { + ToolResult::error(format!( + "Invalid arguments: {error}\n\nYou can call `describe_tools(tools: [\"{name}\"])` to \ + learn more about how to use the tool correctly." + )) + })?; + + Ok((name, arguments)) +} + +/// The outcome of one attempt, from what a command printed or a plugin sent. +fn command_outcome(id: String, name: &str, result: CommandResult) -> ExecutionOutcome { + match result { + CommandResult::Success(content) => ExecutionOutcome::Completed { + id, + result: ToolResult::text(content), + }, + CommandResult::NeedsInput(question) => ExecutionOutcome::NeedsInput { id, question }, + CommandResult::Cancelled => ExecutionOutcome::Cancelled { id }, + other => ExecutionOutcome::Completed { + id, + result: other.into_tool_result(name), + }, } } @@ -747,28 +811,16 @@ pub async fn execute( /// Arguments are defaulted and validated first either way. async fn execute_local( execution: &Execution<'_>, - mut arguments: Value, + arguments: Value, answers: &Answers, tool: Option<&str>, command: CommandConfig, ) -> Result { - let name = execution.invoked_name(tool); let id = execution.id.clone(); - - // Apply configured defaults for missing parameters, then validate. - if let Some(args) = arguments.as_object_mut() { - apply_parameter_defaults(args, &execution.definition.parameters); - - if let Err(error) = validate_tool_arguments(args, &execution.definition.parameters) { - return Ok(ExecutionOutcome::Completed { - id, - result: ToolResult::error(format!( - "Invalid arguments: {error}\n\nYou can call `describe_tools(tools: \ - [\"{name}\"])` to learn more about how to use the tool correctly." - )), - }); - } - } + let (name, arguments) = match prepare_arguments(execution, arguments, tool) { + Ok(prepared) => prepared, + Err(result) => return Ok(ExecutionOutcome::Completed { id, result }), + }; let ctx = execution.context(name, &arguments, answers); @@ -788,17 +840,141 @@ async fn execute_local( ) .await?; - match outcome { - CommandResult::Success(content) => Ok(ExecutionOutcome::Completed { - id, - result: ToolResult::text(content), - }), - CommandResult::NeedsInput(question) => Ok(ExecutionOutcome::NeedsInput { id, question }), - CommandResult::Cancelled => Ok(ExecutionOutcome::Cancelled { id }), - other => Ok(ExecutionOutcome::Completed { - id, - result: other.into_tool_result(name), - }), + Ok(command_outcome(id, name, outcome)) +} + +/// Execute a tool served by a command plugin and return the outcome. +/// +/// Arguments are prepared as for a local tool, since the parameters come from +/// configuration in both cases. +/// The plugin runs once, through the runner, with `init` on its stdin; a +/// cancelled call stops it the way it stops a local tool's command. +/// The outcome the plugin sends is read the way a local tool's stdout is, so a +/// malformed question surfaces the same way from either. +async fn execute_command( + execution: &Execution<'_>, + arguments: Value, + answers: &Answers, + plugin: &str, + tool: Option<&str>, +) -> Result { + let id = execution.id.clone(); + let (name, arguments) = match prepare_arguments(execution, arguments, tool) { + Ok(prepared) => prepared, + Err(result) => return Ok(ExecutionOutcome::Completed { id, result }), + }; + + let admitted = execution.command_plugins.verify(plugin).map_err(|reason| { + ToolError::CommandPluginUnavailable { + plugin: plugin.to_owned(), + reason, + } + })?; + + let arguments = match arguments { + Value::Object(arguments) => arguments, + _ => Map::new(), + }; + + let init = execution.command_plugins.init_message(&CommandToolCall { + action: &execution.action, + plugin, + tool: name, + arguments: &arguments, + answers, + options: execution.config.options(), + root: execution.root, + access: execution.access, + invocation: execution.invocation, + })?; + let stdin = init_line(init, plugin)?; + + let Some(stdout) = run_plugin(execution, &admitted.binary, stdin, plugin, name).await? else { + return Ok(ExecutionOutcome::Cancelled { id }); + }; + + Ok(command_outcome(id, name, plugin_result(&stdout, plugin)?)) +} + +/// Run the plugin `binary` once with `stdin`, returning what it printed on +/// stdout, or `None` when the call was cancelled. +async fn run_plugin( + execution: &Execution<'_>, + binary: &Utf8Path, + stdin: String, + plugin: &str, + tool: &str, +) -> Result, ToolError> { + let mut spec = ProcessSpec::new(binary.as_str(), Vec::::new(), execution.root); + spec.stdin = Some(stdin); + // A Ctrl-C at the terminal must not reach the plugin: JP stops it through + // the cancellation token, once the user has chosen what the interrupt + // means. + spec.own_process_group = true; + + // The process runs on a blocking thread, which dropping this future would + // not stop, so the token is cancelled on drop as well as by the caller. + let cancellation = execution.cancellation.child_token(); + let _stop_on_drop = cancellation.clone().drop_guard(); + let watch = Watch { + stderr_lines: Some(plugin_stderr( + plugin.to_owned(), + tool.to_owned(), + execution.stderr.clone(), + )), + cancellation: Some(cancellation), + ..Watch::default() + }; + + debug!(%binary, plugin, tool, "Running a command plugin tool."); + let run = { + let runner = Arc::clone(execution.runner); + let spec = spec.clone(); + tokio::task::spawn_blocking(move || runner.execute(&spec, &watch)) + }; + + let finished = match run.await { + Ok(Ok(finished)) => finished, + Ok(Err(error)) => { + return Err(plugin_failed( + plugin, + format!("failed to start {spec}: {error}"), + )); + } + Err(error) => return Err(plugin_failed(plugin, format!("the run was lost: {error}"))), + }; + + if finished.ended == Ended::Cancelled { + info!(tool = %execution.definition.name, plugin, "Command plugin tool call cancelled"); + return Ok(None); + } + + Ok(Some(finished.output.stdout)) +} + +/// What a plugin's stdout answered, read the way a local tool's stdout is. +/// +/// A plugin that sent `tool_outcome` said it was answering in the outcome +/// shape, so anything else is a protocol fault, not text for the model. +/// A payload that claims to be a question is left to [`parse_command_output`], +/// which names what is wrong with it. +fn plugin_result(stdout: &str, plugin: &str) -> Result { + let payload = parse_plugin_output(stdout).map_err(|message| plugin_failed(plugin, message))?; + let raw = payload.to_string(); + + if let Err(error) = serde_json::from_value::(payload) + && !Outcome::claims_needs_input(&raw) + { + return Err(ToolError::MalformedOutput(error)); + } + + Ok(parse_command_output(raw.as_bytes(), b"", true)) +} + +fn plugin_failed(plugin: &str, message: String) -> ToolError { + ToolError::CommandPluginFailed { + plugin: plugin.to_owned(), + message, } } @@ -901,6 +1077,19 @@ async fn execute_builtin( }) } +/// Whether a tool is offered to the assistant: enabled, or named by +/// `forced_tool` and not locked off. +/// +/// The rule [`tool_definitions`] applies before resolving anything, exposed so +/// a host preparing a turn can tell which tools it has to make runnable. +#[must_use] +pub fn is_offered(name: &str, config: &ToolConfigWithDefaults, forced_tool: Option<&str>) -> bool { + let enable = config.effective_enable(); + let forced = forced_tool.is_some_and(|f| f == name); + + enable.is_enabled() || (forced && !enable.is_locked()) +} + /// Resolve all enabled tool definitions from config. /// /// If `forced_tool` is provided (e.g. from `ToolChoice::Function`), that tool @@ -918,10 +1107,8 @@ pub async fn tool_definitions( let mut definitions = Vec::new(); for (name, config) in configs { - let enable = config.effective_enable(); let forced = forced_tool.is_some_and(|f| f == name); - // Drop disabled tools, but keep a forced tool unless it is locked-off. - if !enable.is_enabled() && (!forced || enable.is_locked()) { + if !is_offered(name, &config, forced_tool) { continue; } @@ -970,7 +1157,9 @@ async fn resolve_tool( ) -> Result { let path = format!("conversation.tools.{name}.parameters"); let definition = match config.source() { - ToolSource::Local { .. } | ToolSource::Builtin { .. } => ToolDefinition { + ToolSource::Local { .. } + | ToolSource::Builtin { .. } + | ToolSource::CommandPlugin { .. } => ToolDefinition { name: name.to_owned(), docs: tool_docs_from_config(config), parameters: json_schema::from_config(&path, config.parameters())?, diff --git a/crates/jp_mcp/src/server/command.rs b/crates/jp_mcp/src/server/command.rs new file mode 100644 index 000000000..86b5ad119 --- /dev/null +++ b/crates/jp_mcp/src/server/command.rs @@ -0,0 +1,354 @@ +//! Tools served by command plugins. +//! +//! A tool whose source is `plugin.command.` is answered by a plugin +//! binary, run once per attempt the way a local tool's command is: its stdin +//! carries one `init` message and is then closed, and its stdout carries back +//! one `tool_outcome` and an `exit`. +//! +//! Which binaries may run is decided by the host before the turn starts, and +//! handed in as [`CommandPlugins`]: each admitted plugin's binary and the hash +//! of the contents that were admitted, alongside the turn's configuration. +//! A call's `init` carries that configuration and the plugin's options read +//! from it, so the plugin sees what the turn resolved. +//! Nothing here decides trust; a plugin absent from [`CommandPlugins`] does not +//! run. +//! +//! See: `docs/rfd/072-command-plugin-system.md`, "Tool Calls". + +use std::{collections::HashMap, fs, io, sync::Arc}; + +use camino::{Utf8Path, Utf8PathBuf}; +use indexmap::IndexMap; +use jp_config::{AppConfig, types::json_value::JsonValue}; +use jp_plugin::{ + PROTOCOL_VERSION, + message::{ + HostToPlugin, InitMessage, LogMessage, OutputFormat, PathsInfo, PluginToHost, ToolAction, + ToolCall, WorkspaceInfo, + }, +}; +use jp_process::LineSink; +use jp_tool::{AccessPolicy, Action, Error as ToolError, InvocationContext}; +use serde_json::{Map, Value}; +use sha2::{Digest as _, Sha256}; +use tracing::{debug, error, info, trace, warn}; + +use super::{Answers, StderrSink}; + +/// The `init` fields every plugin a turn runs is told, whichever tool it +/// answers. +#[derive(Debug, Clone)] +pub struct PluginInit { + /// The workspace's globally unique ID. + pub workspace_id: String, + + /// The `.jp` storage directory. + /// + /// `None` when filesystem storage is not configured, which leaves a plugin + /// with nowhere it is told to store anything; its calls then fail. + pub storage: Option, + + /// Well-known JP directories. + pub paths: PathsInfo, + + /// The turn's resolved configuration. + /// + /// Serialized into `init.config` for each call, and the source of the + /// plugin's `init.options`. + pub config: Arc, + + /// The host's log verbosity, so plugin stderr matches `-v`. + pub log_level: u8, +} + +/// A plugin binary the host admitted for a turn. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AdmittedPlugin { + /// The binary. + pub binary: Utf8PathBuf, + + /// The SHA-256 of the contents admission decided on, from [`sha256_file`]. + pub sha256: String, +} + +/// The command plugins a turn may run, and what each call's `init` carries. +/// +/// The default admits nothing, for a turn whose tools run through no plugin. +#[derive(Debug, Clone, Default)] +pub struct CommandPlugins { + /// `None` only for the default, which no call reaches + /// [`Self::init_message`] through: with no plugin admitted, + /// [`Self::verify`] refuses first. + init: Option, + admitted: HashMap, +} + +impl CommandPlugins { + /// No plugin admitted yet, with `init` shared by every call. + #[must_use] + pub fn new(init: PluginInit) -> Self { + Self { + init: Some(init), + admitted: HashMap::new(), + } + } + + /// Whether `plugin` was admitted for this turn. + #[must_use] + pub fn is_admitted(&self, plugin: &str) -> bool { + self.admitted.contains_key(plugin) + } + + /// Let the plugin `name` run this turn. + #[must_use] + pub fn with(mut self, name: impl Into, plugin: AdmittedPlugin) -> Self { + self.admitted.insert(name.into(), plugin); + self + } + + /// The admitted binary for `plugin`, provided it still holds the contents + /// that were admitted. + /// + /// # Errors + /// + /// The reason it may not run, as a sentence for + /// [`ToolError::CommandPluginUnavailable`]: it was not admitted, or the + /// binary changed since. + pub fn verify(&self, plugin: &str) -> Result<&AdmittedPlugin, String> { + let admitted = self + .admitted + .get(plugin) + .ok_or("it was not admitted for this turn")?; + + let sha256 = sha256_file(&admitted.binary) + .map_err(|error| format!("cannot read {}: {error}", admitted.binary))?; + if sha256 != admitted.sha256 { + return Err(format!( + "{} changed since it was admitted at the start of this turn", + admitted.binary + )); + } + + Ok(admitted) + } + + /// The `init` that starts one attempt of `call`. + pub(super) fn init_message( + &self, + call: &CommandToolCall<'_>, + ) -> Result { + let unavailable = |reason: &str| ToolError::CommandPluginUnavailable { + plugin: call.plugin.to_owned(), + reason: reason.to_owned(), + }; + let init = self + .init + .as_ref() + .ok_or_else(|| unavailable("no plugin was admitted for this turn"))?; + let storage = init + .storage + .clone() + .ok_or_else(|| unavailable("the workspace has no storage configured"))?; + let config = serde_json::to_value(init.config.to_partial()).map_err(|error| { + ToolError::CommandPluginFailed { + plugin: call.plugin.to_owned(), + message: format!("failed to serialize the configuration: {error}"), + } + })?; + + Ok(InitMessage { + version: PROTOCOL_VERSION, + workspace: WorkspaceInfo { + root: call.root.to_owned(), + storage, + id: init.workspace_id.clone(), + }, + paths: init.paths.clone(), + config, + options: init.config.plugins.command_options(call.plugin), + args: vec![], + log_level: init.log_level, + // A tool's output goes to the model, which reads plain text. + output_format: OutputFormat::Text, + tool: Some(tool_call(call)?), + }) + } +} + +/// The SHA-256 of a file's contents, as lowercase hex. +/// +/// # Errors +/// +/// When the file cannot be read. +pub fn sha256_file(path: &Utf8Path) -> io::Result { + Ok(format!("{:x}", Sha256::digest(fs::read(path)?))) +} + +/// One attempt of a tool call routed to a command plugin. +pub(super) struct CommandToolCall<'a> { + /// Whether the plugin runs the tool or describes the call. + pub action: &'a Action, + + /// The plugin serving the tool, as named under `plugins.command`. + pub plugin: &'a str, + + /// The name the plugin knows the tool by. + pub tool: &'a str, + + /// Arguments after schema coercion, defaults, and validation. + pub arguments: &'a Map, + + /// Answers to questions earlier attempts of this call asked. + pub answers: &'a Answers, + + /// The tool's own `conversation.tools..options`. + pub options: &'a IndexMap, + + /// The workspace root the call runs against. + pub root: &'a Utf8Path, + + /// The tool's compiled access grants, which the plugin must enforce. + /// + /// `None` means the tool declares no policy: unrestricted but + /// workspace-confined filesystem access, as for a local tool. + pub access: Option<&'a AccessPolicy>, + + /// Workspace and conversation the call belongs to. + pub invocation: &'a InvocationContext, +} + +/// The call as the protocol carries it. +/// +/// Fails rather than sending a call without its access policy: a plugin that +/// received none would read it as unrestricted. +fn tool_call(call: &CommandToolCall<'_>) -> Result { + let access = call + .access + .map(serde_json::to_value) + .transpose() + .map_err(|error| ToolError::CommandPluginFailed { + plugin: call.plugin.to_owned(), + message: format!("failed to serialize the access policy: {error}"), + })?; + + Ok(ToolCall { + action: match call.action { + Action::Run => ToolAction::Run, + Action::FormatArguments => ToolAction::FormatArguments, + }, + name: call.tool.to_owned(), + arguments: call.arguments.clone(), + answers: call + .answers + .iter() + .map(|(id, answer)| (id.clone(), answer.clone())) + .collect(), + options: call + .options + .iter() + .map(|(key, JsonValue(value))| (key.clone(), value.clone())) + .collect(), + access, + conversation: call.invocation.conversation_id.clone(), + }) +} + +/// The `init` line written to the plugin's stdin. +pub(super) fn init_line(init: InitMessage, plugin: &str) -> Result { + let mut line = serde_json::to_string(&HostToPlugin::Init(Box::new(init))).map_err(|error| { + ToolError::CommandPluginFailed { + plugin: plugin.to_owned(), + message: format!("failed to serialize the init message: {error}"), + } + })?; + line.push('\n'); + Ok(line) +} + +/// Read what a plugin printed on stdout, returning the outcome it sent. +/// +/// The call is finished only by an `exit` after the outcome: a plugin that +/// answers and then crashes before `exit` fails the call. +/// The error is a sentence about the plugin, for +/// [`ToolError::CommandPluginFailed`]. +pub(super) fn parse_plugin_output(stdout: &str) -> Result { + let mut outcome = None; + let mut exited = false; + + for line in stdout + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()) + { + let message: PluginToHost = serde_json::from_str(line).map_err(|error| { + format!("sent a line that is not a protocol message: {error}: {line}") + })?; + trace!(?message, "Plugin message during a tool call."); + + match message { + PluginToHost::Ready(ready) if ready.protocol > PROTOCOL_VERSION => { + return Err(format!( + "it needs `jp` protocol {}, and this `jp` speaks {PROTOCOL_VERSION}", + ready.protocol + )); + } + PluginToHost::Log(log) => emit_log(&log), + PluginToHost::ToolOutcome(message) => outcome = Some(message.outcome), + PluginToHost::Exit(exit) if exit.code != 0 => { + return Err(exit + .reason + .unwrap_or_else(|| format!("exited with code {}", exit.code))); + } + PluginToHost::Exit(_) => { + exited = true; + break; + } + + // A tool's result is its outcome, and its input closed after + // `init`: printed output and requests have nowhere to go. + other => debug!(?other, "Ignoring a plugin message during a tool call."), + } + } + + match outcome { + Some(outcome) if exited => Ok(outcome), + Some(_) => Err("answered the tool call, then exited without sending `exit`".to_owned()), + None => Err(format!( + "exited without answering the tool call; it may predate `jp` protocol \ + {PROTOCOL_VERSION}" + )), + } +} + +/// Forward each line of a plugin's stderr to tracing, and to `sink` when the +/// turn shows it. +/// +/// Blank lines carry nothing to show, so neither sees them. +pub(super) fn plugin_stderr(plugin: String, tool: String, sink: Option) -> LineSink { + Arc::new(move |line: &str| { + if line.is_empty() { + return; + } + trace!(target: "plugin::stderr", plugin = %plugin, tool = %tool, "{line}"); + if let Some(sink) = &sink { + sink(line); + } + }) +} + +/// Forward a plugin's log message to tracing at the level it named. +fn emit_log(log: &LogMessage) { + let message = &log.message; + match log.level.as_str() { + "trace" => trace!(target: "plugin", message = %message), + "debug" => debug!(target: "plugin", message = %message), + "info" => info!(target: "plugin", message = %message), + "warn" => warn!(target: "plugin", message = %message), + "error" => error!(target: "plugin", message = %message), + level => warn!(target: "plugin", level, message = %message, "unknown log level"), + } +} + +#[cfg(test)] +#[path = "command_tests.rs"] +mod tests; diff --git a/crates/jp_mcp/src/server/command_tests.rs b/crates/jp_mcp/src/server/command_tests.rs new file mode 100644 index 000000000..f8276b505 --- /dev/null +++ b/crates/jp_mcp/src/server/command_tests.rs @@ -0,0 +1,164 @@ +use camino_tempfile::tempdir; +use serde_json::json; + +use super::*; + +/// What a plugin printing `lines`, one protocol message each, printed. +fn printed(lines: &[Value]) -> String { + lines.iter().map(|line| line.to_string() + "\n").collect() +} + +fn outcome(content: &str) -> Value { + json!({"type": "tool_outcome", "outcome": {"type": "success", "content": content}}) +} + +fn exit(code: u8) -> Value { + json!({"type": "exit", "code": code}) +} + +#[test] +fn the_outcome_is_read_back() { + let stdout = printed(&[ + json!({"type": "ready", "protocol": 2}), + outcome("Created T-0abc123"), + exit(0), + ]); + + assert_eq!( + parse_plugin_output(&stdout), + Ok(json!({"type": "success", "content": "Created T-0abc123"})) + ); +} + +/// Stdin carries only `init`, so a request, a print, or a log the plugin sends +/// gets no answer and does not stop the outcome arriving. +#[test] +fn other_messages_are_ignored() { + let stdout = printed(&[ + json!({"type": "ready", "protocol": 2}), + json!({"type": "list_conversations", "id": "3"}), + json!({"type": "print", "text": "working"}), + json!({"type": "log", "level": "warn", "message": "slow disk"}), + outcome("ok"), + exit(0), + ]); + + assert_eq!( + parse_plugin_output(&stdout), + Ok(json!({"type": "success", "content": "ok"})) + ); +} + +#[test] +fn a_failing_exit_reports_its_reason() { + let stdout = printed(&[json!({"type": "exit", "code": 1, "reason": "No ticket T-0abc123."})]); + + assert_eq!( + parse_plugin_output(&stdout), + Err("No ticket T-0abc123.".to_owned()) + ); +} + +#[test] +fn a_failing_exit_without_a_reason_reports_its_code() { + assert_eq!( + parse_plugin_output(&printed(&[exit(3)])), + Err("exited with code 3".to_owned()) + ); +} + +/// What a plugin built before protocol 12 does: it ignores `tool`, finds no +/// arguments to run, and exits without an outcome. +#[test] +fn an_exit_without_an_outcome_is_a_failure() { + assert_eq!( + parse_plugin_output(&printed(&[exit(0)])), + Err("exited without answering the tool call; it may predate `jp` protocol 12".to_owned()) + ); +} + +/// A plugin that answers and then dies before `exit`, a panic during teardown +/// or a failed write, did not finish the call. +/// Its outcome is not reported as the call's result. +#[test] +fn an_outcome_without_an_exit_is_a_failure() { + let stdout = printed(&[json!({"type": "ready", "protocol": 12}), outcome("ok")]); + + assert_eq!( + parse_plugin_output(&stdout), + Err("answered the tool call, then exited without sending `exit`".to_owned()) + ); +} + +#[test] +fn no_output_at_all_is_a_failure() { + assert_eq!( + parse_plugin_output(""), + Err("exited without answering the tool call; it may predate `jp` protocol 12".to_owned()) + ); +} + +#[test] +fn a_plugin_needing_a_newer_protocol_is_refused() { + let stdout = printed(&[ + json!({"type": "ready", "protocol": PROTOCOL_VERSION + 1}), + outcome("ok"), + ]); + + assert_eq!( + parse_plugin_output(&stdout), + Err(format!( + "it needs `jp` protocol {}, and this `jp` speaks {PROTOCOL_VERSION}", + PROTOCOL_VERSION + 1 + )) + ); +} + +#[test] +fn a_line_that_is_not_a_message_is_a_failure() { + let error = parse_plugin_output("not json\n").unwrap_err(); + + assert!( + error.starts_with("sent a line that is not a protocol message: "), + "got: {error}" + ); +} + +#[test] +fn verify_accepts_the_admitted_contents_and_refuses_changed_ones() { + let dir = tempdir().unwrap(); + let binary = dir.path().join("jp-ticket"); + fs::write(&binary, "v1").unwrap(); + let plugins = CommandPlugins::default().with("ticket", AdmittedPlugin { + sha256: sha256_file(&binary).unwrap(), + binary: binary.clone(), + }); + + assert_eq!(plugins.verify("ticket").map(|p| &p.binary), Ok(&binary)); + + fs::write(&binary, "v2").unwrap(); + assert_eq!( + plugins.verify("ticket"), + Err(format!( + "{binary} changed since it was admitted at the start of this turn" + )) + ); + assert_eq!( + plugins.verify("metrics"), + Err("it was not admitted for this turn".to_owned()) + ); +} + +/// The digest the registry and the approval store use, so a hash recorded by +/// either reads the same here. +#[test] +fn sha256_is_lowercase_hex() { + let dir = tempdir().unwrap(); + let file = dir.path().join("f"); + fs::write(&file, "abc").unwrap(); + + assert_eq!( + sha256_file(&file).unwrap(), + "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad" + ); +} diff --git a/crates/jp_mcp/src/server/conformance_tests.rs b/crates/jp_mcp/src/server/conformance_tests.rs index 9fdb61423..852b30b4d 100644 --- a/crates/jp_mcp/src/server/conformance_tests.rs +++ b/crates/jp_mcp/src/server/conformance_tests.rs @@ -29,7 +29,7 @@ use super::Endpoint; use crate::{ Client, Content, server::{ - InvocationContext, + CommandPlugins, InvocationContext, builtin::{BuiltinExecutors, BuiltinTool}, service::{ Admission, ConfiguredTool, HostError, HostReceiver, HostRequest, InputAnswer, @@ -326,6 +326,7 @@ async fn fixture_reporting_every( upstream, builtins, runner, + CommandPlugins::default(), root.path().to_owned(), InvocationContext { workspace_id: "workspace-1".into(), diff --git a/crates/jp_mcp/src/server/http_tests.rs b/crates/jp_mcp/src/server/http_tests.rs index 3aeee6881..5b3cf3461 100644 --- a/crates/jp_mcp/src/server/http_tests.rs +++ b/crates/jp_mcp/src/server/http_tests.rs @@ -19,7 +19,7 @@ use super::*; use crate::{ Client, Content, server::{ - InvocationContext, + CommandPlugins, InvocationContext, builtin::{BuiltinExecutors, BuiltinTool}, service::{ Admission, CallRequest, ConfiguredTool, HostReceiver, Interaction, ReleaseDecision, @@ -61,6 +61,7 @@ fn setup() -> (Service, HostReceiver, Arc) { Client::default(), BuiltinExecutors::new().register("count", Count(count.clone())), no_commands(), + CommandPlugins::default(), "/tmp".into(), InvocationContext::default(), ) diff --git a/crates/jp_mcp/src/server/service.rs b/crates/jp_mcp/src/server/service.rs index dc2ca8f66..8ea78667a 100644 --- a/crates/jp_mcp/src/server/service.rs +++ b/crates/jp_mcp/src/server/service.rs @@ -38,7 +38,8 @@ use tokio::sync::{Notify, broadcast, mpsc, oneshot}; use tokio_util::sync::CancellationToken; use super::{ - Answers, CommandResult, Execution, ExecutionOutcome, InvocationContext, StderrSink, + Answers, CommandPlugins, CommandResult, Execution, ExecutionOutcome, InvocationContext, + StderrSink, builtin::BuiltinExecutors, execute, fan_out::{self, OperationOutcome, Split}, @@ -256,10 +257,6 @@ pub enum Interaction { /// Acknowledge final recording before returning the result to the caller. Record { /// What the Host is being asked to record. - /// - /// Boxed because it is the largest thing the private channel carries, - /// and every other interaction in flight would otherwise be sized for - /// it. recording: Box, /// Acknowledges the Host's configured persistence policy, not an /// unconditional disk write. @@ -451,6 +448,7 @@ struct Inner { upstream: Client, builtins: BuiltinExecutors, runner: Arc, + command_plugins: CommandPlugins, root: Utf8PathBuf, invocation: InvocationContext, host: mpsc::Sender, @@ -617,14 +615,17 @@ impl Service { /// service. /// /// No tool code is run. - /// `runner` runs local commands: `local` tools, and every tool's argument - /// formatter. + /// `runner` runs every subprocess: `local` tools, command plugin binaries, + /// and argument formatters. + /// `command_plugins` names the plugins admitted for this turn; a call to + /// any other plugin fails as unavailable. /// The returned receiver is the private Host interface. pub fn new( tools: Vec, upstream: Client, builtins: BuiltinExecutors, runner: Arc, + command_plugins: CommandPlugins, root: Utf8PathBuf, invocation: InvocationContext, ) -> Result<(Self, HostReceiver), ServiceError> { @@ -655,6 +656,7 @@ impl Service { upstream, builtins, runner, + command_plugins, root, invocation, host, @@ -927,15 +929,15 @@ async fn run_call( validate_arguments(&tool.definition, &mut arguments)?; // A skipped or hidden call shows nothing, so its formatter is a command // that would run for output nobody reads. - let formats = matches!(tool.config.style().parameters, ParametersStyle::Custom(_)) + let formats = has_formatter(&tool) && tool.config.run() != RunMode::Skip && !tool.config.style().hidden && ask(inner, call, |reply| Interaction::RenderArguments { reply }).await?; // What the formatter's questions were answered with. The tool runs with the // same answers, so the call that executes is the one that was described. let mut answers = Answers::new(); - // `format = "ask"` holds a user-configured command back until the Host has - // admitted the call. + // `format = "ask"` holds a formatter back until the Host has admitted the + // call: it is a program, which should not run unprompted. let mut formatted_arguments = None; if formats && tool.config.format() == FormatMode::Allow { match describe(inner, call, &tool, &arguments, &mut answers, cancellation).await? { @@ -1037,6 +1039,19 @@ async fn run_call( deliver_result(inner, call, &tool, arguments, output, executed).await } +/// Whether `tool` has an argument formatter to run: a configured command, or, +/// for `style.parameters = "tool"`, the tool itself. +/// +/// A tool only describes itself when JP runs it as a subprocess, which is where +/// the action reaches it; the configuration refuses `tool` anywhere else. +fn has_formatter(tool: &ConfiguredTool) -> bool { + match tool.config.style().parameters { + ParametersStyle::Custom(_) => true, + ParametersStyle::Tool => tool.config.source().is_subprocess(), + _ => false, + } +} + /// Have the tool's argument formatter describe the call, asking the Host for /// each answer it needs first. /// @@ -1484,6 +1499,7 @@ async fn attempt( invocation: &inner.invocation, builtins: &inner.builtins, runner: &inner.runner, + command_plugins: &inner.command_plugins, upstream: &inner.upstream, cancellation: cancellation.clone(), stderr: Some(stderr), diff --git a/crates/jp_mcp/src/server/service_tests.rs b/crates/jp_mcp/src/server/service_tests.rs index 03909ff6c..4ebc3bc8f 100644 --- a/crates/jp_mcp/src/server/service_tests.rs +++ b/crates/jp_mcp/src/server/service_tests.rs @@ -1,4 +1,5 @@ use std::{ + fs, future::pending, io, sync::{ @@ -10,10 +11,12 @@ use std::{ use assert_matches::assert_matches; use async_trait::async_trait; use camino::Utf8Path; +use camino_tempfile::tempdir; use jp_config::{ AppConfig, Config as _, conversation::tool::{PartialToolConfig, ToolConfig}, }; +use jp_plugin::message::PathsInfo; use jp_process::{ExitCode, MockProcessRunner, ProcessOutput}; use jp_tool::{Outcome, Question, ToolDefinition, ToolDocs}; use serde_json::{Value, json}; @@ -24,7 +27,9 @@ use tokio::{ use super::*; use crate::server::{ + AdmittedPlugin, PluginInit, builtin::BuiltinTool, + command::sha256_file, testing::{echoing, no_commands, printed}, }; @@ -82,6 +87,7 @@ fn service( Client::default(), builtins, runner, + CommandPlugins::default(), root.to_owned(), invocation, ) @@ -107,6 +113,190 @@ fn fixture(run: &str, result: &str) -> (Service, HostReceiver, Arc) (service, host, count) } +/// What every plugin call of these tests' turns is told. +fn plugin_init() -> PluginInit { + PluginInit { + workspace_id: "ws-abc".to_owned(), + storage: Some("/tmp/.jp".into()), + paths: PathsInfo::default(), + config: Arc::new(AppConfig::new_test()), + log_level: 0, + } +} + +/// A service whose `count` tool is served by the command plugin `counter`, +/// configured by `config` on top of its source. +/// +/// The plugin answers by the action its `init` names: `described` when asked to +/// format its arguments, `ran` when run, so a test can tell which of the two +/// produced what it sees. +/// Every run is recorded on the returned runner, which is the only evidence +/// that the plugin ran: a denied call and one that silently went nowhere look +/// the same from the result alone. +fn plugin_service(config: Value) -> (Service, HostReceiver, Arc) { + let dir = tempdir().unwrap(); + let binary = dir.path().join("jp-counter"); + fs::write(&binary, "plugin").unwrap(); + let plugins = CommandPlugins::new(plugin_init()).with("counter", AdmittedPlugin { + sha256: sha256_file(&binary).unwrap(), + binary, + }); + let runner = Arc::new(MockProcessRunner::responding(move |spec| { + // Owns the directory, so the binary lives as long as the service. + let _dir = &dir; + let init: Value = serde_json::from_str(spec.stdin.as_deref().unwrap_or("{}")).unwrap(); + let content = match init["tool"]["action"].as_str() { + Some("format_arguments") => "described", + _ => "ran", + }; + Ok(ProcessOutput { + stdout: format!( + "{}\n{}\n", + json!({"type": "tool_outcome", "outcome": {"type": "success", "content": content}}), + json!({"type": "exit", "code": 0}), + ), + stderr: String::new(), + status: ExitCode::success(), + }) + })); + + let mut partial = json!({"source": "plugin.command.counter"}); + let Value::Object(config) = config else { + panic!("tool config is an object"); + }; + partial.as_object_mut().unwrap().extend(config); + let partial: PartialToolConfig = serde_json::from_value(partial).unwrap(); + let mut app = AppConfig::new_test(); + app.conversation.tools.insert( + "count".into(), + ToolConfig::from_partial(partial, vec![]).unwrap(), + ); + let tool = ConfiguredTool { + definition: ToolDefinition { + name: "count".into(), + docs: ToolDocs::default(), + parameters: json!({ + "type": "object", + "properties": {"path": {"type": "string"}}, + "required": ["path"], + }), + }, + config: app.conversation.tools.get("count").unwrap(), + access: Ok(None), + metadata: Map::new(), + }; + let (service, host) = Service::new( + vec![tool], + Client::default(), + BuiltinExecutors::new(), + runner.clone(), + plugins, + "/tmp".into(), + InvocationContext::default(), + ) + .unwrap(); + (service, host, runner) +} + +/// The action each run of the plugin was started for, in order. +fn actions(runner: &MockProcessRunner) -> Vec { + runner + .calls() + .iter() + .map(|spec| { + let init: Value = serde_json::from_str(spec.stdin.as_deref().unwrap()).unwrap(); + init["tool"]["action"].as_str().unwrap().to_owned() + }) + .collect() +} + +/// A plugin tool styled `parameters = "tool"` is asked to describe the call, +/// and what it answers is what the Host shows before approval. +#[tokio::test] +async fn a_plugin_describes_its_own_call_before_approval() { + let (service, mut host, runner) = plugin_service(json!({ + "run": "ask", "result": "allow", "format": "allow", "style": {"parameters": "tool"} + })); + let call = service.start_call(request()).unwrap(); + + let Interaction::RenderArguments { reply } = next(&mut host).await.interaction else { + panic!("expected visibility request") + }; + reply.send(Ok(true)).unwrap(); + let Interaction::Prepare { + arguments, + formatted_arguments, + reply, + .. + } = next(&mut host).await.interaction + else { + panic!("expected preparation") + }; + assert_eq!(formatted_arguments.as_deref(), Some("described")); + assert_eq!(actions(&runner), ["format_arguments"]); + + reply.send(Ok(Admission::Run { arguments })).unwrap(); + let Interaction::Release { reply, .. } = next(&mut host).await.interaction else { + panic!("expected release") + }; + reply.send(Ok(ReleaseDecision::Execute)).unwrap(); + let Interaction::Record { reply, .. } = next(&mut host).await.interaction else { + panic!("expected recording") + }; + reply.send(Ok(())).unwrap(); + + assert_eq!(call.finish().await.unwrap(), ToolResult::text("ran")); + assert_eq!(actions(&runner), ["format_arguments", "run"]); +} + +/// `format = "ask"` holds the plugin back until the call is admitted, as it +/// does a formatter command: nothing runs before the user says yes. +#[tokio::test] +async fn a_plugin_formatter_waits_for_approval_under_format_ask() { + let (service, mut host, runner) = plugin_service(json!({ + "run": "ask", "result": "allow", "format": "ask", "style": {"parameters": "tool"} + })); + let call = service.start_call(request()).unwrap(); + + let Interaction::RenderArguments { reply } = next(&mut host).await.interaction else { + panic!("expected visibility request") + }; + reply.send(Ok(true)).unwrap(); + let Interaction::Prepare { + arguments, + formatted_arguments, + reply, + .. + } = next(&mut host).await.interaction + else { + panic!("expected preparation") + }; + assert!(formatted_arguments.is_none()); + assert_eq!(actions(&runner), Vec::::new()); + + reply.send(Ok(Admission::Run { arguments })).unwrap(); + let Interaction::Release { + formatted_arguments, + reply, + .. + } = next(&mut host).await.interaction + else { + panic!("expected release") + }; + assert_eq!(formatted_arguments.as_deref(), Some("described")); + assert_eq!(actions(&runner), ["format_arguments"]); + reply + .send(Ok(ReleaseDecision::Complete { + result: ToolResult::text("stopped"), + })) + .unwrap(); + let Interaction::Record { reply, .. } = next(&mut host).await.interaction else { + panic!("expected recording") + }; + reply.send(Ok(())).unwrap(); + call.finish().await.unwrap(); +} + async fn release(host: &mut HostReceiver) { let Interaction::Prepare { arguments, reply, .. @@ -670,6 +860,105 @@ async fn denied_call_never_executes() { assert_eq!(count.load(Ordering::SeqCst), 0); } +#[tokio::test] +async fn command_tool_waits_for_admission_before_the_plugin_runs() { + let (service, mut host, runner) = plugin_service(json!({"run": "ask", "result": "unattended"})); + let call = service.start_call(request()).unwrap(); + + let Interaction::Prepare { + arguments, reply, .. + } = next(&mut host).await.interaction + else { + panic!("expected preparation") + }; + assert_eq!(runner.calls().len(), 0, "ran before it was admitted"); + reply.send(Ok(Admission::Run { arguments })).unwrap(); + + let Interaction::Release { reply, .. } = next(&mut host).await.interaction else { + panic!("expected release") + }; + assert_eq!(runner.calls().len(), 0, "ran before it was released"); + reply.send(Ok(ReleaseDecision::Execute)).unwrap(); + + let Interaction::Record { reply, .. } = next(&mut host).await.interaction else { + panic!("expected recording") + }; + reply.send(Ok(())).unwrap(); + assert_eq!(call.finish().await.unwrap(), ToolResult::text("ran")); + assert_eq!(runner.calls().len(), 1); +} + +#[tokio::test] +async fn denied_command_tool_never_reaches_the_plugin() { + let (service, mut host, runner) = plugin_service(json!({"run": "ask", "result": "unattended"})); + let call = service.start_call(request()).unwrap(); + let Interaction::Prepare { reply, .. } = next(&mut host).await.interaction else { + panic!("expected preparation") + }; + reply + .send(Ok(Admission::Skip { + reason: "denied".into(), + })) + .unwrap(); + let Interaction::Record { reply, .. } = next(&mut host).await.interaction else { + panic!("expected recording") + }; + reply.send(Ok(())).unwrap(); + assert_eq!(call.finish().await.unwrap(), ToolResult::text("denied")); + assert_eq!(runner.calls().len(), 0); +} + +/// `run = "skip"` resolves the call without asking the Host and without running +/// anything. +#[tokio::test] +async fn skipped_command_tool_never_reaches_the_plugin() { + let (service, mut host, runner) = + plugin_service(json!({"run": "skip", "result": "unattended"})); + let call = service.start_call(request()).unwrap(); + let Interaction::Record { recording, reply } = next(&mut host).await.interaction else { + panic!("expected recording, not a preparation") + }; + assert_eq!( + recording.result, + ToolResult::text("Tool execution skipped by configuration.") + ); + reply.send(Ok(())).unwrap(); + call.finish().await.unwrap(); + assert_eq!(runner.calls().len(), 0); +} + +#[tokio::test] +async fn invalid_edited_arguments_never_reach_the_plugin() { + let (service, mut host, runner) = + plugin_service(json!({"run": "edit", "result": "unattended"})); + let call = service.start_call(request()).unwrap(); + let Interaction::Prepare { reply, .. } = next(&mut host).await.interaction else { + panic!("expected preparation") + }; + reply + .send(Ok(Admission::Run { + arguments: Map::new(), + })) + .unwrap(); + assert!(matches!( + call.finish().await, + Err(ServiceError::Tool(ToolError::Arguments { .. })) + )); + assert_eq!(runner.calls().len(), 0); +} + +#[tokio::test] +async fn command_tool_host_loss_fails_closed() { + let (service, host, runner) = plugin_service(json!({"run": "ask", "result": "unattended"})); + drop(host); + let call = service.start_call(request()).unwrap(); + assert!(matches!( + call.finish().await, + Err(ServiceError::HostDisconnected) + )); + assert_eq!(runner.calls().len(), 0); +} + #[tokio::test] async fn invalid_edited_arguments_do_not_reach_execution() { let (service, mut host, count) = fixture("edit", "allow"); @@ -934,6 +1223,7 @@ async fn local_inquiry_exits_and_runs_a_new_process_with_the_answer() { Client::default(), BuiltinExecutors::new(), runner.clone(), + CommandPlugins::default(), "/tmp".into(), InvocationContext::default(), ) @@ -1106,6 +1396,139 @@ async fn dropping_result_receiver_does_not_cancel_or_reexecute() { service.shutdown().await; } +/// A service whose `count` tool is a local command styled `parameters = +/// "tool"`, so the same command both formats its arguments and runs. +/// +/// The command is passed the action it was started for, and prints `described` +/// when formatting and `ran` when run, so a test can see both which runs +/// happened and which produced what it sees. +fn local_tool_formatter_fixture(format: &str) -> (Service, HostReceiver, Arc) { + let runner = Arc::new(MockProcessRunner::responding(|spec| { + let content = match spec.args.first().map(String::as_str) { + Some("format_arguments") => "described", + _ => "ran", + }; + Ok(ProcessOutput { + stdout: content.to_owned(), + stderr: String::new(), + status: ExitCode::success(), + }) + })); + let (service, host) = service( + json!({ + "source": "local", + "run": "ask", + "result": "allow", + "format": format, + "style": {"parameters": "tool"}, + "command": {"program": "count", "args": ["{{context.action}}"], "shell": false}, + }), + "/tmp".into(), + BuiltinExecutors::new(), + runner.clone(), + InvocationContext::default(), + ); + (service, host, runner) +} + +/// The actions the local command was started for, in order. +fn local_actions(runner: &MockProcessRunner) -> Vec { + runner + .calls() + .iter() + .map(|spec| spec.args.join(" ")) + .collect() +} + +/// A local tool styled `parameters = "tool"` runs its own command to describe +/// the call, and what it prints is what the Host shows before approval. +#[tokio::test] +async fn a_local_tool_describes_its_own_call_before_approval() { + let (service, mut host, runner) = local_tool_formatter_fixture("allow"); + let call = service.start_call(request()).unwrap(); + + let Interaction::RenderArguments { reply } = next(&mut host).await.interaction else { + panic!("expected visibility request") + }; + reply.send(Ok(true)).unwrap(); + let Interaction::Prepare { + arguments, + formatted_arguments, + reply, + .. + } = next(&mut host).await.interaction + else { + panic!("expected preparation") + }; + assert_eq!(formatted_arguments.as_deref(), Some("described")); + assert_eq!(local_actions(&runner), ["format_arguments"]); + + reply.send(Ok(Admission::Run { arguments })).unwrap(); + let Interaction::Release { reply, .. } = next(&mut host).await.interaction else { + panic!("expected release") + }; + reply.send(Ok(ReleaseDecision::Execute)).unwrap(); + let Interaction::Record { reply, .. } = next(&mut host).await.interaction else { + panic!("expected recording") + }; + reply.send(Ok(())).unwrap(); + + assert_eq!(call.finish().await.unwrap(), ToolResult::text("ran")); + assert_eq!(local_actions(&runner), ["format_arguments", "run"]); +} + +/// `format = "ask"` holds the local command back until the call is admitted: +/// nothing runs before the user says yes, and a call the Host then resolves +/// itself never runs for real. +#[tokio::test] +async fn a_local_tool_formatter_waits_for_approval_under_format_ask() { + let (service, mut host, runner) = local_tool_formatter_fixture("ask"); + let call = service.start_call(request()).unwrap(); + + let Interaction::RenderArguments { reply } = next(&mut host).await.interaction else { + panic!("expected visibility request") + }; + reply.send(Ok(true)).unwrap(); + let Interaction::Prepare { + arguments, + formatted_arguments, + reply, + .. + } = next(&mut host).await.interaction + else { + panic!("expected preparation") + }; + assert!(formatted_arguments.is_none()); + assert_eq!( + local_actions(&runner), + Vec::::new(), + "nothing ran before admission" + ); + + reply.send(Ok(Admission::Run { arguments })).unwrap(); + let Interaction::Release { + formatted_arguments, + reply, + .. + } = next(&mut host).await.interaction + else { + panic!("expected release") + }; + assert_eq!(formatted_arguments.as_deref(), Some("described")); + reply + .send(Ok(ReleaseDecision::Complete { + result: ToolResult::text("stopped"), + })) + .unwrap(); + let Interaction::Record { reply, .. } = next(&mut host).await.interaction else { + panic!("expected recording") + }; + reply.send(Ok(())).unwrap(); + call.finish().await.unwrap(); + + assert_eq!(local_actions(&runner), ["format_arguments"]); +} + /// A service whose `count` tool formats its arguments with a `formatter` /// command, and the runner that plays it. /// diff --git a/crates/jp_mcp/src/server_tests.rs b/crates/jp_mcp/src/server_tests.rs index 61aa9096e..865d43660 100644 --- a/crates/jp_mcp/src/server_tests.rs +++ b/crates/jp_mcp/src/server_tests.rs @@ -1,16 +1,26 @@ +use std::{fs, sync::Arc}; + use async_trait::async_trait; use camino::Utf8PathBuf; +use camino_tempfile::{Utf8TempDir, tempdir}; use jp_config::{ - AppConfig, Config as _, + AppConfig, Config as _, PartialAppConfig, + assignment::{AssignKeyValue as _, KvAssignment}, conversation::tool::{CommandConfig, PartialToolConfig, ToolConfig, ToolConfigWithDefaults}, + util::build, }; -use jp_tool::{AnswerType, Outcome, ToolDefinition, ToolDocs}; +use jp_plugin::{PROTOCOL_VERSION, message::PathsInfo}; +use jp_process::{ExitCode, MockProcessRunner, ProcessOutput}; +use jp_tool::{AnswerType, EnvRule, Outcome, ToolDefinition, ToolDocs}; use serde_json::Map; use super::*; use crate::{ Client, - server::testing::{echoing, no_commands}, + server::{ + command::sha256_file, + testing::{echoing, no_commands}, + }, }; struct EchoArguments; @@ -31,6 +41,19 @@ struct Fixture { config: ToolConfigWithDefaults, builtins: builtin::BuiltinExecutors, runner: Arc, + + /// The plugins admitted for the turn, rebuilt by [`Fixture::refresh`] from + /// the three fields after it. + command_plugins: CommandPlugins, + + /// The turn's configuration, which gains each plugin's options. + turn: PartialAppConfig, + admitted: Vec<(String, AdmittedPlugin)>, + storage: Option, + + /// Holds the plugin binaries [`Fixture::with_plugin`] writes. + dir: Utf8TempDir, + access: Option, upstream: Client, root: Utf8PathBuf, invocation: InvocationContext, @@ -54,6 +77,12 @@ impl Fixture { config: app.conversation.tools.get(name).unwrap(), builtins: builtin::BuiltinExecutors::new(), runner: no_commands(), + command_plugins: CommandPlugins::default(), + turn: PartialAppConfig::new_test(), + admitted: vec![], + storage: Some("/tmp/.jp".into()), + dir: tempdir().unwrap(), + access: None, upstream: Client::new(IndexMap::new()), root: "/tmp".into(), invocation: InvocationContext::default(), @@ -65,6 +94,62 @@ impl Fixture { self } + /// Put a `jp-{name}` binary on disk and admit it, with `options` set as its + /// `plugins.command..options` in the turn's configuration. + fn with_plugin(mut self, name: &str, options: &Value) -> Self { + let binary = self.binary(name); + fs::write(&binary, "v1").unwrap(); + self.turn + .assign( + format!("plugins.command.{name}.options:={options}") + .parse::() + .unwrap(), + ) + .unwrap(); + self.admitted.push((name.to_owned(), AdmittedPlugin { + sha256: sha256_file(&binary).unwrap(), + binary, + })); + self.refresh() + } + + fn without_storage(mut self) -> Self { + self.storage = None; + self.refresh() + } + + /// The configuration the turn's plugin calls are made under. + fn turn_config(&self) -> Arc { + Arc::new(build(self.turn.clone()).unwrap()) + } + + fn refresh(mut self) -> Self { + let init = PluginInit { + workspace_id: "ws-abc".to_owned(), + storage: self.storage.clone(), + paths: PathsInfo::default(), + config: self.turn_config(), + log_level: 0, + }; + self.command_plugins = self + .admitted + .iter() + .cloned() + .fold(CommandPlugins::new(init), |plugins, (name, admitted)| { + plugins.with(name, admitted) + }); + self + } + + fn binary(&self, name: &str) -> Utf8PathBuf { + self.dir.path().join(format!("jp-{name}")) + } + + fn with_access(mut self, access: AccessPolicy) -> Self { + self.access = Some(access); + self + } + fn with_invocation(mut self, invocation: InvocationContext) -> Self { self.invocation = invocation; self @@ -83,10 +168,11 @@ impl Fixture { action: Action::Run, config: &self.config, root: &self.root, - access: None, + access: self.access.as_ref(), invocation: &self.invocation, builtins: &self.builtins, runner: &self.runner, + command_plugins: &self.command_plugins, upstream: &self.upstream, cancellation: CancellationToken::new(), stderr: None, @@ -707,6 +793,509 @@ async fn tool_with_an_unresolvable_schema_is_skipped() { assert_eq!(names, vec!["healthy_tool"]); } +/// A plugin that prints `lines`, each serialized as one protocol line, and +/// exits successfully. +fn plugin_printing(lines: &[Value]) -> Arc { + let stdout: String = lines.iter().map(|line| line.to_string() + "\n").collect(); + Arc::new(MockProcessRunner::responding(move |_| { + Ok(ProcessOutput { + stdout: stdout.clone(), + stderr: String::new(), + status: ExitCode::success(), + }) + })) +} + +/// A plugin that answers with `outcome` and exits. +fn plugin_answering(outcome: Value) -> Arc { + plugin_printing(&[ + json!({"type": "ready", "protocol": 10}), + Value::Object(Map::from_iter([ + ("type".to_owned(), json!("tool_outcome")), + ("outcome".to_owned(), outcome), + ])), + json!({"type": "exit", "code": 0}), + ]) +} + +/// The `init` the plugin was started with, read back from its stdin. +fn init_sent(runner: &MockProcessRunner) -> Value { + let calls = runner.calls(); + assert_eq!(calls.len(), 1, "the plugin ran once: {calls:?}"); + let stdin = calls[0].stdin.as_deref().expect("init on stdin"); + assert!(stdin.ends_with('\n'), "one line: {stdin:?}"); + serde_json::from_str(stdin).unwrap() +} + +#[tokio::test] +async fn command_tool_runs_its_plugin_with_the_call_on_stdin() { + let runner = plugin_answering(json!({"type": "success", "content": "Created T-0abc123"})); + let fixture = Fixture::new( + "ticket_create", + json!({"source": "plugin.command.ticket.create", "options": {"mode": "strict"}}), + schema([ + ("title", param("string"), true), + ("kind", json!({"type": "string", "default": "bug"}), false), + ]), + ) + .with_plugin("ticket", &json!({"dir": "packages/foo/tickets"})) + .with_runner(runner.clone()) + .with_invocation(InvocationContext { + workspace_id: "ws-abc".to_owned(), + conversation_id: "jp-c17000000000".to_owned(), + }); + + let answers = Answers::from([("confirm".to_owned(), json!(true))]); + let outcome = execute( + &fixture.execution("call-1", json!({"title": "Fix it"})), + &answers, + ) + .await + .unwrap(); + + let ExecutionOutcome::Completed { id, result } = outcome else { + panic!("expected a completed call, got {outcome:?}"); + }; + assert_eq!(id, "call-1"); + assert_eq!(result, ToolResult::text("Created T-0abc123")); + + let spec = &runner.calls()[0]; + assert_eq!(spec.program, fixture.binary("ticket").as_str()); + assert_eq!(spec.args, Vec::::new()); + assert_eq!(spec.dir, Utf8PathBuf::from("/tmp")); + assert!(spec.own_process_group, "a Ctrl-C must not reach the plugin"); + + let mut init = init_sent(&runner); + // The whole configuration the turn resolved, as `jp ` sends it: + // compared against that configuration rather than written out, since it + // runs to every default. + assert_eq!( + init.as_object_mut().unwrap().remove("config"), + Some(serde_json::to_value(fixture.turn_config().to_partial()).unwrap()) + ); + assert_eq!( + init, + json!({ + "type": "init", + "version": PROTOCOL_VERSION, + "workspace": {"root": "/tmp", "storage": "/tmp/.jp", "id": "ws-abc"}, + "paths": {}, + // Read from the turn's `plugins.command.ticket.options`. + "options": {"dir": "packages/foo/tickets"}, + "args": [], + "log_level": 0, + "output_format": "text", + "tool": { + "action": "run", + "name": "create", + // The configured default is applied before the plugin sees the + // call, as it is for a local tool. + "arguments": {"title": "Fix it", "kind": "bug"}, + "answers": {"confirm": true}, + "options": {"mode": "strict"}, + "conversation": "jp-c17000000000" + } + }) + ); +} + +/// A tool that formats its own arguments is asked to, through the same `init`, +/// with the action saying it is a description and not a run. +#[tokio::test] +async fn command_tool_formatting_its_arguments_says_so_in_init() { + let runner = plugin_answering(json!({"type": "success", "content": "File bug: Fix it"})); + let fixture = Fixture::new( + "ticket_create", + json!({"source": "plugin.command.ticket.create", "style": {"parameters": "tool"}}), + schema([("title", param("string"), true)]), + ) + .with_plugin("ticket", &json!({"dir": "packages/foo/tickets"})) + .with_runner(runner.clone()); + + let mut execution = fixture.execution("call-1", json!({"title": "Fix it"})); + execution.action = Action::FormatArguments; + let outcome = execute(&execution, &Answers::new()).await.unwrap(); + + let ExecutionOutcome::Completed { result, .. } = outcome else { + panic!("expected a completed call, got {outcome:?}"); + }; + assert_eq!(result, ToolResult::text("File bug: Fix it")); + let init = init_sent(&runner); + assert_eq!(init["tool"]["action"], "format_arguments"); + assert_eq!(init["options"], json!({"dir": "packages/foo/tickets"})); +} + +#[tokio::test] +async fn command_tool_run_says_so_in_init() { + let runner = plugin_answering(json!({"type": "success", "content": "ok"})); + let fixture = Fixture::new( + "ticket_create", + json!({"source": "plugin.command.ticket.create"}), + schema([]), + ) + .with_plugin("ticket", &json!({})) + .with_runner(runner.clone()); + + execute(&fixture.execution("call-1", json!({})), &Answers::new()) + .await + .unwrap(); + + assert_eq!(init_sent(&runner)["tool"]["action"], "run"); +} + +/// A local tool styled `parameters = "tool"` formats its own arguments by +/// running its own command, with the action in `context.action`. +#[tokio::test] +async fn local_tool_formatting_its_arguments_runs_its_command_for_that_action() { + let fixture = Fixture::new( + "word_count", + json!({ + "source": "local", + "command": {"program": "word_count", "args": ["{{context.action}}"], "shell": false}, + "style": {"parameters": "tool"}, + }), + schema([]), + ) + .with_runner(echoing()); + + let mut execution = fixture.execution("call-1", json!({})); + execution.action = Action::FormatArguments; + let outcome = execute(&execution, &Answers::new()).await.unwrap(); + + let ExecutionOutcome::Completed { result, .. } = outcome else { + panic!("expected a completed call, got {outcome:?}"); + }; + assert_eq!(result, ToolResult::text("format_arguments\n")); +} + +#[tokio::test] +async fn command_tool_without_a_tool_name_uses_the_configured_key() { + let runner = plugin_answering(json!({"type": "success", "content": "ok"})); + let fixture = Fixture::new( + "labels", + json!({"source": "plugin.command.ticket"}), + schema([]), + ) + .with_plugin("ticket", &json!({})) + .with_runner(runner.clone()); + + execute(&fixture.execution("call-1", json!({})), &Answers::new()) + .await + .unwrap(); + + assert_eq!(init_sent(&runner)["tool"]["name"], "labels"); +} + +/// The compiled policy reaches the plugin, which enforces it, as it reaches a +/// local tool's command through `context.access`. +#[tokio::test] +async fn command_tool_receives_the_compiled_access_policy() { + let runner = plugin_answering(json!({"type": "success", "content": "ok"})); + let fixture = Fixture::new( + "ticket_create", + json!({"source": "plugin.command.ticket.create"}), + schema([]), + ) + .with_plugin("ticket", &json!({})) + .with_runner(runner.clone()) + .with_access(AccessPolicy { + env: vec![EnvRule { + name: "AWS_*".to_owned(), + read: false, + }], + ..AccessPolicy::default() + }); + + execute(&fixture.execution("call-1", json!({})), &Answers::new()) + .await + .unwrap(); + + assert_eq!( + init_sent(&runner)["tool"]["access"], + json!({"fs": [], "net": [], "env": [{"name": "AWS_*", "read": false}]}) + ); +} + +#[tokio::test] +async fn command_tool_with_invalid_arguments_never_reaches_the_plugin() { + let runner = plugin_answering(json!({"type": "success", "content": "ok"})); + let fixture = Fixture::new( + "ticket_create", + json!({"source": "plugin.command.ticket.create"}), + schema([("title", param("string"), true)]), + ) + .with_plugin("ticket", &json!({})) + .with_runner(runner.clone()); + + let outcome = execute(&fixture.execution("call-1", json!({})), &Answers::new()) + .await + .unwrap(); + + let ExecutionOutcome::Completed { result, .. } = outcome else { + panic!("expected a completed call, got {outcome:?}"); + }; + assert!(result.is_error()); + assert!( + result.to_text().starts_with("Invalid arguments: "), + "got: {}", + result.to_text() + ); + assert_eq!(runner.calls(), vec![]); +} + +#[tokio::test] +async fn command_tool_question_ends_the_attempt() { + let fixture = Fixture::new( + "ticket_create", + json!({"source": "plugin.command.ticket.create"}), + schema([]), + ) + .with_plugin("ticket", &json!({})) + .with_runner(plugin_answering(json!({ + "type": "needs_input", + "question": {"id": "confirm", "text": "File it?", "answer_type": {"type": "boolean"}} + }))); + + let outcome = execute(&fixture.execution("call-1", json!({})), &Answers::new()) + .await + .unwrap(); + + let ExecutionOutcome::NeedsInput { id, question } = outcome else { + panic!("expected a question, got {outcome:?}"); + }; + assert_eq!(id, "call-1"); + assert_eq!(question.id.as_str(), "confirm"); +} + +#[tokio::test] +async fn command_tool_error_outcome_keeps_its_details() { + let fixture = Fixture::new( + "ticket_create", + json!({"source": "plugin.command.ticket.create"}), + schema([]), + ) + .with_plugin("ticket", &json!({})) + .with_runner(plugin_answering(json!({ + "type": "error", "message": "busy", "trace": ["lock held"], "transient": true + }))); + + let outcome = execute(&fixture.execution("call-1", json!({})), &Answers::new()) + .await + .unwrap(); + + let ExecutionOutcome::Completed { result, .. } = outcome else { + panic!("expected a completed call, got {outcome:?}"); + }; + assert_eq!( + result.status, + ToolStatus::Error(ErrorDetails { + transient: true, + trace: vec!["lock held".into()], + }) + ); +} + +/// A plugin that sent `tool_outcome` said it was answering in the outcome +/// shape, so anything else is a protocol fault, not text for the model. +#[tokio::test] +async fn command_tool_outcome_in_the_wrong_shape_is_malformed_output() { + let fixture = Fixture::new( + "ticket_create", + json!({"source": "plugin.command.ticket.create"}), + schema([]), + ) + .with_plugin("ticket", &json!({})) + .with_runner(plugin_answering(json!({"content": "no type tag"}))); + + let error = execute(&fixture.execution("call-1", json!({})), &Answers::new()) + .await + .unwrap_err(); + + assert!( + matches!(error, ToolError::MalformedOutput(_)), + "got: {error:?}" + ); +} + +#[tokio::test] +async fn command_tool_failing_exit_reports_the_plugin_reason() { + let fixture = Fixture::new( + "ticket_create", + json!({"source": "plugin.command.ticket.create"}), + schema([]), + ) + .with_plugin("ticket", &json!({})) + .with_runner(plugin_printing(&[ + json!({"type": "ready", "protocol": 10}), + json!({"type": "exit", "code": 1, "reason": "No ticket T-0abc123."}), + ])); + + let error = execute(&fixture.execution("call-1", json!({})), &Answers::new()) + .await + .unwrap_err(); + + assert_eq!( + error.to_string(), + "Command plugin `ticket` failed: No ticket T-0abc123." + ); +} + +/// A plugin that sends a successful outcome and then crashes has not finished +/// the call; the model is told it failed, not that it succeeded. +#[tokio::test] +async fn command_tool_that_crashes_after_answering_fails() { + let runner = Arc::new(MockProcessRunner::responding(|_| { + Ok(ProcessOutput { + stdout: format!( + "{}\n", + json!({"type": "tool_outcome", "outcome": {"type": "success", "content": "ok"}}) + ), + stderr: "thread 'main' panicked".to_owned(), + status: ExitCode::from(Some(101)), + }) + })); + let fixture = Fixture::new( + "ticket_create", + json!({"source": "plugin.command.ticket.create"}), + schema([]), + ) + .with_plugin("ticket", &json!({})) + .with_runner(runner); + + let error = execute(&fixture.execution("call-1", json!({})), &Answers::new()) + .await + .unwrap_err(); + + assert_eq!( + error.to_string(), + "Command plugin `ticket` failed: answered the tool call, then exited without sending \ + `exit`" + ); +} + +#[tokio::test] +async fn command_tool_whose_binary_cannot_start_fails() { + let fixture = Fixture::new( + "ticket_create", + json!({"source": "plugin.command.ticket.create"}), + schema([]), + ) + .with_plugin("ticket", &json!({})); + let binary = fixture.binary("ticket"); + let fixture = fixture.with_runner(Arc::new( + MockProcessRunner::builder().expect_any().fails_to_spawn(), + )); + + let error = execute(&fixture.execution("call-1", json!({})), &Answers::new()) + .await + .unwrap_err(); + + assert_eq!( + error.to_string(), + format!("Command plugin `ticket` failed: failed to start {binary}: entity not found") + ); +} + +/// Only plugins the host admitted for the turn run; any other is refused before +/// anything is spawned. +#[tokio::test] +async fn command_tool_of_a_plugin_not_admitted_never_runs() { + let runner = plugin_answering(json!({"type": "success", "content": "ok"})); + let fixture = Fixture::new( + "ticket_create", + json!({"source": "plugin.command.ticket.create"}), + schema([]), + ) + .with_runner(runner.clone()); + + let error = execute(&fixture.execution("call-1", json!({})), &Answers::new()) + .await + .unwrap_err(); + + assert_eq!( + error.to_string(), + "Command plugin `ticket` cannot be run from here: it was not admitted for this turn" + ); + assert_eq!(runner.calls(), vec![]); +} + +/// Admission decided on a binary's contents. +/// One replaced during the turn is refused at its next call rather than run on +/// the strength of the old one. +#[tokio::test] +async fn command_tool_whose_binary_changed_after_admission_never_runs() { + let runner = plugin_answering(json!({"type": "success", "content": "ok"})); + let fixture = Fixture::new( + "ticket_create", + json!({"source": "plugin.command.ticket.create"}), + schema([]), + ) + .with_plugin("ticket", &json!({})) + .with_runner(runner.clone()); + fs::write(fixture.binary("ticket"), "v2").unwrap(); + + let error = execute(&fixture.execution("call-1", json!({})), &Answers::new()) + .await + .unwrap_err(); + + assert_eq!( + error.to_string(), + format!( + "Command plugin `ticket` cannot be run from here: {} changed since it was admitted at \ + the start of this turn", + fixture.binary("ticket") + ) + ); + assert_eq!(runner.calls(), vec![]); +} + +#[tokio::test] +async fn command_tool_without_workspace_storage_never_runs() { + let runner = plugin_answering(json!({"type": "success", "content": "ok"})); + let fixture = Fixture::new( + "ticket_create", + json!({"source": "plugin.command.ticket.create"}), + schema([]), + ) + .without_storage() + .with_plugin("ticket", &json!({})) + .with_runner(runner.clone()); + + let error = execute(&fixture.execution("call-1", json!({})), &Answers::new()) + .await + .unwrap_err(); + + assert_eq!( + error.to_string(), + "Command plugin `ticket` cannot be run from here: the workspace has no storage configured" + ); + assert_eq!(runner.calls(), vec![]); +} + +/// The cancellation token reaches the runner, which is what stops a running +/// plugin. +/// The mock reports a run whose token is cancelled as cancelled, so a token +/// that never reached it would come back as a completed call. +#[tokio::test] +async fn cancelling_a_command_tool_reaches_the_runner() { + let runner = plugin_answering(json!({"type": "success", "content": "ok"})); + let fixture = Fixture::new( + "ticket_create", + json!({"source": "plugin.command.ticket.create"}), + schema([]), + ) + .with_plugin("ticket", &json!({})) + .with_runner(runner.clone()); + + let execution = fixture.execution("call-1", json!({})); + execution.cancellation.cancel(); + + let outcome = execute(&execution, &Answers::new()).await.unwrap(); + + assert!(outcome.is_cancelled(), "got {outcome:?}"); + assert_eq!(runner.calls().len(), 1); +} + /// Naming a tool with `--tool` is an explicit request for it, so its schema /// error surfaces instead of the tool silently disappearing. #[tokio::test] diff --git a/crates/jp_plugin/src/message.rs b/crates/jp_plugin/src/message.rs index 546e25b86..de38e957d 100644 --- a/crates/jp_plugin/src/message.rs +++ b/crates/jp_plugin/src/message.rs @@ -47,7 +47,7 @@ pub struct PathsInfo { #[serde(tag = "type", rename_all = "snake_case")] pub enum HostToPlugin { /// Sent immediately after spawning the plugin. - Init(InitMessage), + Init(Box), /// Response to `list_conversations`. Conversations(ConversationsResponse), @@ -151,6 +151,9 @@ pub enum PluginToHost { /// Respond with plugin metadata. Describe(DescribeResponse), + /// Answer the tool call `init` carried. + ToolOutcome(ToolOutcomeMessage), + /// Signal that the plugin is done. Exit(ExitMessage), } @@ -180,9 +183,12 @@ impl PluginToHost { Self::Interrupt(m) => m.id.as_deref(), // Not requests: nothing is waiting on an answer to any of these. - Self::Ready(_) | Self::Print(_) | Self::Log(_) | Self::Describe(_) | Self::Exit(_) => { - None - } + Self::Ready(_) + | Self::Print(_) + | Self::Log(_) + | Self::Describe(_) + | Self::ToolOutcome(_) + | Self::Exit(_) => None, } } } @@ -323,6 +329,62 @@ pub struct InitMessage { /// nothing a plugin has to refuse to run without. #[serde(default)] pub output_format: OutputFormat, + + /// A tool call to answer, in place of running a command. + /// + /// Set when the assistant calls a tool whose source is + /// `plugin.command.`. + /// The plugin answers with [`PluginToHost::ToolOutcome`], then `exit`, and + /// `args` is empty. + /// `options` carries the plugin's options as the query resolved them, + /// including conversation config and `--cfg` overrides. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub tool: Option, +} + +/// One attempt of a tool call the host routes to a plugin. +/// +/// A tool that asks a question ends its attempt with a `needs_input` outcome. +/// The host runs the plugin again once the answer arrives, with the answers so +/// far in `answers`; the plugin is not kept running in between. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct ToolCall { + /// What the call asks for: running the tool, or describing the call. + #[serde(default)] + pub action: ToolAction, + + /// The name the plugin knows the tool by. + pub name: String, + + /// The arguments the assistant supplied, coerced to the tool's schema. + #[serde(default)] + pub arguments: Map, + + /// Answers to questions earlier attempts asked, keyed by question id. + #[serde(default, skip_serializing_if = "Map::is_empty")] + pub answers: Map, + + /// The tool's own `conversation.tools..options`. + /// + /// Distinct from [`InitMessage::options`], which belong to the plugin and + /// are shared by every tool it serves. + #[serde(default, skip_serializing_if = "Map::is_empty")] + pub options: Map, + + /// The tool's compiled access grants, which the plugin must enforce. + /// + /// The same policy, in the same shape, that a local tool's command reads + /// from `context.access`. + /// Absent when the tool has no policy, which means unrestricted but + /// workspace-confined filesystem access. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub access: Option, + + /// The conversation the call belongs to, in the form JP prints. + /// + /// Empty for a call with no conversation behind it. + #[serde(default, skip_serializing_if = "String::is_empty")] + pub conversation: String, } /// Workspace metadata included in the `init` message. @@ -1021,6 +1083,30 @@ impl DescribeResponse { } } +/// What a tool call asks the plugin to do. +#[derive(Debug, Clone, Copy, Default, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum ToolAction { + /// Run the tool. + #[default] + Run, + + /// Describe the call for the user, without running it. + /// + /// Sent for a tool configured with `style.parameters = "tool"`. + /// The plugin answers with a `success` outcome whose content is shown + /// verbatim above the call, and must not act on the arguments. + FormatArguments, +} + +/// The result of the tool call `init` carried. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct ToolOutcomeMessage { + /// What a local tool prints on stdout: a `success`, `error`, or + /// `needs_input` outcome, in the shape `jp_tool::Outcome` serializes to. + pub outcome: Value, +} + /// The plugin is done and wants JP to exit with this code. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub struct ExitMessage { diff --git a/crates/jp_plugin/src/message_tests.rs b/crates/jp_plugin/src/message_tests.rs index b8745dcb2..d28c01d7f 100644 --- a/crates/jp_plugin/src/message_tests.rs +++ b/crates/jp_plugin/src/message_tests.rs @@ -5,7 +5,7 @@ use super::*; #[test] fn host_init_roundtrip() { - let msg = HostToPlugin::Init(InitMessage { + let msg = HostToPlugin::Init(Box::new(InitMessage { version: 1, workspace: WorkspaceInfo { root: "/project".into(), @@ -22,13 +22,105 @@ fn host_init_roundtrip() { args: vec!["--web".to_owned()], log_level: 0, output_format: OutputFormat::JsonPretty, - }); + tool: None, + })); let json = serde_json::to_string(&msg).unwrap(); let parsed: HostToPlugin = from_str(&json).unwrap(); assert_eq!(msg, parsed); } +/// The wire shape a plugin serving tools parses, pinned so a field renamed on +/// the host side shows up here rather than as a plugin that silently stops +/// answering. +#[test] +fn host_init_with_a_tool_call_serializes_to_the_documented_shape() { + let msg = HostToPlugin::Init(Box::new(InitMessage { + version: 12, + workspace: WorkspaceInfo { + root: "/project".into(), + storage: "/project/.jp".into(), + id: "abc12".to_owned(), + }, + paths: PathsInfo::default(), + config: json!({}), + options: Map::from_iter([("dir".to_owned(), json!("packages/foo/tickets"))]), + args: vec![], + log_level: 0, + output_format: OutputFormat::Text, + tool: Some(ToolCall { + action: ToolAction::FormatArguments, + name: "create".to_owned(), + arguments: Map::from_iter([("title".to_owned(), json!("Fix it"))]), + answers: Map::new(), + options: Map::new(), + access: Some(json!({"fs": [], "net": [], "env": [{"name": "AWS_*", "read": false}]})), + conversation: "jp-c17000000000".to_owned(), + }), + })); + + assert_eq!( + serde_json::to_value(&msg).unwrap(), + json!({ + "type": "init", + "version": 12, + "workspace": {"root": "/project", "storage": "/project/.jp", "id": "abc12"}, + "paths": {}, + "config": {}, + "options": {"dir": "packages/foo/tickets"}, + "args": [], + "log_level": 0, + "output_format": "text", + "tool": { + "action": "format_arguments", + "name": "create", + "arguments": {"title": "Fix it"}, + "access": {"fs": [], "net": [], "env": [{"name": "AWS_*", "read": false}]}, + "conversation": "jp-c17000000000" + } + }) + ); +} + +/// A tool call that names no action is a run. +#[test] +fn tool_call_without_an_action_is_a_run() { + let call: ToolCall = from_str(r#"{"name":"create"}"#).unwrap(); + + assert_eq!(call.action, ToolAction::Run); + assert_eq!( + serde_json::to_value(&call).unwrap(), + json!({"action": "run", "name": "create", "arguments": {}}) + ); +} + +#[test] +fn host_init_without_a_tool_call_reads_as_a_command() { + let json = r#"{"type":"init","version":9,"workspace":{"root":"/p","storage":"/p/.jp","id":"x"},"config":{},"args":["list"]}"#; + let HostToPlugin::Init(init) = from_str(json).unwrap() else { + panic!("expected Init"); + }; + + assert_eq!(init.tool, None); +} + +#[test] +fn plugin_tool_outcome_roundtrip() { + let msg = PluginToHost::ToolOutcome(ToolOutcomeMessage { + outcome: json!({"type": "success", "content": "Created T-0abc123"}), + }); + // Compared as values: key order inside `outcome` depends on whether + // `serde_json`'s `preserve_order` is unified into this build. + let json = serde_json::to_string(&msg).unwrap(); + assert_eq!( + from_str::(&json).unwrap(), + json!({"type": "tool_outcome", "outcome": {"type": "success", "content": "Created T-0abc123"}}) + ); + + let parsed: PluginToHost = from_str(&json).unwrap(); + assert_eq!(msg, parsed); +} + #[test] fn host_init_deserializes_from_json() { let json = r#"{"type":"init","version":1,"workspace":{"root":"/p","storage":"/p/.jp","id":"x"},"config":{},"args":[]}"#; diff --git a/crates/jp_plugin/src/protocol.rs b/crates/jp_plugin/src/protocol.rs index b38051775..7868420b1 100644 --- a/crates/jp_plugin/src/protocol.rs +++ b/crates/jp_plugin/src/protocol.rs @@ -20,7 +20,8 @@ use crate::message::{ExitMessage, ReadyMessage}; /// | 9 | `action` on `interrupt`, and an answer when it carries an `id`. | /// | 10 | `reload` on `read_config`, reading the configuration again. | /// | 11 | `schema` and `expires_in` on `query`, `data` on its reply. | -pub const PROTOCOL_VERSION: u32 = 11; +/// | 12 | `tool` on `init`, answered with `tool_outcome`. | +pub const PROTOCOL_VERSION: u32 = 12; /// Answer a host's `init`, refusing it when it is too old to serve this plugin. /// diff --git a/crates/jp_tool/src/error.rs b/crates/jp_tool/src/error.rs index e941769f6..d23c4efee 100644 --- a/crates/jp_tool/src/error.rs +++ b/crates/jp_tool/src/error.rs @@ -30,6 +30,17 @@ pub enum Error { #[error("Failed to run tool from MCP client")] McpRunToolError(#[source] Box), + /// A tool served by a command plugin, when the plugin cannot be started: it + /// is not installed, its run policy forbids it, or nothing here can run + /// plugins at all. + #[error("Command plugin `{plugin}` cannot be run from here: {reason}")] + CommandPluginUnavailable { plugin: String, reason: String }, + + /// A command plugin started, then failed to answer the tool call: it exited + /// with an error, broke the protocol, or ended without an outcome. + #[error("Command plugin `{plugin}` failed: {message}")] + CommandPluginFailed { plugin: String, message: String }, + #[error("Failed to spawn command: {command}")] SpawnError { command: String, diff --git a/crates/plugins/command/gui/src/lib_tests.rs b/crates/plugins/command/gui/src/lib_tests.rs index 215819f66..476d48021 100644 --- a/crates/plugins/command/gui/src/lib_tests.rs +++ b/crates/plugins/command/gui/src/lib_tests.rs @@ -49,11 +49,12 @@ fn init_message(root: &str, args: &[&str]) -> String { args: args.iter().map(|a| (*a).to_owned()).collect(), log_level: 0, output_format: OutputFormat::default(), + tool: None, }; format!( "{}\n", - serde_json::to_string(&HostToPlugin::Init(init)).unwrap() + serde_json::to_string(&HostToPlugin::Init(Box::new(init))).unwrap() ) } diff --git a/crates/plugins/command/ticket/src/lib_tests.rs b/crates/plugins/command/ticket/src/lib_tests.rs index 2b4f88130..064432d5b 100644 --- a/crates/plugins/command/ticket/src/lib_tests.rs +++ b/crates/plugins/command/ticket/src/lib_tests.rs @@ -172,7 +172,7 @@ fn init(root: &Utf8Path, args: &[&str]) -> HostToPlugin { } fn init_at(version: u32, root: &Utf8Path, args: &[&str]) -> HostToPlugin { - HostToPlugin::Init(InitMessage { + HostToPlugin::Init(Box::new(InitMessage { version, workspace: WorkspaceInfo { root: root.to_path_buf(), @@ -189,7 +189,8 @@ fn init_at(version: u32, root: &Utf8Path, args: &[&str]) -> HostToPlugin { args: args.iter().map(|arg| (*arg).to_owned()).collect(), log_level: 0, output_format: OutputFormat::default(), - }) + tool: None, + })) } #[test] diff --git a/docs/features/tools.md b/docs/features/tools.md index 7adfe598d..0efb3fdf6 100644 --- a/docs/features/tools.md +++ b/docs/features/tools.md @@ -14,12 +14,14 @@ definition. Every tool has a `source`: -| Source | Definition | -| --------------------- | ------------------------------------------------------------------------------- | -| `builtin` | Implemented inside JP. | -| `local` | Runs a configured command. JP configuration defines its parameters. | -| `mcp.` | Uses the same-named tool from an MCP server. The server defines its parameters. | -| `mcp..` | Uses a differently named tool from an MCP server. This allows a local alias. | +| Source | Definition | +| -------------------------------- | ---------------------------------------------------------------------------------- | +| `builtin` | Implemented inside JP. | +| `local` | Runs a configured command. JP configuration defines its parameters. | +| `mcp.` | Uses the same-named tool from an MCP server. The server defines its parameters. | +| `mcp..` | Uses a differently named tool from an MCP server. This allows a local alias. | +| `plugin.command.` | Asks a command plugin to answer the call. JP configuration defines its parameters. | +| `plugin.command..` | Asks a command plugin for a differently named tool. | For example, a local tool starts with a complete definition: @@ -45,6 +47,23 @@ source = "mcp.notes.create_note" JP fetches the MCP tool definition from the server when resolving the tools for a query. +A command plugin tool runs through the plugin, which receives +`plugins.command..options` as the query resolved them: + +```toml +[conversation.tools.ticket_create] +source = "plugin.command.ticket.create" + +[conversation.tools.ticket_create.style] +# Ask the plugin to describe the call before it runs. +parameters = "tool" +``` + +`style.parameters = "tool"` asks the tool itself to format its arguments: a +local tool's command, or the plugin, runs with the action `format_arguments` +instead of `run`. +Only local and command plugin tools support it. + ## Parameter Schemas A parameter schema describes one argument the model may send in a tool call. diff --git a/docs/rfd/072-command-plugin-system.md b/docs/rfd/072-command-plugin-system.md index 47173b3dd..87dc9c171 100644 --- a/docs/rfd/072-command-plugin-system.md +++ b/docs/rfd/072-command-plugin-system.md @@ -421,6 +421,115 @@ it. Paths that resolve to the same file, such as two symlinks into one package store, are one binary. +#### Tool Calls + +A plugin can serve tools to the assistant. +The tool names the plugin as its source: + +```toml +[conversation.tools.ticket_create] +source = "plugin.command.ticket.create" +summary = "File a work item as a ticket." + +[conversation.tools.ticket_create.parameters.title] +type = "string" +required = true +``` + +`plugin.command.` uses the configured tool name; +`plugin.command..` names the tool the plugin knows it by. +The `command` segment names the plugin kind, the same one that keys +`plugins.command.`. +Parameters come from configuration, as they do for a `local` tool. + +For each attempt, JP starts the plugin and sends an `init` that carries a `tool` +and no `args`: + +```json +{ + "type": "init", + "version": 12, + "workspace": { "root": "/path/to/project", "storage": "/path/to/project/.jp", "id": "a1b2c" }, + "config": {}, + "options": { "dir": "packages/foo/tickets" }, + "args": [], + "log_level": 0, + "output_format": "text", + "tool": { + "action": "run", + "name": "create", + "arguments": { "title": "Fix the header" }, + "answers": {}, + "options": {}, + "access": { "fs": [], "net": [], "env": [{ "name": "AWS_*", "read": false }] }, + "conversation": "jp-c17000000000" + } +} +``` + +`config` and `options` are the query's resolved configuration, not the workspace +root's: nested `.jp.toml` files, conversation config, and `--cfg` overrides all +reach the plugin. +`tool.options` is the tool's own `conversation.tools..options`. + +`tool.action` is `run`, or `format_arguments` for a tool styled +`style.parameters = "tool"`. +A `format_arguments` call asks the plugin to describe the call for the user +without acting on it; the content of its `success` outcome is shown verbatim +above the call. +It runs under the same admission, options, and access policy as the run, and the +tool's `format` setting decides whether it runs before or after the user +approves the call, as for a formatter command. + +The plugin answers with one `tool_outcome`, then `exit`: + +```json +{ "type": "tool_outcome", "outcome": { "type": "success", "content": "Created T-0abc123" } } +``` + +`outcome` has the shape a local tool prints on stdout: `success`, `error`, or +`needs_input`. +The call is finished by the `exit`: an outcome not followed by `exit` fails the +call, as a plugin that answered and then crashed has not finished it. +A question ends the attempt. +JP runs the plugin again once it has an answer, with the answers so far in +`tool.answers`. + +A tool call runs once, like a `local` tool's command. +The plugin's stdin carries the `init` and is then closed, so a request the +plugin sends during a tool call gets no answer: everything it needs is in +`init`, and the turn owns the terminal. +`print` output is dropped; the result is the outcome. +A cancelled call kills the plugin, and on Unix every process it started, as it +does a `local` tool's command. +Tool calls need protocol 12. + +A plugin tool runs under the same rules as a `local` tool. +The tool's `run`, `result`, and `format` settings decide whether a call is asked +about, edited, or skipped, exactly as for any other source. +Its `access` grants, including those inherited from +`conversation.tools.'*'.access` and added by `--mount`, are compiled by the host +and sent as `tool.access`; the plugin enforces them, as a local tool binary +does. + +The plugin binary itself is admitted under `plugins.command..run`, the +same way `jp ` admits it. +Admission happens once per plugin when the turn starts, before any tool runs, so +the user is never asked about a binary in the middle of a tool call. +Answering "run this time" admits the binary for that turn. +Without a terminal, a binary that would be asked about is refused, as `jp +` refuses it. +Nothing is installed at turn start: an official plugin that is missing is +refused, and `jp plugin install` or one `jp ` run fetches it. + +Admission decides on the binary's contents. +Before each call the host hashes the binary again, and refuses the call if it +changed since the turn started. + +A plugin that is not admitted has its tools left out of the turn, and the turn +names them and the reason on stderr; naming one of them with `--tool` fails the +query instead. + #### Plugin Self-Description An admitted plugin describes itself in full over the protocol. diff --git a/docs/ticket/0vm44ks-pass-a-plugin-s-effective-options-to-the-local-tools-it-serv.md b/docs/ticket/0vm44ks-pass-a-plugin-s-effective-options-to-the-local-tools-it-serv.md new file mode 100644 index 000000000..9eff19bd1 --- /dev/null +++ b/docs/ticket/0vm44ks-pass-a-plugin-s-effective-options-to-the-local-tools-it-serv.md @@ -0,0 +1,63 @@ +# Pass a plugin's effective options to the local tools it serves + +- **Status**: Done +- **Kind**: Feature +- **Authors**: jp +- **Date**: 2026-10-01 +- **Label**: domain=plugins +- **Label**: package=jp_mcp + +A local tool whose command runs a command plugin (`jp ticket tool {{context}} +{{tool}}`) gets a child `jp` that resolves configuration on its own, from the +workspace root. +The query's own configuration does not reach it. + +The tool context carries the per-tool options and invocation ids, not +`plugins.command.*` (`crates/jp_mcp/src/server.rs:53-67`). +The command starts in the workspace root (`server.rs:497`). +External commands load no conversation config +(`crates/jp_cli/src/cmd.rs:104-115`). + +Input that hits it: a monorepo where `packages/foo/.jp.toml` sets +`plugins.command.ticket.options.dir`. +A query run from `packages/foo` sees that setting; the `ticket_create` call it +makes writes to the ticket directory configured at the root. +Query `--cfg` overrides and conversation config are lost the same way. + +The tool result names the path it wrote, so the mistake is visible, but the file +stays where it landed. + +RFD 116 documents this limit for v1. +The fix is the host passing the effective options of the plugin a tool runs +through, so the child uses them instead of resolving its own. +That needs a decision on how a tool declares which plugin it runs through. + +## Comments + +----- + +- **From**: jp +- **Date**: 2026-10-02T14:26:08Z + +Fixed on the host side: a tool declares the plugin it runs through with `source += "plugin.command.[.]"`, and JP runs the plugin itself with the +turn's resolved `plugins.command..options` in `init.options`. +No child `jp` resolves config any more, so nested `.jp.toml` files, conversation +config, and `--cfg` all reach the plugin. + +The ticket tools still need to adopt it on the RFD 116 branch: + +- `jp-ticket` answers an `init` carrying `tool`: run `tool.name` with + `tool.arguments`, reply with `tool_outcome`, then `exit 0`. + Stdin is closed after `init`, so the tool path must not `compose` or + `read_config`. +- On `tool.action == "format_arguments"`, return the preview as a `success` + outcome and write nothing. +- Read the ticket directory from `init.options["dir"]` when `--dir` is absent. +- Enforce `tool.access` when present. +- `REQUIRED_PROTOCOL = 12`. +- `.jp/mcp/tools/ticket/*.toml`: `source = "plugin.command.ticket."`, drop + `command`, and `style.parameters = "tool"`. +- Admit the binary: `plugins.command.ticket.run = "allow"` in `.jp/config.toml`, + or `jp plugin approve`. + Otherwise the turn drops the ticket tools and says why. diff --git a/docs/ticket/0vnd5xf-cancelling-a-local-tool-leaves-the-processes-it-started-runn.md b/docs/ticket/0vnd5xf-cancelling-a-local-tool-leaves-the-processes-it-started-runn.md new file mode 100644 index 000000000..902478efa --- /dev/null +++ b/docs/ticket/0vnd5xf-cancelling-a-local-tool-leaves-the-processes-it-started-runn.md @@ -0,0 +1,52 @@ +# Cancelling a local tool leaves the processes it started running + +- **Status**: Todo +- **Kind**: Bug +- **Authors**: jp +- **Date**: 2026-10-01 +- **Label**: domain=plugins +- **Label**: package=jp_cli +- **Label**: package=jp_mcp + +A cancelled local tool call kills only the process JP spawned. +`run_tool_command` puts the child in its own process group +(`crates/jp_mcp/src/server.rs`, `cmd.process_group(0)`), then relies on +`kill_on_drop(true)`, which signals the child's pid, not the group. +A tool command that is a shell or a `just` recipe (every tool under +`.jp/mcp/tools/` runs `just serve-tools ...`) has its real work in a grandchild, +which survives the cancel and keeps running, writing files, or holding a port. + +Tools served by command plugins (`source = "plugin.command."`, +T-0vm44ks) are spawned the same way in `crates/jp_cli/src/cmd/plugin/tool.rs` +and have the same gap. + +`jp ` commands do not: on the plugin-routing branch (PR #1216) they are +stopped through `ProcessTree`, which kills the process group on Unix and the job +object on Windows. + +Input that hits it: a local tool whose command is `sh -c 'sleep 600'`, cancelled +with Ctrl+C mid-call. +The `sh` exits; the `sleep` is still running afterwards. + +Fix: kill the tool's whole tree on cancel, for every subprocess-backed source, +reusing `ProcessTree` once PR #1216 lands. +Test with a command that starts a background worker and writes its pid, cancel +the call, and assert the worker is gone, as +`process_tree_tests::terminating_the_tree_kills_what_the_plugin_started` does. + +## Comments + +----- + +- **From**: jp +- **Date**: 2026-10-01T15:15:06Z + +#1225 merged `jp_process` with group-aware stopping: a process spawned with +`own_process_group` is stopped by signalling `-pid` (SIGINT, then SIGKILL once +`Watch::grace` runs out), even after the leader itself has exited. +Command plugin tool calls (T-0vm44ks) run through `ProcessRunner` with +`own_process_group = true`, so on Unix they are covered once that branch is on +main's `jp_process`. +Local tools are covered when `run_tool_command` moves onto `ProcessRunner` +(#1228 in the #1175 split). +Windows still kills only the child: `jp_process` has no job object. diff --git a/docs/ticket/0vnx563-let-github_pulls-find-open-prs-that-touch-a-path.md b/docs/ticket/0vnx563-let-github_pulls-find-open-prs-that-touch-a-path.md new file mode 100644 index 000000000..a08c84955 --- /dev/null +++ b/docs/ticket/0vnx563-let-github_pulls-find-open-prs-that-touch-a-path.md @@ -0,0 +1,31 @@ +# Let github_pulls find open PRs that touch a path + +- **Status**: Todo +- **Kind**: Feature +- **Authors**: jp +- **Date**: 2026-10-01 +- **Label**: domain=tooling + +`github_pulls` lists pull requests by state only +(`.config/jp/tools/src/github/pulls.rs`, `list`). +`github_code_search` searches the default branch only, so code that exists only +in an open PR is invisible to it. +Answering "which open PR adds `crates/jp_process`?" takes one `github_pr_diff` +call per open PR, and an assistant that skips some of them answers wrong. + +That happened during T-0vm44ks: the search covered the code index and one PR's +file list, found nothing, and reported that no shared process runner existed. +PR #1175 added it. +The user found it with a shell loop over `gh api repos/dcdpr/jp/pulls/$n/files`. + +Fix: add an optional `path` parameter to `github_pulls`, a path prefix. +With it set, the tool lists the PRs matching `state` and keeps those whose +changed files include a path under the prefix, naming the matching files for +each. +The file lists are paginated (100 per page, up to 3000 files), so every page is +read; a PR with more files than the API returns says so rather than being +reported as a non-match. + +Test with a mocked client: two open PRs, one touching +`crates/jp_process/src/lib.rs`, filtered on `crates/jp_process`, returns only +that PR and that file.